Bug 9236 - ACL masks incorrectly applied when setting ACLs.
Summary: ACL masks incorrectly applied when setting ACLs.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.0.0rc4
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 8266
  Show dependency treegraph
 
Reported: 2012-10-02 19:06 UTC by Jeremy Allison
Modified: 2021-02-11 14:01 UTC (History)
1 user (show)

See Also:


Attachments
git-am fix for 3.6.next. (8.39 KB, patch)
2012-10-02 19:21 UTC, Jeremy Allison
asn: review+
Details
git-am fix for 3.5.next (7.96 KB, patch)
2012-10-02 20:28 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.x (1.86 KB, patch)
2012-11-09 00:45 UTC, Jeremy Allison
jra: review? (rsharpe)
asn: review+
Details
git-am fix for 3.5.x (1.86 KB, patch)
2012-11-09 00:50 UTC, Jeremy Allison
jra: review? (rsharpe)
asn: review+
Details
git-am fix for 4.0.0.rc.next (1.86 KB, patch)
2012-11-09 01:08 UTC, Jeremy Allison
jra: review? (rsharpe)
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2012-10-02 19:06:27 UTC
See bug #9190 Regression (change in behavior) of default acl masks as well.

We incorrectly apply ACL masks to default ACL entries on directories (we should not) and miss applying them to SMB_ACL_USER and SMB_ACL_GROUP entries.

Normally this isn't noticed as the default ACL masks are 0777.

Patches for 3.5.x and 3.6.x to follow.

Jeremy.
Comment 1 Jeremy Allison 2012-10-02 19:21:48 UTC
Created attachment 7980 [details]
git-am fix for 3.6.next.
Comment 2 Jeremy Allison 2012-10-02 20:28:29 UTC
Created attachment 7981 [details]
git-am fix for 3.5.next
Comment 3 Andreas Schneider 2012-10-04 13:34:20 UTC
Comment on attachment 7980 [details]
git-am fix for 3.6.next.

Looks fine for me. Thanks!
Comment 4 Andreas Schneider 2012-10-04 13:35:00 UTC
Comment on attachment 7981 [details]
git-am fix for 3.5.next

Looks good to me. Simo looked at them too. Thanks.
Comment 5 Jeremy Allison 2012-10-04 17:02:21 UTC
Re-assigning to Karolin for inclusion in 3.6.next and 3.5.next.

Thanks !

Jeremy.
Comment 6 Karolin Seeger 2012-10-05 07:51:03 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!
Comment 7 Jeremy Allison 2012-10-05 22:35:48 UTC
There is one more fix needed here I missed. Additional patch to follow.
Jeremy.
Comment 8 Jeremy Allison 2012-10-05 22:42:51 UTC
Arg. No, we don't need another patch (sorry). We're technically wrong, but we get away with it :-). Re-closing the bug. Sorry for the noise.
Jeremy.
Comment 9 Jeremy Allison 2012-11-08 21:39:34 UTC
There is one more case here I need to address in 3.5.x, 3.6.x and 4.0.x.

Patch to follow.

Jeremy.
Comment 10 Jeremy Allison 2012-11-09 00:45:17 UTC
Created attachment 8169 [details]
git-am fix for 3.6.x

    Another fix needed for bug #9236 - ACL masks incorrectly applied when setting ACLs.
    
    Not caught by make test as it's an extreme edge case for strange
    incoming ACLs. I only found this as I'm making raw.acls and smb2.acls
    pass against 3.6.x with acl_xattr mapped onto a POSIX backend (which
    isn't tested in make test).
    
    An incoming inheritable ACE entry containing only one permission,
    WRITE_DATA maps into a POSIX owner perm of "-w-", which violates
    the principle that the owner of a file/directory can always read.

Patches to follow for 3.5.x and 4.0.x.

Jeremy
Comment 11 Jeremy Allison 2012-11-09 00:50:25 UTC
Created attachment 8170 [details]
git-am fix for 3.5.x

Same fix applies to 3.5.x.
Comment 12 Jeremy Allison 2012-11-09 01:08:35 UTC
Created attachment 8171 [details]
git-am fix for 4.0.0.rc.next

Version for 4.0.0.rc.next.
Comment 13 Jeremy Allison 2012-11-09 18:33:01 UTC
This is needed for 4.0.0rc.next also.

Jeremy.
Comment 14 Richard Sharpe 2012-11-11 20:06:42 UTC
(In reply to comment #10)
> Created attachment 8169 [details]
> git-am fix for 3.6.x
> 
>     Another fix needed for bug #9236 - ACL masks incorrectly applied when
> setting ACLs.
> 
>     Not caught by make test as it's an extreme edge case for strange
>     incoming ACLs. I only found this as I'm making raw.acls and smb2.acls
>     pass against 3.6.x with acl_xattr mapped onto a POSIX backend (which
>     isn't tested in make test).
> 
>     An incoming inheritable ACE entry containing only one permission,
>     WRITE_DATA maps into a POSIX owner perm of "-w-", which violates
>     the principle that the owner of a file/directory can always read.
> 
> Patches to follow for 3.5.x and 4.0.x.
> 
> Jeremy

Doesn't this create a problem that when the user reads the ACL back they see bits there that they did not add?
Comment 15 Jeremy Allison 2012-11-12 01:47:33 UTC
Only if they're not using acl_xattr (remember, with acl_xattr they see the exact Windows ACL they set, regardless of POSIX ACL permissions underneath).

If they're mapping directly onto POSIX ACLs then yes, they'll see different permissions to those they set, but that's normal for a POSIX ACL mapping.

This is a necessary precursor to the fixes I'm adding in bug #9374, which will allow 3.6.x and above to pass both the smb2.acl and the raw.acl tests with acl_xattr mapped onto a POSIX ACL backend.

The issue is this:

Default ACL is set to be Everyone:WRITE_DATA (no other permissions). Without this patch it creates a default POSIX ACL on the directory of owner:-w- which breaks the assumptions that the rest of the POSIX ACL mapping code makes that the owner can *always* open with O_RDONLY (which the previous code used to enforce - this is replacing that mapping).

Hope this explanation helps.

Jeremy.
Comment 16 Jeremy Allison 2012-11-13 17:35:29 UTC
Comment on attachment 8169 [details]
git-am fix for 3.6.x

Need to get this reviewed and in for 4.0.0 final...

The smbtorture ACL fixes also rely on this.
Comment 17 Andreas Schneider 2012-11-14 07:19:12 UTC
Comment on attachment 8169 [details]
git-am fix for 3.6.x

I'm fine with it, if Richard is too :)
Comment 18 Andreas Schneider 2012-11-14 07:19:55 UTC
Comment on attachment 8170 [details]
git-am fix for 3.5.x

I'm fine with it, if Richard is too :)
Comment 19 Andreas Schneider 2012-11-14 07:20:17 UTC
Comment on attachment 8171 [details]
git-am fix for 4.0.0.rc.next

I'm fine with it, if Richard is too :)
Comment 20 Jeremy Allison 2012-11-14 22:43:15 UTC
As Richard is pretty busy with his new job I'm going to declare victory and assign to Karolin for inclusion in 3.5.x, 3.6.x and 4.0.0.rc.next :-).

Jeremy.
Comment 21 Karolin Seeger 2012-11-15 08:07:20 UTC
(In reply to comment #20)
> As Richard is pretty busy with his new job I'm going to declare victory and
> assign to Karolin for inclusion in 3.5.x, 3.6.x and 4.0.0.rc.next :-).
> 
> Jeremy.

Pushed to v3-5-test, v3-6-test and autobuild-v4-0-test.
Comment 22 Karolin Seeger 2012-11-15 10:22:08 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!