Bug 1324926 (CVE-2016-3106)

Summary: CVE-2016-3106 pulp: Insecure creation of temporary directory when generating new CA key
Product: [Other] Security Response Reporter: Adam Mariš <amaris>
Component: vulnerabilityAssignee: Red Hat Product Security <security-response-team>
Status: CLOSED NOTABUG QA Contact:
Severity: low Docs Contact:
Priority: low    
Version: unspecifiedCC: fweimer, rbarlow, sean.myers, security-response-team
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard: impact=low,public=20160519,reported=20160406,source=redhat,cvss2=2.6/AV:L/AC:H/Au:N/C:P/I:P/A:N,cwe=CWE-377,rhn_satellite_6/pulp=notaffected,rhui-2/pulp=notaffected
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2019-06-08 02:50:25 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 1322708    
Attachments:
Description Flags
Proposed patch
none
Second patch proposal
none
Second patch proposal
none
Proposed patch none

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