[PATCH] Bug in dealing with "Owner Rights" ACEs when calculating maximum access

[David Disseldorp]

On Thu, 28 Feb 2019 11:48:26 -0800, Jeremy Allison via samba-technical wrote:

> On Thu, Feb 28, 2019 at 06:49:51PM +0100, Ralph Böhme via samba-technical wrote:
> > Hi!
> > 
> > Just came across this one:
> > 
> > https://bugzilla.samba.org/show_bug.cgi?id=13812
> > 
> > When an SMB2 client queries maximum permission on a file or directory that
> > has an explicit "Owner Rights" ACE, hell breaks loose. The bugreport has all
> > the nasty details.
> > 
> > A customer ran across this with macOS clients, as in vfs_fruit I'm also
> > calling se_access_check() with SEC_FLAG_MAXIMUM_ALLOWED.
> > 
> > Please review & push if happy. Thanks!  
> 
> Mostly looks great, and already got pushed :-).
> 
> But I would like clarification on this bit please :
> 
>                 if (dom_sid_equal(&ace->trustee, &global_sid_Owner_Rights)) {
>                         if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED) {
>                                 owner_rights_allowed |= ace->access_mask;
>                                 owner_rights_default = false;
>                         } else if (ace->type == SEC_ACE_TYPE_ACCESS_DENIED) {
>                                 owner_rights_denied |= (owner_rights_allowed &
>                                                         ace->access_mask);
> 
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                                 SEE BELOW(*)
> 
>                                 owner_rights_default = false;
>                         }
>                         continue;
>                 }
> 
> That adds to owner_rights_denied only the access bits
> that have already been set in owner_rights_allowed, masked
> (ANDed) with the access mask in the deny ACE.
> 
> Can you explain where that bit of the algorithm came from ?
> 
> My naive reading of this says it should be:
> 
> owner_rights_denied |= ace->access_mask
> 
> Is there a MS-DTYP reference for this that I'm missing ?

Hmm, yeah I think you're right, considering owner_rights_allowed in the
TYPE_ACCESS_DENIED path looks wrong here. I'd guess it's a cut'n'paste
error from se_access_check()->bits_remaining. You can have a RB+ from
me for your follow up fix, but let's wait for Ralph's response :)

Cheers, David