|Summary:||CVE-2016-3106 pulp: Insecure creation of temporary directory when generating new CA key|
|Product:||[Other] Security Response||Reporter:||Adam Mariš <amaris>|
|Component:||vulnerability||Assignee:||Red Hat Product Security <security-response-team>|
|Status:||CLOSED NOTABUG||QA Contact:|
|Version:||unspecified||CC:||fweimer, rbarlow, sean.myers, security-response-team|
|Fixed In Version:||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2019-06-08 02:50:25 UTC||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Bug Depends On:|
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 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