Hello, when using custom LDAP attributes, Samba seems to recognize the searchFlags confidential flag on custom attributes and hides them from all non-admin users. However, the values of the attributes can still be guessed efficiently by brute forcing them one character after another in a wildcard search query: (myConfidentialattribute=*) => 1 search result (myConfidentialattribute=a*) => 0 search results (myConfidentialattribute=b*) => 1 search result (myConfidentialattribute=ba*) => 0 search results (myConfidentialattribute=bb*) => 1 search result (myConfidentialattribute=bba*) => 0 search results ... (myConfidentialattribute=bbz*) => 0 search results The search results themselves don't actually return the attribute itself, even if requested, but by the (non-=existence of the result the value is exposed. This way, a non-privileged user can quickly find out the value "bb" of the attribute when the expected charset is limited. Yours sincerely Phillip Kuhrt
In testenv, after modifying the description attribute with searchFlags: 128 (0x80), you can query (note that using the Administrator account has different behaviour): garming@addc:~/samba-review$ bin/ldbsearch -H ldap://$SERVER -Utestallowed\ account%$PASSWORD "(description=All*)" name # record 1 dn: CN=Domain Controllers,CN=Users,DC=addom,DC=samba,DC=example,DC=com name: Domain Controllers # record 2 dn: CN=Domain Computers,CN=Users,DC=addom,DC=samba,DC=example,DC=com name: Domain Computers # record 3 dn: CN=Domain Guests,CN=Users,DC=addom,DC=samba,DC=example,DC=com name: Domain Guests # record 4 dn: CN=Domain Users,CN=Users,DC=addom,DC=samba,DC=example,DC=com name: Domain Users # Referral ref: ldap://addom.samba.example.com/CN=Configuration,DC=addom,DC=samba,DC=example,DC=com # Referral ref: ldap://addom.samba.example.com/DC=DomainDnsZones,DC=addom,DC=samba,DC=example,DC=com # Referral ref: ldap://addom.samba.example.com/DC=ForestDnsZones,DC=addom,DC=samba,DC=example,DC=com
There seems to be an attempt in the acl module to implement this behaviour correctly, prior to the search occuring (rather than post-hoc, like the acl_read module is currently doing). In acl_search_update_confidential_attrs it fails to get past the first if-statement and always short-circuits the function (which likely wouldn't have worked anyways though).
Can Red Hat help us get a CVE for this bug?
Can we (again) please get a CVE for this issue allocated. Thanks!
I understand how this issue comes to be and have spent some time looking into possible solutions. This is my current work item so expect some progress on this soon.
This issue isn't restricted to confidential attributes, as I see it. Any attribute could be declared as not-readable. However to check for that we need to check on the way out, as the ACL is per object. Additionally, there is a really strange interaction with dirsync that I just don't trust, as it is tested to make a deleted objects case work, but has read ACL significance. That leaves the question of what to do with confidential attributes, should we filter them in acl.c like we do passwords (and so not allow the confidential flag to be overridden per-object, and so also avoid a timing attack), or just ensure we include all attributes from the filter in the check for readability. That is the fundamental bug here, that an attribute is only checked in the ACL if the attribute is in the list of attributes being requested (or returned), but not if it is in the filter expression. The whole post-search ACL filter worries me, but at this stage we just need to fix what we have.
Just to note what I said on the team list before SambaXP. I don't expect to be able to work on this work in the short term. Specifically I'm on leave until 20 June and will be busy until at least the end of the month. It would be great if someone else could take this on to ensure we progress this in a reasonable timeframe.
Assigning to Tim as he is going to try and take a look at it. Removing the confidential flag on one note so he can read it.
To be clear, I think untangling the full ACL situation here will take a couple of engineer-weeks. Thankfully Tim has become available to work on it, but the next security release should not be delayed for this issue.
G'Day Nadezhda, I've added you to the CC on this bug because it stems from the original acl.c rewrite. I know that was many, many moons and jobs ago, but if you could at least keep an eye over Tim's work here that would be very helpful.
Note that to reproduce this problem you need to filter the results on another attribute (other than the confidential attribute you're trying to guess). This tripped me up when I initially tried to reproduce it. E.g. if you don't filter on any attributes, then you don't see the problem: bin/ldbsearch -H ldap://$SERVER -b 'OU=confattr8505368,DC=samba,DC=example,DC=com' '(businessCategory=a*)' -Utestallowed\ account%locDCpass1 # returned 0 records Nor does it happen if you filter on the confidential attribute itself: bin/ldbsearch -H ldap://$SERVER -b 'OU=confattr8505368,DC=samba,DC=example,DC=com' '(businessCategory=a*)' -Utestallowed\ account%locDCpass1 businessCategory # returned 0 records but filter on a random other attribute (e.g. 'cn') and you do see the problem: bin/ldbsearch -H ldap://$SERVER -b 'OU=confattr8505368,DC=samba,DC=example,DC=com' '(businessCategory=a*)' -Utestallowed\ account%locDCpass1 cn # record 1 dn: OU=confattr8505368,DC=samba,DC=example,DC=com # returned 1 records
Created attachment 14318 [details] Tests that demonstrate the problem Attached test cases that highlight the problem. They all pass against Windows and all fail against Samba.
I've got a basic fix that extends the attribute rights checks in aclread_callback() to also traverse the search-tree attributes and check those too. One problem is this fix breaks the acl.py tests, i.e. test_search1(). The problem is the user has List Contents rights for a child, but doesn't have Read Property rights to view objectClass. My change means the search on objectClass=* now no longer returns the child, whereas Windows does return it. However, Windows doesn't have a blanket allow rule on LDB_OP_PRESENT searches, because things like instanceType=* or confidentialAttribute=* do NOT return the child. So it seems like there is some additional logic here that Windows is using, but I can't find this behaviour documented anywhere in the spec. I've queried our Microsoft liason about this, with reference to the current ACL test (i.e. without disclosing the actual details of this #13434 bug). The other problem I've been looking at is the '!' search operation. Windows behaviour more closely matches the kludgeACLredactedattribute behaviour, in that the '!' searches act as if the hidden attribute doesn't exist at all, so it returns ALL objects. Unfortunately, this doesn't gel well with the fix I've currently got. I'm leaning towards diverging from Windows behaviour here slightly, and just assume what we really want is the search to not be disclosive at all, i.e. always return zero results if a !(confidentialAttribute=blah) search filter is specified. I'll double-check with Andrew when he's back on Thursday. Other than that, there's still a few more test cases I'd like to add for this problem.
Created attachment 14337 [details] WIP changes Attaching my current WIP patches, mostly just as a backup. They contain some tests that highlight the problem and a basic change that fixes it. I still need to: - rework the tests more (just testing against one object isn't really good enough) - Investigate the '!' search cases more, as I think Samba is disclosive in the deny-ACL cases, but I'm not sure there's much we can do about that... - Investigate the dirsync behaviour, as my patch breaks behaviour that someone intentionally added back in 2011 (although there appear to be no tests to highlight any breakage)
Can we urgently get a CVE for this please? We need to freeze the next security release on 1 August.
Created attachment 14364 [details] proposed CVE text (needs CVE)
Created attachment 14367 [details] Fix patch-set (some test rework needed) Attached is my latest patch-set. I think it's close to final, apart from I'm still cleaning up the confidential_attr.py changes. RE: my previous comment: - My patch now honours the existing dirsync behaviour. I've got a test-case that proves that this is not disclosive (but there's still no test case to prove the existing dirsync behaviour is needed in the first place). - I've extended the tests so that they now involve multiple objects (which complicates things somewhat). I'm still tidying up these tests (i.e. top-most patch). - I think the Samba behaviour for the negative search (i.e. '!' searches) is not disclosive for confidential attributes. Regardless of whether the '!' expression matches the attribute or not, it should never appear in the search results, so there's no way to guess it.
Created attachment 14368 [details] Patch-set ready for review Attached patch.txt with the new test code cleaned up. Changes should be ready for review/CI/delivery now.
Attributes with fCONFIDENTIAL set in schema (2008R2): unixUserPassword, msFVE-KeyPackage, msFVE-RecoveryPassword, msPKIAccountCredentials, msPKIAccountCredentials, msPKI-CredentialRoamingTokens, msPKIDPAPIMasterKeys, msPKIRoamingTimeStamp, msTPM-OwnerInformation Sub-set with fPRESERVEONDELETE set as well: msFVE-KeyPackage, msFVE-RecoveryPassword, msTPM-OwnerInformation One area I still wasn't entirely sure about was the dirsync behaviour with deleted objects. If an object is deleted *and* the user doesn't have rights to view it, then it looks like the dirsync module will still return just the object-GUID for the object. I still don't fully understand what the dirsync code is trying to achieve and whether this would potentially leak sensitive information. This corner-case would only potentially affect the attributes above with both fCONFIDENTIAL and fPRESERVEONDELETE set.
Created attachment 14372 [details] Patch for v4.8.3 and v4.7.8. Attached patch should apply to samba v4.8.3 and v4.7.8. Only differences are: - acl.py had a minor difference in the surrounding lines, which meant the patch didn't apply cleanly. - confidential_attr.py required a set_schema_update_now() function that didn't exist on the older branches. Otherwise the new tests didn't run. Looks like v4.6.12 still doesn't apply cleanly.
Created attachment 14373 [details] proposed CVE text (needs CVE) v2 Updated CVE text. While the 'remaining issues' text is unfortunate, we are out of resource to continue down the dirsync rathole as this area has insufficient testing. It is safer to just document this corner case for now.
Created attachment 14374 [details] proposed CVE text (needs CVE) v3
Created attachment 14376 [details] Patch for v4.6.12 Added patch for 4.6 branch (appeared to just be a whitespace difference in acl_read.c that it was complaining about).
CVE number requested from secalert@redhat.com.
Created attachment 14377 [details] proposed CVE text (needs CVE) v4 Due entirely to an accident in untested code, the issue with dirsync does not appear to be exploitable, so remove it as a 'remaining issue'. Instead note that timing attacks (acl processing is after the search filter match) might however be possible.
Created attachment 14378 [details] Patch-set ready for review Added updated patch-set: - removed a spurious 'break' I had incorrectly added in aclread_callback() (I don't think it had an impact on functionality - it only affected the dirsync case, where the dirsync module would drop the message anyway). - added an extra test case (in a new patch) to prove that dirsync does not leak confidential information for deleted objects
Created attachment 14379 [details] Patch for v4.8.3 and v4.7.8.
Comment on attachment 14378 [details] Patch-set ready for review Looks great. Thanks!
Created attachment 14380 [details] Patch for v4.6.12
Opening up to vendors. Release details on bug 13509.
Comment on attachment 14378 [details] Patch-set ready for review Yes, really good. Tim, if you do end up doing another version, you might want to fix some of these typos in the commit message, but I am NOT making my review contingent on that. The only reason I mention it is these messages will be scrutinised a bit more than usual. >Subject: [PATCH 01/10] security: Move object-specific access checks into > separate function [...] >from ading in the boolean grant_access return variable. ^^^^^ *adding >Subject: [PATCH 03/10] tests: Add tests for guessing confidential attributes >These tests all pass when run against a Windows Dc and all fail against ^^ this would be better as DC, like on the next line: >a Samba DC. >Subject: [PATCH 06/10] acl_read: Split access_mask logic out into helper > function > >So we can re-use the same logic laster for checking the search-ops. ^^^^^^ later? >Subject: [PATCH 10/10] tests: Add extra test for dirsync deleted object > corner-case > hangs around for deleted objcts. Because the attributes now persist ^^^^^^
CVE-2018-10919 assigned by Red Hat security.
A full autobuild on 4.5 shows this issue likely also a problem for 4.6: UNEXPECTED(error): samba.tests.samba_tool.sites.samba.tests.samba_tool.sites.SitesCmdTestCase.test_site_create(ad_dc:local) REASON: Exception: Exception: Traceback (most recent call last): File "/home/ubuntu/autobuild/b13732/samba/bin/python/samba/tests/samba_tool/sites.py", line 53, in test_site_create expression='(dn=%s)' % str(dnsite)) LdbError: (1, 'LDAP error 1 LDAP_OPERATIONS_ERROR - <acl_read: CN=new_site,CN=Sites,CN=Configuration,DC=addom,DC=samba,DC=example,DC=com check search ops Operations err or - acl_read: CN=new_site,CN=Sites,CN=Configuration,DC=addom,DC=samba,DC=example,DC=com cannot find attr[dn] in schema\n\n> <>') FAILED (0 failures, 1 errors and 0 unexpected successes in 0 testsuites) We need to also confirm the correct behaviour for non-schema attributes searches (which should just match nothing).
(In reply to Andrew Bartlett from comment #33) Samba 4.7 and earlier have 44eee9ce9e9818df8387b2b3782504408112f12c (or a backport) in it, which avoids the incorrect dn= filter. However we need to check on other incorrect filters. I'm concerned we may now error out where we should just return no match.
(In reply to Andrew Bartlett from comment #34) Looking at this some more, the error is in post-processing. So we would error out when we DO match, so except in conjunction with (eg) an LDAP or/not it is mostly restricted to the not-actually-valid 'dn=' search. While fail-open is always a bit scary, we 'just' need to fail open in check_attr_access_rights(): + attr = dsdb_attribute_by_lDAPDisplayName(ac->schema, attr_name); + if (!attr) { + ldb_debug_set(ldb, LDB_DEBUG_FATAL, + "acl_read: %s cannot find attr[%s] in schema\n", + ldb_dn_get_linearized(dn), attr_name); + return LDB_ERR_OPERATIONS_ERROR; + } Long-term we could consolidate the parse tree modification into an updated resolve_oids, and there replace all non-schema/misspelled search attributes, and the secret attributes with a 'always false' extended match rule there.
Created attachment 14383 [details] Updated patch for master Update patch for master, correctly handles attributes not in the schema
Created attachment 14386 [details] diff between the previous and current patch set
Created attachment 14387 [details] Updated CVE text v5
Created attachment 14388 [details] Updated patch applies to v4-8-test and v4-7-test
Created attachment 14389 [details] patch for master and 4.9 with CVE
Created attachment 14390 [details] patch for 4.7 and 4.8 with CVE
Created attachment 14391 [details] patch for master and 4.9 with CVE and Reviewed-by: tags
Created attachment 14392 [details] patch for 4.5 and 4.6 with CVE and reviewed by tags.
Comment on attachment 14387 [details] Updated CVE text v5 Hi Andrew, Two proposals: 1) Could we make it more explicit in the Description text that only the AD LDAP server is affected (similar to the wording of the CVE description in https://bugzilla.samba.org/show_bug.cgi?id=CVE-2018-10918 )? 2) This is already contained in the subject line, but the wording is slightly awkward to me, maybe "Confidential attribute disclosure from (the) AD LDAP server" or so? (Some preposition clarifies here, imho). (I hope my understanding is correct here.. :-) Thanks - Michael
(In reply to Michael Adam from comment #44) Regarding 1) As you say in 2), your concern described in the subject, but I'm happy to look over a new proposal. For 2) I'm happy to with wording "Confidential attribute disclosure from (the) AD LDAP server" Can you help keep this collaborative and upload a new text? Thanks, Andrew Bartlett
Comment on attachment 14391 [details] patch for master and 4.9 with CVE and Reviewed-by: tags This patch passed a full autobuild on master.
This is a CVSS 6.5 by my calculations, assuming the attributes are in use. https://www.first.org/cvss/calculator/3.0#CVSS:3.0/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:N/A:N
Comment on attachment 14388 [details] Updated patch applies to v4-8-test and v4-7-test This patch passed a full autobuild on 4.8
The patch set passed a full autobuild on Samba 4.5 and 4.9. On the particular test environment in use (Catalyst Cloud VMs) Samba 4.7 and 4.6 both hang at: ==> samba.stdout <== [560(3421)/2280 at 1h21m1s] samba3.smb2.kernel-oplocks(nt4_dc) I'm unable to put more time into autobuild watching sadly.
Created attachment 14400 [details] Updated CVE text v6.
Comment on attachment 14400 [details] Updated CVE text v6. LGTM, thanks! Andrew: I could not get back earlier due to travelling back home from India.
Samba 4.8.4, 4.9.7 an 4.6.16 have been released in order to address these defects.
(In reply to Karolin Seeger from comment #52) 4.7.9
Pushed to autobuild-master and autobuild-v4-9-test.
Removing embargo restrictions (specifically so bug 13556 makes more sense).
Pushed to all branches. Closing out bug report. Thanks!