Bug 13812 - access_check_max_allowed() doesn't process "Owner Rights" ACEs
Summary: access_check_max_allowed() doesn't process "Owner Rights" ACEs
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-28 08:57 UTC by Ralph Böhme
Modified: 2019-03-12 09:37 UTC (History)
1 user (show)

See Also:


Attachments
Patch for 4.8, 4.9 and 4.10 cherry-picked from master (39.66 KB, patch)
2019-03-07 08:52 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2019-02-28 08:57:41 UTC
This was probably missed in 44590c1b70c0a24f853c02d5fcdb3c609401e2ca.

As a result, given the following ACL, se_access_check() when called with SEC_FLAG_MAXIMUM_ALLOWED returns ACCESS_DENIED:

$ bin/smbcacls -U slow%x //localhost/test "dir/file"
REVISION:1
CONTROL:SR|DP
OWNER:SLOWSERVER\slow
GROUP:Unix Group\slow
ACL:Owner Rights:ALLOWED/0x0/READ
ACL:SLOWSERVER\slow:ALLOWED/0x0/READ

The problem is triggered by:

* se_access_check() calls access_check_max_allowed()

* access_check_max_allowed() unconditionally adds SEC_STD_WRITE_DAC (and SEC_STD_READ_CONTROL, but ignoring that bit from here on)

* access_check_max_allowed() returns READ | SEC_STD_WRITE_DAC permissions

* se_access_check() assigns result of access_check_max_allowed (READ | SEC_STD_WRITE_DAC) to bits_remaining

* se_access_check() processes ACL to check if all bits in bits_remaining can be satisfied

* as the ACL contains an "Owner Rights" ACE that doesn NOT grant SEC_STD_WRITE_DAC (only READ), at the end of processing the ACL bits_remaining still contains SEC_STD_WRITE_DAC

* se_access_check() fails with ACCESS_DENIED because bits_remaining is not zero
Comment 1 Ralph Böhme 2019-03-07 08:52:52 UTC
Created attachment 14908 [details]
Patch for 4.8, 4.9 and 4.10 cherry-picked from master
Comment 2 Jeremy Allison 2019-03-07 17:13:52 UTC
Thanks Ralph, I was planning to do this work myself today (you saved me a job :-). I'll review asap.
Comment 3 Jeremy Allison 2019-03-07 20:41:19 UTC
Comment on attachment 14908 [details]
Patch for 4.8, 4.9 and 4.10 cherry-picked from master

LGTM for all branches !
Comment 4 Jeremy Allison 2019-03-07 20:41:49 UTC
Re-assigning to Karolin for inclusion in 4.8.next, 4.9.next, 4.10.rcNext.
Comment 5 Karolin Seeger 2019-03-11 07:52:31 UTC
(In reply to Jeremy Allison from comment #4)
Pushed to autobuild-v4-{10,9,8}-test.
Comment 6 Karolin Seeger 2019-03-12 09:37:19 UTC
(In reply to Karolin Seeger from comment #5)
Pushed to all branches.
Closing out bug report.

Thanks!