Bug 13434 (CVE-2018-10919) - [SECURITY] CVE-2018-10919 - Confidential attribute disclosure via substring search
Summary: [SECURITY] CVE-2018-10919 - Confidential attribute disclosure via substring s...
Alias: CVE-2018-10919
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.8.1
Hardware: All All
: P5 critical (vote)
Target Milestone: ---
Assignee: Tim Beale
QA Contact: Samba QA Contact
Depends on:
Blocks: 13556 13509
  Show dependency treegraph
Reported: 2018-05-11 18:56 UTC by Andrew Bartlett
Modified: 2018-08-24 09:30 UTC (History)
18 users (show)

See Also:

proposed CVE text (needs CVE) (2.37 KB, text/plain)
2018-07-30 03:51 UTC, Andrew Bartlett
gary: review+
Fix patch-set (some test rework needed) (110.40 KB, text/plain)
2018-07-30 23:26 UTC, Tim Beale
no flags Details
Patch-set ready for review (78.22 KB, text/plain)
2018-07-31 01:55 UTC, Tim Beale
no flags Details
Patch for v4.8.3 and v4.7.8. (78.37 KB, text/plain)
2018-07-31 03:46 UTC, Tim Beale
no flags Details
proposed CVE text (needs CVE) v2 (2.94 KB, text/plain)
2018-07-31 04:03 UTC, Andrew Bartlett
no flags Details
proposed CVE text (needs CVE) v3 (2.72 KB, text/plain)
2018-07-31 04:11 UTC, Andrew Bartlett
gary: review+
Patch for v4.6.12 (78.43 KB, text/plain)
2018-07-31 04:26 UTC, Tim Beale
no flags Details
proposed CVE text (needs CVE) v4 (2.74 KB, text/plain)
2018-07-31 22:29 UTC, Andrew Bartlett
dbagnall: review+
Patch-set ready for review (91.19 KB, text/plain)
2018-08-01 03:39 UTC, Tim Beale
abartlet: review+
dbagnall: review+
Patch for v4.8.3 and v4.7.8. (91.34 KB, text/plain)
2018-08-01 04:02 UTC, Tim Beale
abartlet: review+
Patch for v4.6.12 (91.41 KB, text/plain)
2018-08-01 04:05 UTC, Tim Beale
abartlet: review+
Updated patch for master (92.58 KB, patch)
2018-08-03 04:30 UTC, Gary Lockyer
abartlet: review+
diff between the previous and current patch set (1.63 KB, patch)
2018-08-05 22:23 UTC, Andrew Bartlett
no flags Details
Updated CVE text v5 (2.76 KB, text/plain)
2018-08-05 22:40 UTC, Andrew Bartlett
obnox: review-
Updated patch applies to v4-8-test and v4-7-test (92.81 KB, patch)
2018-08-05 22:43 UTC, Gary Lockyer
no flags Details
patch for master and 4.9 with CVE (92.94 KB, patch)
2018-08-05 22:45 UTC, Andrew Bartlett
gary: review+
patch for 4.7 and 4.8 with CVE (93.09 KB, patch)
2018-08-05 22:56 UTC, Andrew Bartlett
gary: review+
patch for master and 4.9 with CVE and Reviewed-by: tags (93.96 KB, patch)
2018-08-05 23:51 UTC, Andrew Bartlett
gary: review+
patch for 4.5 and 4.6 with CVE and reviewed by tags. (94.04 KB, patch)
2018-08-06 01:47 UTC, Gary Lockyer
abartlet: review+
Updated CVE text v6. (2.75 KB, text/plain)
2018-08-07 23:39 UTC, Jeremy Allison
obnox: review+
jra: review? (garming)
abartlet: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2018-05-11 18:56:51 UTC

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
Comment 1 Garming Sam 2018-05-14 02:36:35 UTC
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
Comment 2 Garming Sam 2018-05-14 02:39:31 UTC
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).
Comment 3 Garming Sam 2018-05-14 02:47:13 UTC
Can Red Hat help us get a CVE for this bug?
Comment 4 Andrew Bartlett 2018-05-21 02:58:35 UTC
Can we (again) please get a CVE for this issue allocated.  Thanks!
Comment 5 Andrew Bartlett 2018-05-21 10:01:50 UTC
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.
Comment 6 Andrew Bartlett 2018-05-31 01:18:34 UTC
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.
Comment 7 Andrew Bartlett 2018-06-13 16:56:38 UTC
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.
Comment 8 Andrew Bartlett 2018-07-04 22:18:19 UTC
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.
Comment 9 Andrew Bartlett 2018-07-06 03:49:55 UTC
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.
Comment 10 Andrew Bartlett 2018-07-08 19:05:58 UTC
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.
Comment 11 Tim Beale 2018-07-09 02:12:10 UTC
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
Comment 12 Tim Beale 2018-07-11 05:07:52 UTC
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.
Comment 13 Tim Beale 2018-07-23 21:23:48 UTC
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.
Comment 14 Tim Beale 2018-07-26 05:32:32 UTC
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)
Comment 15 Andrew Bartlett 2018-07-30 03:25:01 UTC
Can we urgently get a CVE for this please?  We need to freeze the next security release on 1 August.
Comment 16 Andrew Bartlett 2018-07-30 03:51:04 UTC
Created attachment 14364 [details]
proposed CVE text (needs CVE)
Comment 17 Tim Beale 2018-07-30 23:26:54 UTC
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.
Comment 18 Tim Beale 2018-07-31 01:55:44 UTC
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.
Comment 19 Tim Beale 2018-07-31 03:28:43 UTC
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.
Comment 20 Tim Beale 2018-07-31 03:46:35 UTC
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.
Comment 21 Andrew Bartlett 2018-07-31 04:03:27 UTC
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.
Comment 22 Andrew Bartlett 2018-07-31 04:11:08 UTC
Created attachment 14374 [details]
proposed CVE text (needs CVE) v3
Comment 23 Tim Beale 2018-07-31 04:26:42 UTC
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).
Comment 24 Jeremy Allison 2018-07-31 20:16:35 UTC
CVE number requested from secalert@redhat.com.
Comment 25 Andrew Bartlett 2018-07-31 22:29:33 UTC
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.
Comment 26 Tim Beale 2018-08-01 03:39:24 UTC
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
Comment 27 Tim Beale 2018-08-01 04:02:59 UTC
Created attachment 14379 [details]
Patch for v4.8.3 and v4.7.8.
Comment 28 Andrew Bartlett 2018-08-01 04:03:53 UTC
Comment on attachment 14378 [details]
Patch-set ready for review

Looks great.  Thanks!
Comment 29 Tim Beale 2018-08-01 04:05:13 UTC
Created attachment 14380 [details]
Patch for v4.6.12
Comment 30 Andrew Bartlett 2018-08-01 04:20:37 UTC
Opening up to vendors.  Release details on bug 13509.
Comment 31 Douglas Bagnall 2018-08-01 04:36:47 UTC
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.

>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.

>Subject: [PATCH 10/10] tests: Add extra test for dirsync deleted object
> corner-case

>  hangs around for deleted objcts. Because the attributes now persist
Comment 32 Jeremy Allison 2018-08-01 16:15:52 UTC
CVE-2018-10919 assigned by Red Hat security.
Comment 33 Andrew Bartlett 2018-08-02 06:45:32 UTC
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).
Comment 34 Andrew Bartlett 2018-08-02 06:57:41 UTC
(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.
Comment 35 Andrew Bartlett 2018-08-02 08:21:35 UTC
(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);
+	}

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.
Comment 36 Gary Lockyer 2018-08-03 04:30:04 UTC
Created attachment 14383 [details]
Updated patch for master

Update patch for master, correctly handles attributes not in the schema
Comment 37 Andrew Bartlett 2018-08-05 22:23:48 UTC
Created attachment 14386 [details]
diff between the previous and current patch set
Comment 38 Andrew Bartlett 2018-08-05 22:40:01 UTC
Created attachment 14387 [details]
Updated CVE text v5
Comment 39 Gary Lockyer 2018-08-05 22:43:50 UTC
Created attachment 14388 [details]
Updated patch applies to v4-8-test and v4-7-test
Comment 40 Andrew Bartlett 2018-08-05 22:45:22 UTC
Created attachment 14389 [details]
patch for master and 4.9 with CVE
Comment 41 Andrew Bartlett 2018-08-05 22:56:00 UTC
Created attachment 14390 [details]
patch for 4.7 and 4.8 with CVE
Comment 42 Andrew Bartlett 2018-08-05 23:51:20 UTC
Created attachment 14391 [details]
patch for master and 4.9 with CVE and Reviewed-by: tags
Comment 43 Gary Lockyer 2018-08-06 01:47:26 UTC
Created attachment 14392 [details]
patch for 4.5 and 4.6 with CVE and reviewed by tags.
Comment 44 Michael Adam 2018-08-06 17:43:30 UTC
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
Comment 45 Andrew Bartlett 2018-08-06 21:21:39 UTC
(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?


Andrew Bartlett
Comment 46 Andrew Bartlett 2018-08-06 22:06:53 UTC
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.
Comment 47 Andrew Bartlett 2018-08-07 02:15:37 UTC
This is a CVSS 6.5 by my calculations, assuming the attributes are in use. 

Comment 48 Andrew Bartlett 2018-08-07 04:01:43 UTC
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
Comment 49 Andrew Bartlett 2018-08-07 21:05:30 UTC
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.
Comment 50 Jeremy Allison 2018-08-07 23:39:35 UTC
Created attachment 14400 [details]
Updated CVE text v6.
Comment 51 Michael Adam 2018-08-08 09:24:29 UTC
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.
Comment 52 Karolin Seeger 2018-08-14 08:44:13 UTC
Samba 4.8.4, 4.9.7 an 4.6.16 have been released in order to address these defects.
Comment 53 Karolin Seeger 2018-08-14 08:54:12 UTC
(In reply to Karolin Seeger from comment #52)
Comment 54 Karolin Seeger 2018-08-14 10:06:58 UTC
Pushed to autobuild-master and autobuild-v4-9-test.
Comment 55 Andrew Bartlett 2018-08-15 00:55:26 UTC
Removing embargo restrictions (specifically so bug 13556 makes more sense).
Comment 56 Karolin Seeger 2018-08-24 09:30:32 UTC
Pushed to all branches.
Closing out bug report.