[PATCH] Patches required for POSIX ACL support of GPOs

[Andrew Bartlett]

On Tue, 2012-05-15 at 13:04 -0700, Jeremy Allison wrote:
> On Thu, May 10, 2012 at 11:38:48AM +1000, Andrew Bartlett wrote:
> > These patches are in my master-devel branch, and are needed for GPO
> > support to create the correct POSIX ACL.  I would very much appreciate
> > review, so we can consider enabling s3fs by default, and making the 4.0
> > Beta release.
> > 
> > This patch is needed for the same idea, in the NFSv4 ACL code.  It removes the sidmap as discussed, but I can't test it.
> > 
> > commit 5193b7f00181831c3d631e8b6f88cd3b783fd577
> > Author: Andrew Bartlett <abartlet at samba.org>
> > Date:   Mon May 7 08:48:24 2012 +1000
> > 
> >     s3-nfs4acls: Remove lookup_sid and sidmap from NFSv4 ACL mapping and check gid first
> >     
> >     By checking just the IDMAP, and by removing the sidmap and lookup_sid calls, we support
> >     IDMAP_BOTH.  This is because by checking for a mapping to a GID first, we can rely on
> >     the fact that IDMAP_BOTH will resolve to a GID.
> >     
> >     If the sidmap idea is valued - it allows multiple SIDs to map to a single unix ID, this should
> >     be done in the IDMAP layer.
> >     
> >     Andrew Bartlett
> 
> Hmmmmm. I'm just not sure of the above patch. I'll look more carefully
> at this.

Thanks.  As I said, it belongs at the idmap layer if it must remain, but
as discussed with Christian, it is undocumented. 

> > This came up when looking over the debug logs while fixing another bug,
> > but I think is worthwhile.  It isn't strictly required, but avoids going
> > via NSS to build the fake token. 
> > 
> > commit abf6ca1c560e1bec5656d830c61227cfb8af6133
> > Author: Andrew Bartlett <abartlet at samba.org>
> > Date:   Thu May 10 09:19:46 2012 +1000
> > 
> >     s3-smbd: Create a shortcut for building the token of a user by SID for posix_acls
> >     
> >     When a user owns a file, but does not have specific permissions on that file, we need to
> >     make up the user permissions.  This change ensures that the first thing that we do
> >     is to look up the SID, and confirm it is a user.  Then, we avoid the getpwnam()
> >     and directly create the token via the SID.
> >     
> >     Andrew Bartlett
> 
> Unfortunately the above patch is wrong, and has a really serious
> side effect.
> 
> You add a new call:
> 
> +static NTSTATUS create_token_from_sid(TALLOC_CTX *mem_ctx, 
> +                                     const struct dom_sid *user_sid,
> +                                     bool is_guest,
> +                                     uid_t *uid, gid_t *gid,
> +                                     char **found_username,
> +                                     struct security_token **token)
> 
> which you rewrite create_token_from_username() to call (this
> is a nice idea BTW, and good in theory).
> 
> Note that the user_sid parameter is "const struct dom_sid *"
> (emphasis on the const).
> 
> Inside that new function you do :
> 
> -               uid_to_unix_users_sid(*uid, &user_sid);
> +               uid_to_unix_users_sid(*uid, &tmp_sid);
> +               user_sid = &tmp_sid;
> 
> BOOP ! BOOP ! WARNING ! GO NO FURTHER !!!!
> 
> You just overwrote the passed in user_sid, which is explicitly
> supposted to be const (yeah, I know it's a const pointer, but
> still....).

How did I do that?  I overwrote the local copy of the pointer to the
passed in SID. 

> When you're invoking a new "tmp_sid" variable to get around a
> const violation, this is a sign you're doing things wrong.

I'm very glad it's const, as it nicely proves that I've not done what
you think I've done. 

> Now this doesn't matter when calling from the new
> create_token_from_username() call, as you throw away
> the SID anyway (and this is a hint that user_sid in
> the new create_token_from_sid() shouldn't be defined
> as const).
> 
> However it's *fatal* when you call it from your new :
> 
> +bool user_sid_in_group_sid(const struct dom_sid *sid, const struct dom_sid *group_sid)
> 
> which you call directly from the change in source3/smbd/posix_acls.c
> to uid_entry_in_group().
> 
> +       return user_sid_in_group_sid(&uid_ace->trustee, &group_ace->trustee);
> 
> This *COULD* modify &uid_ace->trustee now - which I really
> don't think is what we want.

How, under standard rules of C, could this happen?

> It's a shame, as this looks like a nice optimization, but is
> it really *needed* to make the code work ?

I don't think so, but I'm also quite confident that your analysis is
incorrect. 

> This is where I'm up to so far, just wanted to let you know.

Thanks.

> The patchset is good in theory, but you should try not to
> mix up optimizations that look like a good idea with real
> code changes to make the thing just basically work, and this
> patchset is a mix of the two.

I wouldn't mix them into the same patch, but I did clearly describe the
intent, purpose and background of each patch. 

> I don't think this one is needed to make it just work, so
> I'm going to omit this in my updated patchset.

That's up to you.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org