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

Jeremy Allison jra at samba.org
Thu Feb 28 20:05:25 UTC 2019


On Thu, Feb 28, 2019 at 11:48:26AM -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 ?

One more data point. If I make the change:

diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
index 5d49b718f0c..9a8109bb413 100644
--- a/libcli/security/access_check.c
+++ b/libcli/security/access_check.c
@@ -133,8 +133,7 @@ static uint32_t access_check_max_allowed(const struct security_descriptor *sd,
                                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);
+                               owner_rights_denied |= ace->access_mask;
                                owner_rights_default = false;
                        }
                        continue;

we still pass:

make test TESTS=samba3.*acls

so if it is needed, we're not testing it :-).



More information about the samba-technical mailing list