[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