Bug 1324926 (CVE-2016-3106) - CVE-2016-3106 pulp: Insecure creation of temporary directory when generating new CA key
Summary: CVE-2016-3106 pulp: Insecure creation of temporary directory when generating ...
Keywords:
Status: CLOSED NOTABUG
Alias: CVE-2016-3106
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
low
low
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: 1322708
TreeView+ depends on / blocked
 
Reported: 2016-04-07 15:22 UTC by Adam Mariš
Modified: 2021-02-17 04:05 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-06-08 02:50:25 UTC
Embargoed:


Attachments (Terms of Use)
Proposed patch (1.54 KB, patch)
2016-04-07 15:24 UTC, Adam Mariš
no flags Details | Diff
Second patch proposal (2.71 KB, patch)
2016-04-10 19:21 UTC, Randy Barlow
no flags Details | Diff
Second patch proposal (3.38 KB, patch)
2016-04-11 12:27 UTC, Randy Barlow
no flags Details | Diff
Proposed patch (3.64 KB, patch)
2016-04-12 13:44 UTC, Randy Barlow
no flags Details | Diff

Description Adam Mariš 2016-04-07 15:22:21 UTC
It was found that fix for CVE-2016-3095 was incomplete, introducing new vulnerabilities due to insecure way of creating the temporary directory when generating new CA key.

Comment 1 Adam Mariš 2016-04-07 15:22:26 UTC
Acknowledgments:

Name: Florian Weimer (Red Hat), Sander Bos

Comment 2 Adam Mariš 2016-04-07 15:24:04 UTC
Created attachment 1144778 [details]
Proposed patch

Comment 3 Randy Barlow 2016-04-10 19:18:34 UTC
For the record, Sander Bos has also independently reported this issue to the Pulp team. Thanks to both Florian and Sander!

Comment 4 Randy Barlow 2016-04-10 19:21:02 UTC
Created attachment 1145757 [details]
Second patch proposal

Sander Bos recommended also setting the umask on the patch, which I think is a good addition to the patch. I am attaching that version of the patch here as well.

Comment 5 Randy Barlow 2016-04-11 12:27:56 UTC
Created attachment 1145973 [details]
Second patch proposal

Here is the same patch with attribution included in the AUTHORS file.

Comment 6 Randy Barlow 2016-04-12 13:44:16 UTC
Created attachment 1146458 [details]
Proposed patch

I discovered that the umask statement in my second revision of the patch was unnecessary, so this revision removes that. Additionally, this version adds a -Z to the mv statements at the end so that the correct SELinux label will get applied when the certificate and key are moved to their final destination.

Comment 7 Florian Weimer 2016-04-12 14:25:20 UTC
(In reply to Randy Barlow from comment #6)
> Created attachment 1146458 [details]
> Proposed patch
> 
> I discovered that the umask statement in my second revision of the patch was
> unnecessary, so this revision removes that. Additionally, this version adds
> a -Z to the mv statements at the end so that the correct SELinux label will
> get applied when the certificate and key are moved to their final
> destination.

Unfortunately, “mv -Z” has a race condition and is unsafe if the destination directory is owned by an untrusted user.  If the directory is owned by root, it is fine in this context, but I don't know if this applies here.

Comment 8 Randy Barlow 2016-04-12 14:54:16 UTC
Hello Florian, thanks for letting me know about the race condition, I was not aware. Here are the permissions on the destination folder:

$ ls -lah /etc/pki/pulp | head -n 6
total 28K
drwxr-xr-x.  3 root root   4.0K Apr 10 13:31 .
drwxr-xr-x. 10 root root   4.0K Apr 10 13:29 ..
-rw-r-----.  1 root apache 1.8K Apr 10 13:31 ca.crt
-rw-r-----.  1 root apache 3.2K Apr 10 13:31 ca.key
drwxr-xr-x.  2 root root   4.0K Apr 10 13:29 nodes

Thus this CA does get installed to a directory owned by root.

However, do you have a proposal of a better way to get the correct SELinux contexts applied that would be safe in all conditions?

Comment 9 Randy Barlow 2016-04-13 16:44:29 UTC
This issue is filed upstream as #1827 and is fixed by PR #2527:

https://pulp.plan.io/issues/1827
https://github.com/pulp/pulp/pull/2527


Note You need to log in before you can comment on or make changes to this bug.