Created attachment 8362 [details] patch to add test case for "write list" I've been chasing down a bug for work, where it was reported that "write list" (which overrides the read only=yes in smb.conf) does not work with Samba 4.0. This can be fixed by reverting 4544c52fc432c4eb5ba45389519d00923d9698ca. However, this made me look into the whole situation around conn->share_mask. It appears that this member of connection_struct is set during the tree connect so it can be used along side the per-file ACL mask handling. This was added in: commit 720fa46f9443ccbe471b265f1c2b9cb9782a3c26 Author: Volker Lendecke <vl@samba.org> Date: Mon Jul 4 18:35:21 2011 +0200 s3: Calculate&store the maximum share access mask Signed-off-by: Stefan Metzmacher <metze@samba.org> However, due to the way this code works, a new user connecting to a share on the same tree connect will not have a new 'conn' structure, but re-uses the structure esablished by the first user. The code in smbd/uid.c:check_user_ok() will replace the read_only and session_info elements, but not the conn->share_access element. The original code also appears to be the confusing factor here, as the share ACL (rarely used ability to set an NT ACL on a share, using sharesec or a windows GUI tool) is additionally checked before change_to_user(), probably to give back a nicer error, and then along with the other access checks, in change_to_user(). As we (essentially) always honour posix permissions and the read only flag as set on the connection, I see this as a correctness issue - we could get an odd interaction between the mask from the share ACL and the mask from the file ACL. The fix, as I see it, is to push all the per-user access control stuff back into the change_to_user code, that is always run before a user can access a share. Once we sort this out, the attached is the test case for fixing "write list".
Created attachment 8364 [details] git-am patchset for master Not asking for review yet (this compiles, but I'm still fighting to get "make test" working in the master tree). This is how I'm planning to fix for master. It does change connection struct so makes a VFS ABI change. I have a (hack) way to fix this for 4.0.1 that doesn't change the VFS ABI (but is ugly - another global cache) but I think we need to do that to prevent VFS changes. Posting this here so people can see what I'm thinking, more when I have make test back working. Jeremy.
I think we should totally remove the share access stuff from service.c, otherwise we will end up back in the exact same spot, just for privileges. That is, the privelges check needs to move to uid.c. make_connection_snum() does call change_to_user(), so it will be set up by the end of the function anyway. Also, only uid.c handles the "write list" with the is_share_read_only_for_token() call (which is the original bug, that I need this to also fix).
Yep - good point - I'll add that to the final master patch once I've gotten "make test" back working so I can ensure this is a good fix. Jeremy.
A simple test case to reproduce the main symptom would be create a share like this: [share] path = /data/share write list = "jmaggard" valid users = "jmaggard" And try to write a file: $ smbclient -Ujmaggard '\\server\share' -c 'put foobar'
Ok, this (Justin's case) is not reproducing here with current unpatched v4-0-test. My share definition is: [foo] path = /tmp read only = yes write list = jra valid users = jra Justin, can I visit on-site to see the problem, or can you upload a debug level 10 log please ? Jeremy.
Created attachment 8380 [details] access denied - debug level 10 log Here's the debug level 10 log from the Samba 4.0.0 release. We're back from break now, so you're welcome to stop by the office any time.
Looking at that log it's rejecting access on : const int SEC_FILE_WRITE_DATA = 0x00000002; const int SEC_FILE_APPEND_DATA = 0x00000004; const int SEC_FILE_WRITE_EA = 0x00000010; const int SEC_FILE_WRITE_ATTRIBUTE = 0x00000100; which looks like it thinks it's a read-only share for this user. Aha, we have: Processing section "[foo]" doing parameter path = /tmp doing parameter read only = yes doing parameter write list = jmaggard doing parameter valid users = jmaggard So it's set as read-only, but should be read-write for jmaggard, and indeed we have: [2013/01/03 10:15:13.486397, 10, pid=3498, effective(0, 0), real(0, 0)] ../source3/smbd/share_access.c:289(is_share_read_only_for_token) is_share_read_only_for_user: share foo is read-write for unix user jmaggard and we are connecting as jmaggard: [2013/01/03 10:15:13.488064, 1, pid=3498, effective(0, 0), real(0, 0)] ../source3/smbd/service.c:891(make_connection_snum) jmaggard-thinkpad-w520 (ipv4:10.20.1.175:48002) connect to service foo initially as user jmaggard (uid=1001, gid=1002) (pid 3498) So conn->read_only should be set as false in check_user_ok. Ah - but that doesn't change conn->share_access. I think I see the problem and how to reproduce now ! Jeremy.
OK - yep, reproduced it ! Will create patchset asap. Jeremy.
Created attachment 8381 [details] Better git-am patchset for master. Difference from the previous version - adds a new function check_user_share_access() called by both make_connection_snum() and check_user_ok() to ensure we use the same share access evaluation on both tconX and become_uid. It's not possible to remove the share access evaluation on the tconX call as we still need to reject the tconX call if the user doing the tconX should have no access to the share. Jeremy.
(Continued from the previous comment.. - didn't finish it correctly). It's not possible to remove the share access evaluation on the tconX call as we still need to reject the tconX call if the user doing the tconX should have no access to the share - *BEFORE* we call any of the VFS functions or the root_preexec() code (this may have side effects like actually calling a script that creates a directory on the file system, so we can't rely on the setup done later in change_to_user() - this is too late - we must already have rejected by then). Jeremy.
Jeremy, Thanks for this! I'm happy with those changes in principal, how do you want to handle them from here? Does the test I have in https://bugzilla.samba.org/attachment.cgi?id=8362 now pass with this? Would you like me to autobuild them both? Thanks,
(In reply to comment #10) > (Continued from the previous comment.. - didn't finish it correctly). > > It's not possible to remove the share access evaluation on the > tconX call as we still need to reject the tconX call if the user doing the > tconX should have no access to the share - *BEFORE* we call any of the VFS > functions or the root_preexec() code (this may have side effects like actually > calling a script that creates a directory on the file system, so we can't rely > on the setup done later in change_to_user() - this is too late - we must > already have rejected by then). > > Jeremy. I totally agree, and was coming to the exact same conclusion. The root preexec would be a particular problem, given the kind of things that hook is often used for.
(In reply to comment #11) > Jeremy, > > Thanks for this! I'm happy with those changes in principal, how do you want to > handle them from here? > > Does the test I have in https://bugzilla.samba.org/attachment.cgi?id=8362 now > pass with this? > > Would you like me to autobuild them both? Sure if you like. I haven't tested against that specific change yet, although I think it'd be a really good change to add to the patchset also. Tomorrow I'll add your test in, ensure it passes and then propose it as a fix for master on samba-technical (I'll CC: you) for you to review. I am part-way through creating a back-port for 4.0.1 - but the problem here is I can't make the VFS change I used in the master patchset, so I'll have to do something uglier :-). I'll post the 4.0.x version here for your review tomorrow once I've finished it. Jeremy.
Created attachment 8383 [details] git-am patchset for master. Ok, here's the complete patchset I'm going to post to samba-technical for master. Took a while to get this right, but I think this is it. Jeremy.
Created attachment 8384 [details] git-am fix for 4.0.1 Here is the version for 4.0.1. It is the same as the master one, but doesn't change the VFS ABI (it uses a horrible static cache instead but there's no way around that). Justin, if you could test this on-site I'd be grateful ! Cheers, Jeremy.
Comment on attachment 8384 [details] git-am fix for 4.0.1 Getting additional eyes on this :-).
(In reply to comment #15) > Created attachment 8384 [details] > git-am fix for 4.0.1 > > Here is the version for 4.0.1. It is the same as the master one, but doesn't > change the VFS ABI (it uses a horrible static cache instead but there's no way > around that). > > Justin, if you could test this on-site I'd be grateful ! > I just tried this patch, and my simple test case is working now. So it looks good from my side. Thanks!
Created attachment 8394 [details] git-am patchset for master. Add's Volker's cleanup fixes, plus should cope with the special become_user_by_session() case without having to restore all the UID_FIELD_INVALID code.
Created attachment 8395 [details] git-am fix for master. Here is the complete set, starting with your fix, then my changes to change all callers of "conn = talloc_zero(ctx, connection_struct)" to call create_conn_struct() instead, then the rest of the fix.
Created attachment 8401 [details] git-am format patch for 4.0.x. Simpler version of the patch that landed in master. Jeremy.
Comment on attachment 8401 [details] git-am format patch for 4.0.x. Is this still the patch we want to have in 4.0? As we had to do some more changes in the master patch...
(In reply to comment #21) > Comment on attachment 8401 [details] > git-am format patch for 4.0.x. > > Is this still the patch we want to have in 4.0? As we had to do some more > changes in the master patch... The memory fixes are not required (the vuid cache is not a pointer in 4.0), and the other fixes are in 4969a7826d95b9068d5f7bea7069d22094cefa1f so I think we are OK. Andrew Bartlett
(In reply to comment #21) > Comment on attachment 8401 [details] > git-am format patch for 4.0.x. > > Is this still the patch we want to have in 4.0? As we had to do some more > changes in the master patch... Yes. The extra changes in the master branch were due to changing the vuid cache to be a pointer, which broke a bunch of auxiliary code that used hand-generated connection structs. This isn't being done in this patch to keep the VFS ABI consistent over the 4.0.x release stream, so those other changes aren't needed. Jeremy.
Comment on attachment 8401 [details] git-am format patch for 4.0.x. I don't see how, in the 4.0.0 patch, the combination of VUID and conn is referenced. It seems to be referenced by share_array_index, which is not globally unique.
Let me look over it again. I may have messed something up. Thanks for the review ! Jeremy.
Ah, I see the problem. It isn't share_array_index that is the problem (that is correctly initialized from conn->vuid_cache.next_entry), it's vuid_cache_share_access_array - which currently is global. It really needs to be allocated one for each new connection struct. I'll fix that up and re-submit a new 4.0.x patch. Thanks for the catch ! Good call. Jeremy.
Created attachment 8412 [details] git-am format patch for 4.0.x. Andrew - here is an updated patchset for your review. My mistake was to create only one static array of share_array entries for a single conn (Doh!). Obviously that won't work correctly (and thanks for catching it). This version creates a cache of array of share_array entries for every new conn struct, storing them in an alternate linked list inside source3/smbd/conn.c in the same way that conn structs are created, and tallocing them off conn so they keep the same talloc lifecycle of real conn structs. The rest of the code is basically the same, the new code is here: ea0942b969d3f57af8aac9f5b5380258edddb0a2 Add parallel cache for share_access entries, one per connection struct. Needed as we cannot change the VFS ABI for 4.0.x, but need to add the equivalent of 'uint32_t share_access' to the struct vuid_cache referenced in connection_struct. Exports 2 accessor functions - lifetime managed by talloc on the conn struct list. I checked this under valgrind and it's clean. Justin, if you could reconfirm using this patchset that it fixes your use case that would help greatly ! Cheers, Jeremy.
Re-assigning to Karolin for inclusion in 4.0.next. Jeremy.
Pushed to autobuild-v4-0-test.
Pushed to v4-0-test. Closing out bug report. Thanks!
Oh, and BTW, it's working for me. :-)
Yeah, you kinda got lost in the noise :-). But I was pretty confident with that last patch :-). Jeremy.