Bug 13834 - (CVE-2019-3870) CVE-2019-3870 [SECURITY] pysmbd: missing restoration of original umask after umask(0)
(CVE-2019-3870)
CVE-2019-3870 [SECURITY] pysmbd: missing restoration of original umask after ...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-12 21:17 UTC by Björn Baumbach
Modified: 2019-04-09 07:50 UTC (History)
11 users (show)

See Also:


Attachments
pysmbd patch (815 bytes, patch)
2019-03-12 21:22 UTC, Björn Baumbach
abartlet: review-
Details
Initial advisory (2.03 KB, text/plain)
2019-03-14 04:20 UTC, Andrew Bartlett
no flags Details
WIP patch for master (3.83 KB, patch)
2019-03-14 05:28 UTC, Andrew Bartlett
dbagnall: review+
Details
Integration test to highlight the problem (4.05 KB, patch)
2019-03-15 01:13 UTC, Tim Beale
no flags Details
Unit-style smbd test (4.76 KB, text/plain)
2019-03-15 03:06 UTC, Tim Beale
no flags Details
Integration test to highlight the problem (4.17 KB, text/plain)
2019-03-15 05:17 UTC, Tim Beale
no flags Details
WIP patch for master (v2) (19.57 KB, patch)
2019-03-21 05:00 UTC, Andrew Bartlett
jra: review+
Details
patch for master (v3) (19.79 KB, patch)
2019-03-26 02:02 UTC, Andrew Bartlett
no flags Details
patch for master (v4) (19.79 KB, patch)
2019-03-26 02:04 UTC, Andrew Bartlett
garming: review+
abartlet: review+
Details
patch for 4.10 (v4) (19.79 KB, patch)
2019-03-26 02:05 UTC, Andrew Bartlett
garming: review+
abartlet: review+
Details
patch for 4.9 (v4) (19.83 KB, patch)
2019-03-26 02:05 UTC, Andrew Bartlett
garming: review+
abartlet: review+
Details
advisory with CVE (v2) (2.05 KB, text/plain)
2019-03-26 02:16 UTC, Andrew Bartlett
no flags Details
advisory with CVE (v3) (2.31 KB, text/plain)
2019-03-26 03:18 UTC, Garming Sam
abartlet: review+
Details
advisory with CVE (v4) (2.32 KB, text/plain)
2019-03-27 03:39 UTC, Andrew Bartlett
garming: review+
Details
advisory with CVE (v5) (2.65 KB, text/plain)
2019-03-29 04:26 UTC, Andrew Bartlett
dbagnall: review+
abartlet: review+
Details
advisory with CVE (v6) (2.65 KB, text/plain)
2019-04-01 01:03 UTC, Andrew Bartlett
garming: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Baumbach 2019-03-12 21:17:05 UTC
In source3/smbd/pysmbd.c the set_nt_acl_conn() function calls init_files_struct() which sets:

saved_umask = umask(0);

But the umask(saved_umask) to restore the original umask is missing for the non-error case. This pysmbd code is used by the ntacl-calls for settings the sysvol ACLs.

This leads to world writeable files in the private dir, which have been created by samba-tool domain provision:

openat(AT_FDCWD, "/var/lib/samba/private/krb5.conf",
O_WRONLY|O_CREAT|O_TRUNC, 0666) = 26
openat(AT_FDCWD, "/var/lib/samba/private/dns_update_list",
O_WRONLY|O_CREAT|O_TRUNC, 0666) = 26
openat(AT_FDCWD, "/var/lib/samba/private/spn_update_list",
O_WRONLY|O_CREAT|O_TRUNC, 0666) = 26

root@bb-ubuntu-bio:/var/lib/samba/private# ls -l | grep -- -rw-rw-rw-
-rw-rw-rw- 1 root root    3663 Mar 12 16:05 dns_update_list
-rw-rw-rw- 1 root root      95 Mar 12 16:05 krb5.conf
-rw-rw-rw- 1 root root     955 Mar 12 16:05 spn_update_list

This has been introduced with Samba 4.9 with the commit 747c3f1fb379bb68cc7479501b85741493c05812.
Comment 1 Björn Baumbach 2019-03-12 21:22:22 UTC
Created attachment 14923 [details]
pysmbd patch
Comment 2 Andrew Bartlett 2019-03-13 23:24:44 UTC
Removing private marker from the patch and description to Joe (who isn't on the team) can work on it. 

The while bug is protected by the samba-devel restriction, while also allowing access to those on the CC list.
Comment 3 Andrew Bartlett 2019-03-14 04:20:57 UTC
Created attachment 14928 [details]
Initial advisory

I've asked Red Hat Product Security for a CVE.
Comment 4 Andrew Bartlett 2019-03-14 04:37:07 UTC
Comment on attachment 14923 [details]
pysmbd patch

I don't think this patch is quite right, the init_files_struct shouldn't be setting the umask or un-setting it, shouldn't that be in a wrapper be around SMB_VFS_FSET_NT_ACL() in set_nt_acl_conn()?

(As this the routine setting the file permissions).
Comment 5 Andrew Bartlett 2019-03-14 05:28:34 UTC
Created attachment 14929 [details]
WIP patch for master

This needs some very careful examination, plus some work to understand exactly which operations we actually need to change the umask for.  However I think it is better and cleaner than the original patch. 

TODO: 
 * Add tests to pin down expected permission behaviour of each pysmbd API, smbd.create_file in particular
 * Add integration test for provision (that it doesn't create world-writable files)
 * confirm the umask is unchanged after all pysmbd calls.
Comment 6 Andrew Bartlett 2019-03-14 06:02:17 UTC
I've confirmed both patches work, but frustratingly something is changing the umask in our test system to 0000 so files created by 'make testenv' are still 0666.

This in turn makes it much harder to write tests that prove either patches do what is expected.

This may be a can of worms.  :-(
Comment 7 Andrew Bartlett 2019-03-14 06:06:20 UTC
selftest.pl has:

 # We need to have no umask limitations for the tests.
 umask 0000;
Comment 8 Tim Beale 2019-03-15 01:13:52 UTC
Created attachment 14930 [details]
Integration test to highlight the problem

Attached is a integration test that demonstrates the problem. Note that I've skipped checking the file permissions of directories and sockets in private/ - this excludes (along with other dirs) the following, but I'm guessing their  permissions are expected.

timbeale@timbeale-pc:~/code/samba$ ls -l st/ad_dc/private/
total 12488
srwxrwxrwx 1 timbeale timbeale       0 Mar 15 12:53 ldapi
drwxr-x--- 2 timbeale timbeale    4096 Mar 15 12:53 ldap_priv
drwxr-xr-x 2 timbeale timbeale    4096 Mar 15 12:53 smbd.tmp
Comment 9 Tim Beale 2019-03-15 03:06:32 UTC
Created attachment 14931 [details]
Unit-style smbd test

Attached is test that the individual smbd APIs don't mess with the umask. It seems to work a bit better - it fails without Andrew's patch and passes with it.

The integration test I added earlier still needs some work - I think it needs to set the umask at the start too, but I'm still trying to get my head around what behaviour we should be asserting in that case.
Comment 10 Tim Beale 2019-03-15 05:17:09 UTC
Created attachment 14932 [details]
Integration test to highlight the problem

I think this test now fails in the bad case and passes in the good case. Updated it to just check there are no world-writable files.
Comment 11 Andrew Bartlett 2019-03-18 00:14:40 UTC
Can I get some assistance from someone who really understands the smbd internals being used by pysmbd and which actions strictly need umask() changes?

It would be good to get a solid understanding of this area to complement the simpler confirmation that we don't leak the umask() change.

Sadly all I know is that I added the umask() changes originally in:

commit e146fe5ef96c1522175a8e81db15d1e8879e5652
Author: Andrew Bartlett <abartlet@samba.org>
Date:   Fri Oct 26 14:22:07 2012 +1100

    pysmbd: Set umask to 0 during smbd operations

But I didn't record why they were needed beyond the simple comment about freedom to fully set file permissions:

+        /* we want total control over the permissions on created files,
+          so set our umask to 0 */
Comment 12 Jeremy Allison 2019-03-20 20:08:13 UTC
Comment on attachment 14929 [details]
WIP patch for master

Patch isn't complete I think. You also need to set/reset the umask inside:

py_smbd_set_simple_acl()
py_smbd_set_sys_acl()
py_smbd_mkdir()
Comment 13 Andrew Bartlett 2019-03-21 04:58:41 UTC
(In reply to Jeremy Allison from comment #12)
You are correct on mkdir, I can't see how the others use umask.
Comment 14 Andrew Bartlett 2019-03-21 05:00:25 UTC
Created attachment 14968 [details]
WIP patch for master (v2)

I've added additional testing to confirm behaviour around create_file() and mkdir().
Comment 15 Jeremy Allison 2019-03-21 17:07:52 UTC
I just went and looked inside Andreas Grunbacher's sys ACL library source code, and yes I can't find a use of the umask() call. Just checking (paranoia-is-us :-).
Comment 16 Jeremy Allison 2019-03-22 21:42:58 UTC
Comment on attachment 14968 [details]
WIP patch for master (v2)

OK, I'm good with this thanks !
Comment 17 Andrew Bartlett 2019-03-25 20:56:19 UTC
Adding Arvid as he just raised the same issue with me independently.  This should keep us all on the same page.
Comment 18 Andrew Bartlett 2019-03-26 02:02:23 UTC
Created attachment 14985 [details]
patch for master (v3)

Identical to attachment 14968 [details] except adding Jeremy's review.
Comment 19 Andrew Bartlett 2019-03-26 02:04:10 UTC
Created attachment 14986 [details]
patch for master (v4)

Sorry, v3 for master was the wrong file.
Comment 20 Andrew Bartlett 2019-03-26 02:05:00 UTC
Created attachment 14987 [details]
patch for 4.10 (v4)
Comment 21 Andrew Bartlett 2019-03-26 02:05:40 UTC
Created attachment 14988 [details]
patch for 4.9 (v4)
Comment 22 Andrew Bartlett 2019-03-26 02:16:34 UTC
Created attachment 14989 [details]
advisory with CVE (v2)

Advisory, pending release date and so release number.
Comment 23 Garming Sam 2019-03-26 03:18:03 UTC
Created attachment 14990 [details]
advisory with CVE (v3)
Comment 24 Arvid Requate 2019-03-26 10:48:07 UTC
Do you have a time frame for the embargo? We would like to release our fixed samba packages ASAP. Your patch covers more ground, of course.
Comment 25 Karolin Seeger 2019-03-26 12:42:03 UTC
(In reply to Arvid Requate from comment #24)
We are working on setting a release date. Please stay tuned.
Comment 26 Andrew Bartlett 2019-03-27 03:39:57 UTC
Created attachment 14995 [details]
advisory with CVE (v4)

Adding release versions.
Comment 27 Karolin Seeger 2019-03-27 10:56:45 UTC
(In reply to Karolin Seeger from comment #25)
Proposed release date: Monday, April 8 2019.
Comment 28 Andrew Bartlett 2019-03-29 04:26:32 UTC
Created attachment 15018 [details]
advisory with CVE (v5)
Comment 29 Mathieu Parent 2019-03-31 07:24:41 UTC
Would there be a message announcing the security release one week before?
Comment 30 Andrew Bartlett 2019-03-31 08:04:45 UTC
(In reply to Mathieu Parent from comment #29)
Yes, that is on my TODO list for the office tomorrow. 

Samba Team policy is for a 7-day pre-announcement for all security releases.
Comment 31 Andrew Bartlett 2019-04-01 01:03:09 UTC
Created attachment 15029 [details]
advisory with CVE (v6)

Add numerical CVE score.
Comment 32 Andrew Bartlett 2019-04-08 08:44:17 UTC
Samba 4.10.2 and 4.9.6 Security Releases were made to address this issue.

Leaving open until patches land in master, removing embargo.
Comment 33 Karolin Seeger 2019-04-08 10:32:42 UTC
Pushed to v4-8-test, v4-9-test, v4-10-test and autobuild-master.
Comment 34 Karolin Seeger 2019-04-09 07:50:46 UTC
Pushed to master.
Closing out bug report.

Thanks!