Bug 9518 - conn->share_access appears not be be reset between users
conn->share_access appears not be be reset between users
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: File services
4.0.0
All All
: P5 major
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-20 21:40 UTC by Andrew Bartlett
Modified: 2013-01-16 17:04 UTC (History)
3 users (show)

See Also:


Attachments
patch to add test case for "write list" (3.75 KB, patch)
2012-12-20 21:40 UTC, Andrew Bartlett
no flags Details
git-am patchset for master (21.60 KB, patch)
2012-12-21 20:51 UTC, Jeremy Allison
no flags Details
access denied - debug level 10 log (35.71 KB, application/x-xz)
2013-01-03 18:24 UTC, Justin Maggard
no flags Details
Better git-am patchset for master. (29.34 KB, patch)
2013-01-04 01:18 UTC, Jeremy Allison
no flags Details
git-am patchset for master. (42.44 KB, patch)
2013-01-04 23:54 UTC, Jeremy Allison
no flags Details
git-am fix for 4.0.1 (37.77 KB, patch)
2013-01-04 23:55 UTC, Jeremy Allison
no flags Details
git-am patchset for master. (47.21 KB, patch)
2013-01-08 20:59 UTC, Jeremy Allison
no flags Details
git-am fix for master. (57.85 KB, application/octet-stream)
2013-01-09 01:25 UTC, Jeremy Allison
no flags Details
git-am format patch for 4.0.x. (39.64 KB, patch)
2013-01-09 21:28 UTC, Jeremy Allison
abartlet: review-
Details
git-am format patch for 4.0.x. (44.17 KB, patch)
2013-01-11 19:59 UTC, Jeremy Allison
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2012-12-20 21:40:06 UTC
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".
Comment 1 Jeremy Allison 2012-12-21 20:51:58 UTC
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.
Comment 2 Andrew Bartlett 2012-12-21 22:47:45 UTC
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).
Comment 3 Jeremy Allison 2012-12-21 22:57:04 UTC
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.
Comment 4 Justin Maggard 2012-12-22 00:10:13 UTC
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'
Comment 5 Jeremy Allison 2012-12-22 00:18:19 UTC
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.
Comment 6 Justin Maggard 2013-01-03 18:24:26 UTC
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.
Comment 7 Jeremy Allison 2013-01-03 21:59:59 UTC
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.
Comment 8 Jeremy Allison 2013-01-03 22:15:54 UTC
OK - yep, reproduced it !

Will create patchset asap.

Jeremy.
Comment 9 Jeremy Allison 2013-01-04 01:18:52 UTC
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.
Comment 10 Jeremy Allison 2013-01-04 01:26:58 UTC
(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.
Comment 11 Andrew Bartlett 2013-01-04 01:31:18 UTC
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,
Comment 12 Andrew Bartlett 2013-01-04 01:32:11 UTC
(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.
Comment 13 Jeremy Allison 2013-01-04 01:38:36 UTC
(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.
Comment 14 Jeremy Allison 2013-01-04 23:54:16 UTC
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.
Comment 15 Jeremy Allison 2013-01-04 23:55:39 UTC
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 16 Jeremy Allison 2013-01-04 23:56:33 UTC
Comment on attachment 8384 [details]
git-am fix for 4.0.1

Getting additional eyes on this :-).
Comment 17 Justin Maggard 2013-01-07 23:03:40 UTC
(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!
Comment 18 Jeremy Allison 2013-01-08 20:59:52 UTC
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.
Comment 19 Jeremy Allison 2013-01-09 01:25:32 UTC
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.
Comment 20 Jeremy Allison 2013-01-09 21:28:57 UTC
Created attachment 8401 [details]
git-am format patch for 4.0.x.

Simpler version of the patch that landed in master.

Jeremy.
Comment 21 Stefan Metzmacher 2013-01-10 08:51:44 UTC
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...
Comment 22 Andrew Bartlett 2013-01-10 09:42:41 UTC
(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
Comment 23 Jeremy Allison 2013-01-10 17:00:39 UTC
(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 24 Andrew Bartlett 2013-01-11 06:31:13 UTC
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.
Comment 25 Jeremy Allison 2013-01-11 16:53:33 UTC
Let me look over it again. I may have messed something up.

Thanks for the review !

Jeremy.
Comment 26 Jeremy Allison 2013-01-11 17:53:14 UTC
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.
Comment 27 Jeremy Allison 2013-01-11 19:59:05 UTC
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.
Comment 28 Jeremy Allison 2013-01-11 21:00:04 UTC
Re-assigning to Karolin for inclusion in 4.0.next.

Jeremy.
Comment 29 Karolin Seeger 2013-01-14 18:05:24 UTC
Pushed to autobuild-v4-0-test.
Comment 30 Karolin Seeger 2013-01-15 10:16:04 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!
Comment 31 Justin Maggard 2013-01-16 01:03:55 UTC
Oh, and BTW, it's working for me. :-)
Comment 32 Jeremy Allison 2013-01-16 17:04:16 UTC
Yeah, you kinda got lost in the noise :-). But I was pretty confident with that last patch :-).

Jeremy.