Bug 12497 (CVE-2019-14902) - [SECURITY] CVE-2019-14902 Replication of ACLs down subtree on AD Directory not automatic
Summary: [SECURITY] CVE-2019-14902 Replication of ACLs down subtree on AD Directory no...
Status: RESOLVED FIXED
Alias: CVE-2019-14902
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.5.2
Hardware: x64 Linux
: P4 major (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14220
  Show dependency tree / graph
 
Reported: 2017-01-04 18:58 UTC by (dead mail address)
Modified: 2020-02-16 20:59 UTC (History)
7 users (show)

See Also:

Attachments
initial patch for master (needs tests) (7.87 KB, patch)
2019-11-26 05:07 UTC, Andrew Bartlett 		no flags Details
a patch that passes the tests (24.62 KB, patch)
2019-12-06 05:40 UTC, Andrew Bartlett 		no flags Details
first finished patch for master (45.52 KB, patch)
2019-12-12 02:54 UTC, Andrew Bartlett 		no flags Details
first patch for 4.11 (40.97 KB, patch)
2019-12-12 03:10 UTC, Andrew Bartlett 		no flags Details
first patch for 4.10 (7.52 KB, patch)
2019-12-12 03:10 UTC, Andrew Bartlett 		no flags Details
first patch for 4.9 (40.99 KB, patch)
2019-12-12 03:18 UTC, Andrew Bartlett 		no flags Details
patch for master (v2) (57.47 KB, patch)
2019-12-16 04:14 UTC, Andrew Bartlett 		abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
patch for Samba 4.11 (cherry-picked from master patch) v2 (52.53 KB, patch)
2019-12-16 04:16 UTC, Andrew Bartlett 		abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
patch for Samba 4.10 (cherry-picked/backported from master patch) v2 (52.56 KB, patch)
2019-12-16 04:16 UTC, Andrew Bartlett 		abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
patch for Samba 4.9 (cherry-picked/backported from master patch) v2 (52.56 KB, patch)
2019-12-16 04:19 UTC, Andrew Bartlett 		abartlet: review? (gary)
gary: review+
abartlet: ci-passed+
Details
first draft security advisory (2.21 KB, patch)
2019-12-16 04:47 UTC, Andrew Bartlett 		gary: review+
Details
updated advisory (v2) with manual steps included (2.88 KB, text/plain)
2019-12-19 02:20 UTC, Andrew Bartlett 		abartlet: review? (gary)
 Details
updated advisory (v3) (3.08 KB, text/plain)
2019-12-19 02:53 UTC, Andrew Bartlett 		gary: review+
abartlet: review+
Details
Updated advisory with version numbers (3.09 KB, text/plain)
2020-01-17 08:51 UTC, Karolin Seeger 		no flags Details
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description (dead mail address) 2017-01-04 18:58:43 UTC 
The main issue is that applying a new ACL to directory objects only fully propagates down the tree on the server on which the ACL modification was performed. On other DCs replication only seems to propagate the new ACLs down one or two levers at most. This affects delegation in ADUC as well. The only solution is to run a samba-tool drs replicate --full-sync on the other DCs. I post below the posts I've sent to the ML with no response so far:

>> I've been testing Samba 4.5.1 extensively as an AD DC. We have 3 DC set up, and replication of users, groups, OUs, DNS etc has been working fine.
>>
>> However we wanted to add some custom attributes and a class to the schema (an assortment of string and numericalString) for our own purposes. This also worked fine (and the Schema replication worked), but some oddness happened when we wanted to restrict access to one of these attributes.
>>
>> The class was added to the "user" AD class as an auxiliary, and then the following procedure was used:
>>
>> https://support.microsoft.com/en-us/kb/320528
>>
>> To add anonymous access to the Public Information, Phone and Mail, and our additional attributes (excepting the restricted one). Then a deny ACL for "Everyone" and "Anonymous Logon" was added recursively for the restricted attribute at the root of the domain tree on descendent User objects.
>>
>> This seemed to work on the server that ADSI edit was connected to when tested with ldapsearch, but *not* on the other two DCs. They behaved as if no ACLs had been changed. When I connected ADSI edit to the other DCs I could see that the ACLs seemed to be present at the domain root, but were not propagating down the tree even though the inherit box was checked on subordinates.
>>
>> I had to do:
>>
>> samba-tool drs replicate s4-dc-01 s4-dc-02 DC=my,DC=ifa,DC=net --full-sync
>>
>> (where s4-dc-02 was the "working" DC) and all seemed to be fixed, both using ldapsearch and ADSI Edit.
>>
>> Is it a known issue that ACLs aren't completely replicated? Or is it that "custom" attributes cause problems with ACLs (even though they are applied as auxiliary to the "user" AD class.
>>
>> Otherwise everything is working 100% correctly (showrepl gives no errors).
>>
>>
>> Cheers,
>>
>> Alex
>>
>

Another update on this problem:

This seems only to occur if ACLs on directory objects or delegation is changed /after/ DCs have been joined to the domain.

A fresh DC will get all of the ACLs correct as it does seem to do a full sync of objects from an existing DC when joined.

However if we change, say, delegation in ADUC, we will seem to get some part of the corresponding ACLs not replicated on other existing DCs (sometimes it's all the others than the one the change hit, sometimes just one). For instance, we delegated control of user objects below a certain ou (to all descendant user objects) and this is visible from ADUC in the descendant objects on two DCs, but on a third, the ACL only shows on that ou itself - and despite it indeed saying it applies to all descendant user objects in ADUC, when we look at any entry in the tree below, it is not applied in the list of ACLs.
Comment 1 (dead mail address) 2017-02-16 18:50:06 UTC 
This is still affecting us. Every time we need to enable additional access to any attribute we need to perform this workaround. 

Does anyone have any input or similar experience?
Comment 2 Andrew Bartlett 2017-03-03 20:39:58 UTC 
I think the fundemental issue is that the descriptor module is above repl_meta_data in the list in samba_dsdb.  This means that the recursive SD propagation does not happen for changes that come over replication.

Because DSDB_CONTROL_SEC_DESC_PROPAGATION_OID is set by the descriptor module, the repl_meta_data module does not update the metadata for the child objects, so the changes made to nTSecurityDescriptor are not seen by other DCs until a full sync.
Comment 3 Stefan Metzmacher 2017-03-05 22:31:37 UTC 
(In reply to Andrew Bartlett from comment #2)

dsdb_module_schedule_sd_propagation() uses DSDB_FLAG_TOP_MODULE, so it's likely
to be something else.
Comment 4 Andrew Bartlett 2017-03-06 02:49:16 UTC 
(In reply to Stefan Metzmacher from comment #3)
OK, that makes things more difficult.

Thanks for the pointer.
Comment 5 Andrew Bartlett 2019-11-06 04:00:42 UTC 
Steps to reproduce:

 SELFTEST_TESTENV=vampire_dc make testenv

 bin/samba-tool ou create ou=l1 -H ldap://$SERVER -U$USERNAME%$PASSWORD
 bin/samba-tool ou create ou=l2,ou=l1 -H ldap://$SERVER -U$USERNAME%$PASSWORD

 bin/samba-tool dsacl set --sddl "(A;CIOI;GA;;;SY)" --objectdn=ou=l1,DC=samba,DC=example,DC=com -H  ldap://$SERVER -U$USERNAME%$PASSWORD


 bin/samba-tool dsacl get --objectdn=ou=l2,ou=l1,DC=samba,DC=example,DC=com -H  ldap://$DC_SERVER -U$USERNAME%$PASSWORD
 bin/samba-tool dsacl get --objectdn=ou=l2,ou=l1,DC=samba,DC=example,DC=com -H  ldap://$SERVER -U$USERNAME%$PASSWORD

These should give the same ACL, but don't. 

A full sync fixes it:

bin/samba-tool drs replicate --full-sync $SERVER $DC_SERVER DC=samba,DC=example,DC=com -U$USERNAME%$PASSWORD

bin/samba-tool drs replicate --full-sync $DC_SERVER $SERVER DC=samba,DC=example,DC=com -U$USERNAME%$PASSWORD
Comment 6 Andrew Bartlett 2019-11-06 04:25:20 UTC 
To get this under the debugger, after creating the ou object in the testenv, run:
 bin/samba-tool drs options --dsa-option=+DISABLE_INBOUND_REPL -s st/vampire_dc/etc/smb.conf  -U$USERNAME%$PASSWORD
 bin/samba-tool drs options --dsa-option=+DISABLE_OUTBOUND_REPL -s st/vampire_dc/etc/smb.conf  -U$USERNAME%$PASSWORD
 bin/samba-tool drs options --dsa-option=+DISABLE_OUTBOUND_REPL -s st/ad_dc_ntvfs/etc/smb.conf  -U$USERNAME%$PASSWORD
 bin/samba-tool drs options --dsa-option=+DISABLE_INBOUND_REPL -s st/ad_dc_ntvfs/etc/smb.conf  -U$USERNAME%$PASSWORD

(that stops background replication)

Then set the SD:

 bin/samba-tool dsacl set --sddl "(A;CIOI;GA;;;SY)" --objectdn=ou=l1,DC=samba,DC=example,DC=com -H  ldap://$DC_SERVER -U$USERNAME%$PASSWORD

and the debug:

 gdb --args python3 bin/samba-tool drs replicate $SERVER $DC_SERVER DC=samba,DC=example,DC=com -U$USERNAME%$PASSWORD --sync-forced --local -s st/vampire_dc/etc/smb.conf
Comment 7 Andrew Bartlett 2019-11-06 04:54:27 UTC 
Bug 8621 interestingly added the inheritance here, but specifically only handled the originating update case.
Comment 8 Andrew Bartlett 2019-11-26 05:03:41 UTC 
While this issue is well known and well described, it has now finally been triaged as a security issue.  Therefore marking this as embargoed while we get a CVE and prepare a security release.
Comment 9 Andrew Bartlett 2019-11-26 05:07:29 UTC 
Created attachment 15633 [details]
initial patch for master (needs tests)

This patch addresses the primary issue, that being that replicated changes almost never cause inherited ACEs in an ACL to apply over replication.
Comment 10 Andrew Bartlett 2019-11-26 05:14:38 UTC 
With a bit of work due to missing commands in older versions, I've reproduced the issue back to before Samba 4.3.
Comment 11 Andrew Bartlett 2019-11-26 05:21:09 UTC 
CVSS score: 

CVSS:3.1/AV:N/AC:L/PR:L/UI:R/S:U/C:L/I:L/A:N
Comment 12 Andrew Bartlett 2019-12-05 04:35:10 UTC 
I'm making good progress on this but don't have a working fix right now, the tests need to defend against a race, if you run the reproduction advise in the bug too fast then they will pass. 

If you slow down then it matches what I have in my branch, abartlet-bug-12497-inherit, on the private gitlab.
Comment 13 Andrew Bartlett 2019-12-06 05:40:42 UTC 
Created attachment 15670 [details]
a patch that passes the tests

Still some cleanup needed but this patch seems to do what we need.
Comment 14 Andrew Bartlett 2019-12-12 02:54:46 UTC 
Created attachment 15678 [details]
first finished patch for master
Comment 15 Andrew Bartlett 2019-12-12 03:10:13 UTC 
Created attachment 15679 [details]
first patch for 4.11
Comment 16 Andrew Bartlett 2019-12-12 03:10:41 UTC 
Created attachment 15680 [details]
first patch for 4.10
Comment 17 Andrew Bartlett 2019-12-12 03:18:13 UTC 
Created attachment 15681 [details]
first patch for 4.9
Comment 18 Andrew Bartlett 2019-12-16 04:14:39 UTC 
Created attachment 15685 [details]
patch for master (v2)
Comment 19 Andrew Bartlett 2019-12-16 04:16:01 UTC 
Created attachment 15686 [details]
patch for Samba 4.11 (cherry-picked from master patch) v2
Comment 20 Andrew Bartlett 2019-12-16 04:16:47 UTC 
Created attachment 15687 [details]
patch for Samba 4.10 (cherry-picked/backported from master patch) v2
Comment 21 Andrew Bartlett 2019-12-16 04:19:08 UTC 
Created attachment 15688 [details]
patch for Samba 4.9 (cherry-picked/backported from master patch) v2

I've updated the patch with a change to no longer be named-based (and so now much less sensitive to renames), and to test that we have inheritance, not just that it matches.
Comment 22 Andrew Bartlett 2019-12-16 04:47:45 UTC 
Created attachment 15689 [details]
first draft security advisory
Comment 23 Andrew Bartlett 2019-12-17 00:59:58 UTC 
Thanks gary, assigning to Karolin for the next security release (which is why I've not included specific release versions in the advisory).
Comment 24 Karolin Seeger 2019-12-18 07:29:14 UTC 
Planned release date: Tuesday, January 14 2020
Comment 25 Andrew Bartlett 2019-12-19 02:20:19 UTC 
Created attachment 15706 [details]
updated advisory (v2) with manual steps included

On reflection, a dbcheck rule would be ideal, but we don't have a dbcheck rule for ACL inheritance re-calculation at this stage.
Comment 26 Andrew Bartlett 2019-12-19 02:53:29 UTC 
Created attachment 15708 [details]
updated advisory (v3)

Clarifying that the replication just needs to be TO each DC, not pair-wise.
Comment 27 Stefan Metzmacher 2019-12-19 04:50:10 UTC 
(In reply to Andrew Bartlett from comment #25)

Also note that Windows uses the dSCorePropagationData attribute to
maintain state for their background task... (I don't know more details,
I just recently noticed that attribute again and wanted to find out what it's for on Windows).
Comment 28 Karolin Seeger 2020-01-08 10:04:52 UTC 
Waiting for review on the advisory.
Comment 29 Karolin Seeger 2020-01-13 09:50:36 UTC 
Will be delayed.
The new planned release date will be published as soon as possible.
Comment 30 Karolin Seeger 2020-01-14 08:38:18 UTC 
Planned release date: Tuesday, January 21 2020
Opening bug report for vendors.
Comment 31 Marc Deslauriers 2020-01-15 12:45:54 UTC 
Has anyone managed to backport this to 4.3 yet?
Comment 32 Karolin Seeger 2020-01-17 08:51:46 UTC 
Created attachment 15738 [details]
Updated advisory with version numbers
Comment 33 Karolin Seeger 2020-01-21 09:59:37 UTC 
Samba 4.11.5, 4.10.12 and 4.9.18 have been shipped to address these defects.
Comment 34 Karolin Seeger 2020-01-21 10:09:10 UTC 
Merged into v4-{11,10,9}-test.
Comment 35 Karolin Seeger 2020-01-21 10:11:31 UTC 
Pushed to autobuild-master.
Comment 36 Karolin Seeger 2020-01-23 09:50:15 UTC 
Pushed to master.
Closing out bug report.

Thanks!
Comment 37 Andrew Bartlett 2020-02-16 20:59:11 UTC 
Removing embargo, after checking that there are no comments or attachments that need additional privacy.