Created attachment 14923 [details]
pysmbd patch

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.

Created attachment 14928 [details]
Initial advisory

I've asked Red Hat Product Security for a CVE.

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).

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.

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.  :-(

selftest.pl has:

 # We need to have no umask limitations for the tests.
 umask 0000;

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

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.

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.

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 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()

(In reply to Jeremy Allison from comment #12)
You are correct on mkdir, I can't see how the others use umask.

Created attachment 14968 [details]
WIP patch for master (v2)

I've added additional testing to confirm behaviour around create_file() and mkdir().

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 on attachment 14968 [details]
WIP patch for master (v2)

OK, I'm good with this thanks !

Adding Arvid as he just raised the same issue with me independently.  This should keep us all on the same page.

Created attachment 14985 [details]
patch for master (v3)

Identical to attachment 14968 [details] except adding Jeremy's review.

Created attachment 14986 [details]
patch for master (v4)

Sorry, v3 for master was the wrong file.

Created attachment 14987 [details]
patch for 4.10 (v4)

Created attachment 14988 [details]
patch for 4.9 (v4)

Created attachment 14989 [details]
advisory with CVE (v2)

Advisory, pending release date and so release number.

Created attachment 14990 [details]
advisory with CVE (v3)

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.

(In reply to Arvid Requate from comment #24)
We are working on setting a release date. Please stay tuned.

Created attachment 14995 [details]
advisory with CVE (v4)

Adding release versions.

(In reply to Karolin Seeger from comment #25)
Proposed release date: Monday, April 8 2019.

Created attachment 15018 [details]
advisory with CVE (v5)

Would there be a message announcing the security release one week before?

(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.

Created attachment 15029 [details]
advisory with CVE (v6)

Add numerical CVE score.

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.

Pushed to v4-8-test, v4-9-test, v4-10-test and autobuild-master.

Pushed to master.
Closing out bug report.

Thanks!