Bug 9878 - force user does not work as expected
force user does not work as expected
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: File services
4.0.5
All Linux
: P5 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-11 22:46 UTC by Alexander Popolitov
Modified: 2014-04-27 19:55 UTC (History)
7 users (show)

See Also:


Attachments
git-am fix for 4.1.next (2.25 KB, patch)
2014-03-17 22:19 UTC, Jeremy Allison
abartlet: review-
Details
git-am fix for 4.0.next (2.27 KB, patch)
2014-03-17 22:19 UTC, Jeremy Allison
no flags Details
git-am fix for 4.1.next (2.25 KB, patch)
2014-03-17 23:09 UTC, Jeremy Allison
obnox: review+
asn: review+
Details
git-am fix for 4.0.next (2.28 KB, patch)
2014-03-17 23:11 UTC, Jeremy Allison
obnox: review+
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Popolitov 2013-05-11 22:46:41 UTC
Hi!
I have this config:

[myshare]
    path = /mnt/myshare
    create mode = 0644
    directory mode = 0775
    force create mode = 0644
    force directory mode  = 0775
    force user = http
    valid users = user1 user2
    write list = user1

which is not working. Samba server is launching, but on the client side
I get "Permission denied error", when trying to authenticate either as user1 or as user2.

If I comment out 'force user' line, I authenticate just fine, but then, obviously,
files and directories created are owned by user1 or user2, but not http.

If I add http user to 'valid users' and to 'write list', and do not comment
'force user = http' line, I also am able to log in as user1 and user2, however,
not only user1 can create and change files, but also a user2, which now happens
to be treated as http user.

In previous versions of samba (in 3.<something> at least) behaviour was
a) forced user (http) was not required to be present in 'valid users' list
b) even when overrided to forced UNIX user, permissions to write to a share we
still somehow controlled by the permissions of original Samba user (permission was bitwise-and of those permissions)

N.B. http user does not exist in by tdbsam database of Sambe users - it is purely UNIX user, while user1 and user2 both and UNIX and Samba users.

I wonder, if this change in behaviour was intentional and if so,
how can I get the previous behaviour?
Comment 1 Alexander Popolitov 2013-05-11 23:04:24 UTC
Figured out, how to do what I want - do not need to add http user to write list.
If I do this way, user1 gets read/write permissions and files are created as http user
and user2 gets read-only permissions.

Still, necessity to add http user to valid users is counterintuitive - was it intentional?
Comment 2 Michael Adam 2013-12-07 23:04:19 UTC
Confirmed.
And also broken for 4.1.
This is definitely a regression and needs to be fixed.
In a member setup, I even needed to specify


"Unix User\user" in the valid users setting
for unix user "user" in "force user = user".
Comment 3 Jeremy Allison 2013-12-11 20:28:10 UTC
So the code path looks like this. Inside source3/smbd/service.c we have:


        conn->read_only = lp_readonly(SNUM(conn));

        status = set_conn_force_user_group(conn, snum);
        if (!NT_STATUS_IS_OK(status)) {
                goto err_root_exit;
        }

        conn->vuid = vuser->vuid;

...

        /*
         * Set up the share security descriptor
         */

        status = check_user_share_access(conn,
                                        conn->session_info,
                                        &conn->share_access,
                                        &conn->read_only);

The "set_conn_force_user_group()" sets the token to be that of the forced user. The check_user_share_access() then compares the token against the 'valid users' (or 'invalid users') list. If http (the forced user) isn't in the valid users list, then status is returned as ACCESS_DENIED and we exit.

This *does* seem like a reasonable check to make, even if it was different in 3.6.x (so is strictly a regression).

We might want to address this by updating the 'force user' documentation to explain the user must be valid for this share connection.

I'll look into how 3.6.x did it before making a final evaluation.

Jeremy.
Comment 4 Jeremy Allison 2013-12-11 22:41:34 UTC
Hmmm. The interaction between "force user", "valid users", "read list" and "write list" is horribly undefined..

If force user is set does this mean :

a). No access unless the forced user is in the valid users list ?
(currently true in master).

b). Read-only access unless the forced user is in the write list ?
(currently false in master).

c). Read-write access unless the forced user is in the read list ?
(not tested by me yet).

We need to define once and for all what the interactions between these parameters *should* be, then add tests to ensure it doesn't break.

Jeremy.
Comment 5 Michael Adam 2013-12-12 07:59:11 UTC
Hi Jeremy,

yeah this is definitely a regression.

My understanding of forced user vs. valid users (and friends) is this:

- "valid users" (and read list and write list) are checks that are done
  at the smb level, i.e. against the windows token of the connecting
  user. This is so to speak a poor man's share ACL.

- "force user" and "force group" are a means to change the unix
  user / group accessing the file system on behalf of the connected
  Windows user. I.e. this at the posix level, between smbd and
  the file system. This is impersonation is done after the SMB level
  access checks and is completely unrelated.

It has been a very long practice to use "force user" with a user
that is only a unix user unknown to the smb level of samba (pdb).

(In reply to comment #4)
> Hmmm. The interaction between "force user", "valid users", "read list" and
> "write list" is horribly undefined..
> 
> If force user is set does this mean :
> 
> a). No access unless the forced user is in the valid users list ?
> (currently true in master).
> 
> b). Read-only access unless the forced user is in the write list ?
> (currently false in master).
> 
> c). Read-write access unless the forced user is in the read list ?
> (not tested by me yet).
> 
> We need to define once and for all what the interactions between these
> parameters *should* be, then add tests to ensure it doesn't break.

So according to my (and many setups I have seen and done myself),
none of the above should be true.

access/read/write checking based on the "valid users"/"write list"
is done at the smb level only, and the completely unrelated
forced user will perform any file system operation that the SMB layer
of Samba commands it to do, based on these SMB checks (given that the
forced user is allowed to perform them).

So the wrong thing is master is that a windows token mapped from
the forced unix user is checked against the lists at all.

Here is what the smb.conf manpage says regarding the topic:

>>> This user name only gets used once a connection is established.
>>> Thus clients still need to connect as a valid user and supply
>>> a valid password. Once connected, all file operations will be
>>> performed as the "forced user", no matter what username the
>>> client connected as.

Cheers - Michael
Comment 6 Jeremy Allison 2013-12-12 17:12:26 UTC
(In reply to comment #5)
> Hi Jeremy,
> 
> yeah this is definitely a regression.
> 
> My understanding of forced user vs. valid users (and friends) is this:
> 
> - "valid users" (and read list and write list) are checks that are done
>   at the smb level, i.e. against the windows token of the connecting
>   user. This is so to speak a poor man's share ACL.
> 
> - "force user" and "force group" are a means to change the unix
>   user / group accessing the file system on behalf of the connected
>   Windows user. I.e. this at the posix level, between smbd and
>   the file system. This is impersonation is done after the SMB level
>   access checks and is completely unrelated.
> 
> It has been a very long practice to use "force user" with a user
> that is only a unix user unknown to the smb level of samba (pdb).
>
> So according to my (and many setups I have seen and done myself),
> none of the above should be true.
> 
> access/read/write checking based on the "valid users"/"write list"
> is done at the smb level only, and the completely unrelated
> forced user will perform any file system operation that the SMB layer
> of Samba commands it to do, based on these SMB checks (given that the
> forced user is allowed to perform them).
> 
> So the wrong thing is master is that a windows token mapped from
> the forced unix user is checked against the lists at all.

"So it's once more into the breach then, dear friends" :-).

This keeps getting broken because we don't have a test
that stops us regressing here, and also because this
expected behavior has never been written down and has
never been codified as "expected". Trust me, I'd know
if it had - and I can never remember the interactions
between these parameters.

I can work on the code to change it back to have the
"correct" semantics, but I really need some help getting
the tests written to make sure we don't regress on this.
Comment 7 Michael Adam 2013-12-16 12:47:45 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > So the wrong thing is master is that a windows token mapped from
> > the forced unix user is checked against the lists at all.
> 
> "So it's once more into the breach then, dear friends" :-).
> 
> This keeps getting broken because we don't have a test
> that stops us regressing here, and also because this
> expected behavior has never been written down and has
> never been codified as "expected".

I always understood the above quote from the manpage
to describe exactly this, but at second reading, I consent
that it can be made much more clear.

> I can work on the code to change it back to have the
> "correct" semantics, but I really need some help getting
> the tests written to make sure we don't regress on this.

Ok, great! I'll also happily work/collaborate on this.

Cheers - Michael
Comment 8 Michael Adam 2013-12-17 21:33:07 UTC
Btw, if I got this right,
the regression was introduced with

86d1e1db8e2747e30c89627cda123fde1e84f579:

https://git.samba.org/?p=samba.git;a=commitdiff;h=86d1e1db8e2747e30c89627cda123fde1e84f579
Comment 9 Jeremy Allison 2013-12-17 21:52:38 UTC
Oh, good catch. That fix was definitely required, as it fixed an issue with calculating the share permissions correctly for the user.

Looks like the call to check_user_share_access() is doing more than is required, as the call inside check_user_share_access() to:

        if (!user_ok_token(session_info->unix_info->unix_name,
                           session_info->info->domain_name,
                           session_info->security_token, snum)) {
                return NT_STATUS_ACCESS_DENIED;
        }

is causing the access denied if the forced user isn't in the write list. The previous code just calculated the share access mask directly.
Comment 10 Jeremy Allison 2013-12-17 21:55:03 UTC
Hmmm. Although thinking about it, the conn->session_info->security_token at this point is already the forced user token.

Should we be calculating the share access mask based off the forced user, or the original valid user ?

Again, this is a little trickier than it looks at first glance :-). Michael, what are your thoughts on who we should use to calculate the share access mask and readonly parameter on the connection ?
Comment 11 Michael Adam 2013-12-17 23:45:20 UTC
(In reply to comment #10)
> Hmmm. Although thinking about it, the conn->session_info->security_token at
> this point is already the forced user token.
> 
> Should we be calculating the share access mask based off the forced user, or
> the original valid user ?
> 
> Again, this is a little trickier than it looks at first glance :-). Michael,
> what are your thoughts on who we should use to calculate the share access mask
> and readonly parameter on the connection ?

Hmmm, can't we keep the original connecting user's token
for access checks instead of overwriting it with the
forced user's token which can be a unix token mapped
up with the "Unix User\user" style fake sid-level token?

The share access check should be done with the original
windows-level token and not the forced token, so we somehow
need to keep both, right? So do we need to keep two tokens?
Or do we just overwrite the one token later?

...
Comment 12 Jeremy Allison 2013-12-17 23:57:24 UTC
And here's one more thing.

Right now the logic flow in make_connection_snum() calculates the session_info of the 'forced user' and then uses it in the :

               char *s = talloc_sub_advanced(talloc_tos(),
                                        lp_servicename(talloc_tos(), SNUM(conn)),
                                        conn->session_info->unix_info->unix_name,
                                        conn->connectpath,
                                        conn->session_info->unix_token->gid,
                                        conn->session_info->unix_info->sanitized_username,
                                        conn->session_info->info->domain_name,
                                        lp_pathname(talloc_tos(), snum));


call which can change the details of the connect path etc. (due to the %u and other substitutions). Should these substitutions be done as the original, or the forced user ?

For example, think of a share with 

path = /home/%u

Should this be: /home/'forced user' or /home/'logged in user' ?

We need to decide upon these details. If I make the simple change to move the forced user calculation after the share access mask changes, this will have the side effect of changing the substitutions done.
Comment 13 Thomas Bork 2014-01-02 20:35:33 UTC
With Samba 4.1.2 this is /home/'forced user'. With 3.6.22 this is /home/'forced user'. And I think, this is the right thing:

Once authenticated, all operations should be done with the forced user - and thats why also the substitutions.
Comment 14 David C. Rankin 2014-01-12 07:43:38 UTC
  I ran into this same issue on 4.1.3 related to sharing the root filesystem where force user and force group are set to root. (standalone server) See the thread "What in samba 4.1 prevents a '/' share?" In summary, using the share definition of:

[config]
   comment = Phoinix Config (Archlinux)
   path = /
   valid users = david
   force user = root
   force group = root
   browseable = no
   writeable = Yes

  Caused mount failure returning the following error:

[14:45 providence:~/cnf] # mount.cifs //nemesis/config /mnt/nm-cfg/ -o uid=1000,credentials=/root/cnf/mountcfile,noperm
mount error(13): Permission denied
Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)

  After searching through a level 10 debug with 'log file = /var/log/samba/level10-%S.log' I ran across the following user_ok_token issues:

[2014/01/07 20:32:58.157111,  5, pid=5405, effective(0, 0), real(0, 0)]
../source3/lib/username.c:181(Get_Pwnam_alloc)
  Finding user david

<snip>

[2014/01/07 20:32:58.158932, 10, pid=5405, effective(0, 0), real(0, 0)]
../source3/smbd/share_access.c:237(user_ok_token)
  user_ok_token: share config is ok for unix user david
[2014/01/07 20:32:58.159036,  5, pid=5405, effective(0, 0), real(0, 0)]
../source3/lib/username.c:181(Get_Pwnam_alloc)
  Finding user root

<big snip>

[2014/01/07 20:32:58.176304, 10, pid=5405, effective(0, 0), real(0, 0)]
../source3/smbd/share_access.c:215(user_ok_token)
  User root not in 'valid users'

<snip>

[2014/01/07 20:32:58.176620,  3, pid=5405, effective(0, 0), real(0, 0)]
../source3/smbd/error.c:82(error_packet_set)
  NT error packet at ../source3/smbd/reply.c(952) cmd=117 (SMBtconX)
NT_STATUS_ACCESS_DENIED

  Removing the 'force user' and 'force group' options from the [config] share allowed the share to mount without error. I have used the 'force user' and 'force group' options in the [config] share since at least 2.0.7 (maybe 1.8.x), and 4.x is the first time I ran into a mount failure. If it will help, I have the level 10 for the failure.
Comment 15 Jeremy Allison 2014-03-17 21:05:17 UTC
Ok, I might have a quick and easy fix for this...

Testing now.

Jeremy.
Comment 16 Jeremy Allison 2014-03-17 22:19:39 UTC
Created attachment 9781 [details]
git-am fix for 4.1.next
Comment 17 Jeremy Allison 2014-03-17 22:19:58 UTC
Created attachment 9782 [details]
git-am fix for 4.0.next
Comment 18 Andrew Bartlett 2014-03-17 22:39:12 UTC
Comment on attachment 9781 [details]
git-am fix for 4.1.next

Either I am very confused, or one of these statements is not true:

conn->session_info represents the token to use
when actually accessing the file system, and so
is modified by force user and force group.

conn->session_info represents the "pristine"
token of the user logging in, and is never modified
by force user and force group.

I think you mean vuser in the second instance.

Either way, I think we should move check_user_share_access() up to just below   create_connection_session_info() to more closely match check_user_ok() in uid.c (or better still, find a way to merge more aspects of both into a single fucntion).

Finally, this needs a test-suite.  We chickened out of this last time we worked here, and this is our reward, so we must do it this time, including not just valid users but also setting a per-share security descriptor.
Comment 19 Jeremy Allison 2014-03-17 23:05:25 UTC
(In reply to comment #18)
> Comment on attachment 9781 [details]
> git-am fix for 4.1.next
> 
> Either I am very confused, or one of these statements is not true:
> 
> conn->session_info represents the token to use
> when actually accessing the file system, and so
> is modified by force user and force group.
> 
> conn->session_info represents the "pristine"
> token of the user logging in, and is never modified
> by force user and force group.
> 
> I think you mean vuser in the second instance.

Indeed I do, thanks for noticing that. I'll upload a changed version with the correct comment :-).

> Either way, I think we should move check_user_share_access() up to just below  
> create_connection_session_info() to more closely match check_user_ok() in uid.c
> (or better still, find a way to merge more aspects of both into a single
> fucntion).

Nope. Don't want to make arbitrary changes right now. It's best to
do the "minimum necessary fix" for the bug. Let's not experiment
with making the code elegant until it's fixed.

> Finally, this needs a test-suite.  We chickened out of this last time we worked
> here, and this is our reward, so we must do it this time, including not just
> valid users but also setting a per-share security descriptor.

Now this I agree with wholeheartedly :-). Luckily for me Michael has volunteered above to help with this :-).

Jeremy.
Comment 20 Jeremy Allison 2014-03-17 23:09:22 UTC
Created attachment 9783 [details]
git-am fix for 4.1.next

Fixed commit message as noticed by Andrew.
Comment 21 Jeremy Allison 2014-03-17 23:11:21 UTC
Created attachment 9784 [details]
git-am fix for 4.0.next

Fixed commit message as noticed by Andrew.
Comment 22 Jeremy Allison 2014-03-17 23:14:08 UTC
(In reply to comment #19)

> Now this I agree with wholeheartedly :-). Luckily for me Michael has
> volunteered above to help with this :-).

But I won't object if you want to add tests too :-).

Cheers,

Jeremy.
Comment 23 Andreas Schneider 2014-03-18 11:09:53 UTC
We don't have any 'force user' tests at all. We also need a test for 'force user' with 'security = ads'.

So I would say review+, but it would be great to have tests in master so we avoid regressions in future.
Comment 24 Jeremy Allison 2014-03-18 15:54:11 UTC
Comment on attachment 9783 [details]
git-am fix for 4.1.next

Adding Andreas so he can formally +1
Comment 25 Jeremy Allison 2014-03-18 15:54:29 UTC
Comment on attachment 9784 [details]
git-am fix for 4.0.next

Adding Andreas so he can formally +1
Comment 26 Jeremy Allison 2014-03-18 15:55:05 UTC
Here is the feedback from the list:

Date: Tue, 18 Mar 2014 07:32:35 +0100 (CET)
From: Gerhard Wiesinger <lists@wiesinger.com>
To: Jeremy Allison <jra@samba.org>
cc: samba-technical@samba.org, obnox@samba.org
Subject: Re: [PATCH] Fix bug #9878 - force user does not work as expected.
User-Agent: Alpine 2.02 (LFD 1266 2009-07-14)


Reviewed-by: Gerhard Wiesinger <lists@wiesinger.com>
Tested-by: Gerhard Wiesinger <lists@wiesinger.com>
Ack-by: Gerhard Wiesinger <lists@wiesinger.com>

Thank you for the fast fix.

Ciao,
Gerhard
Comment 27 Andreas Schneider 2014-03-19 09:03:57 UTC
Karolin, please add the patches to the relevant branches. Thanks!
Comment 28 Andreas Schneider 2014-03-19 15:13:19 UTC
The fix isn't working correctly. See samba-technical.
Comment 29 Jeremy Allison 2014-03-19 17:08:40 UTC
Issue turns out not to be with these fixes, but to do with user login not getting the correct group list (missing an added group from /etc/group).

Andreas will re-review (and hopefully +1 :-).
Comment 30 Andreas Schneider 2014-03-19 22:43:06 UTC
Karolin, please add the patches to the relevant branches. Thanks!
Comment 31 Karolin Seeger 2014-03-25 09:57:14 UTC
(In reply to comment #30)
> Karolin, please add the patches to the relevant branches. Thanks!

Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 32 Karolin Seeger 2014-04-04 18:47:27 UTC
Pushed to v4-0-test and v4-1-test.
Closing out bug report.

Thanks!