Bug 14655 (CVE-2021-20277) - CVE-2021-20277 [SECURITY] out of bounds read in ldb_handler_fold
Summary: CVE-2021-20277 [SECURITY] out of bounds read in ldb_handler_fold
Status: RESOLVED FIXED
Alias: CVE-2021-20277
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: unspecified
Hardware: All All
: P5 major (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14656 14661
  Show dependency tree / graph
 
Reported: 2021-03-05 09:50 UTC by Douglas Bagnall
Modified: 2021-04-01 05:34 UTC (History)
6 users (show)

See Also:

Attachments
the immediate fix (leaving other bugs) (992 bytes, patch)
2021-03-05 09:50 UTC, Douglas Bagnall 		no flags Details
additional fix for other bugs (2.13 KB, patch)
2021-03-05 10:18 UTC, Douglas Bagnall 		no flags Details
patch for master with tests (2.77 KB, patch)
2021-03-10 21:43 UTC, Douglas Bagnall 		no flags Details
patch for master (v2) (2.81 KB, patch)
2021-03-12 01:10 UTC, Andrew Bartlett 		dbagnall: review+
abartlet: ci-passed+
Details
patch backported to 4.14 (v2) (9.91 KB, patch)
2021-03-12 01:13 UTC, Andrew Bartlett 		dbagnall: review+
abartlet: ci-passed+
Details
patch backported to 4.13 (v2) (9.91 KB, patch)
2021-03-12 01:13 UTC, Andrew Bartlett 		dbagnall: review+
abartlet: ci-passed+
Details
patch backported to 4.12 (v2) (9.91 KB, patch)
2021-03-12 01:14 UTC, Andrew Bartlett 		dbagnall: review+
abartlet: ci-passed+
Details
Advisory v1 (1.99 KB, text/plain)
2021-03-12 03:22 UTC, Andrew Bartlett 		dbagnall: review+
Details
Updated advisory v2 incl. versions (2.00 KB, text/plain)
2021-03-12 08:56 UTC, Karolin Seeger 		dbagnall: review+
abartlet: review+
Details
Show Obsolete (2) 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 Douglas Bagnall 2021-03-05 09:50:10 UTC 
Created attachment 16500 [details]
the immediate fix (leaving other bugs)

A string that contains multiple consecutive leading spaces can lead to a memmove() of out of bounds memory in ldb_handler_fold().

ldb_handler_fold() is used by the LDB_SYNTAX_OBJECTCLASS, LDB_SYNTAX_SAMBA_SDDL_SECURITY_DESCRIPTOR, and LDB_SYNTAX_DIRECTORY_STRING syntaxes. The last of these is probably the most exposed, though I don't have a proof of concept yet.

I found this with the help of Honggfuzz last year, and forgot all about it because CVE-2020-27840 (bug #14595 in ldb_dn_explode) was bigger. Then I found it again while trying to write tests for bug #14044 (ldb_wildcard_compare problems).

The code in question is: 
	
	/* remove trailing spaces if any */
	l = strlen(s);
	while (l > 0 && s[l - 1] == ' ') l--;
	s[l] = '\0';
	
	/* remove leading spaces if any */
	if (*s == ' ') {
		for (t = s; *s == ' '; s++) ;

		/* remove leading spaces by moving down the string */
		memmove(t, s, l);

		s = t;
	}

Putting aside that here s is an ldb_val.data and might not have a terminating '\0' (so strlen() is wrong), l is the purported length of the string. When we get to the "remove leading spaces if any" loop, we increase the gap between s and t,  then we move l bytes from s to t. But as the gap from s to the end of the string is smaller, l should be smaller too.

We start with s =  "    x", and l is 5. At the end of the loop, s points to the x, and t is where s was, and l is still 5:

    "    x.???   
     t   s    

So we move 'x', '\0', and three mystery bytes into the allocated buffer.

This syntax is used in e.g. telephone numbers, so if an attacker can put an arbitrary number of spaces at the start of a phone number field they can either crash the process or read heap bytes into their phone number.

It is not possible to write out of bounds here, so long as we assume l is the true length of the value buffer.

There are other bugs with space handling in this function that I will address in comments.
Comment 1 Douglas Bagnall 2021-03-05 10:18:14 UTC 
Created attachment 16501 [details]
additional fix for other bugs

As well as assuming strlen will work on a not necessarily terminated buffer, ldb_handler_fold() makes mistakes when collapsing multiple internal spaces down to a single space. 

When trying to add a test asserting that "1    0" would match "*1 0" (because all those spaces shrink to one, and "*X" matches "X"), I found that instead "1    0" was being converted to "1 0  0": the internal spaces were shrunk, but the terminating '\0' was not added and the ldb_val length was not adjusted.


That's because here:

	/* check middle spaces */
	while ((t = strchr(s, ' ')) != NULL) {
		for (s = t; *s == ' '; s++) ;

		if ((s - t) > 1) {
			l = strlen(s);

			/* remove all spaces but one by moving down the string */
			memmove(t + 1, s, l);
		}
	}

let's label our four spaces a, b, c, d and assume a terminated string. We have the 7 bytes {'1', a, b, c, d, '0', '\0'}. 
The strchr() points t to space a.
The for loop walks s along the spaces until it hits '0'.  
`s - t` is 4, so we set l to strlen(s) which is 1. 
Then we memmove(b, s, 1). That copies the '0' to position b.
Next strchr hits the '\0' and we're done, leaving us with "1 0  0".

The proposed fix is to walk though the whole string simply and carefully in a single loop, stepping over extra spaces as we find them. This patch is written to go on top of attachment 16500 [details].
Comment 2 Douglas Bagnall 2021-03-05 20:27:25 UTC 
Note that while I cast aspersions and fret about the possible lack of a trailing '\0', this seems unwarranted, because we have just created that value using talloc_strndup or strupper_talloc_n(via ldb_casefold or wrap_casefold, via ldb->utf8_fns.casefold). (I probably knew that when I wrote attachment 16501 [details], because it relies on the same thing).

I have created bug 14656 for the issue described in comment 1.
Comment 3 Douglas Bagnall 2021-03-09 22:14:35 UTC 
As far as I can see this is really just an "authenticated user can crash a server" bug.

Reading heap junk into the buffer is possible, but you won't see past the first zero byte (because strlen).
Comment 4 Andrew Bartlett 2021-03-10 00:46:47 UTC 
I've asked Red Hat for a CVE for this, so we can get this fixed in a security release.
Comment 5 Andrew Bartlett 2021-03-10 08:49:51 UTC 
Douglas has some tests that build on the basis in https://gitlab.com/samba-team/samba/-/merge_requests/1817 for bug 14044 so I've pushed that so we can then add the remaining tests here.
Comment 6 Douglas Bagnall 2021-03-10 21:43:54 UTC 
Created attachment 16518 [details]
patch for master with tests

The test patch will only apply on master, because it builds on recent commits.
Comment 7 Andrew Bartlett 2021-03-12 01:10:30 UTC 
Created attachment 16525 [details]
patch for master (v2)
Comment 8 Andrew Bartlett 2021-03-12 01:13:26 UTC 
Created attachment 16526 [details]
patch backported to 4.14 (v2)
Comment 9 Andrew Bartlett 2021-03-12 01:13:54 UTC 
Created attachment 16527 [details]
patch backported to 4.13 (v2)
Comment 10 Andrew Bartlett 2021-03-12 01:14:36 UTC 
Created attachment 16528 [details]
patch backported to 4.12 (v2)
Comment 11 Andrew Bartlett 2021-03-12 03:22:21 UTC 
Created attachment 16529 [details]
Advisory v1

Note:  While well reproduced at the unit test level and in a fuzzer, we are having difficultly reproducing this at the LDAP layer.  However we feel this is worth fixing regardless.
Comment 12 Andrew Bartlett 2021-03-12 03:27:24 UTC 
Karolin,

With just one CI job remaining to get a final pass pending (for 4.14) I'm assigning this to you now and attached it (presumptively) to the next security release.  Let me know if there are any issues with this please.

Andrew Bartlett
Comment 13 Andrew Bartlett 2021-03-12 03:52:56 UTC 
We think the use of 

_PUBLIC_ char *strupper_talloc_n_handle(struct smb_iconv_handle *iconv_handle,
					TALLOC_CTX *ctx, const char *src, size_t n)


in the utf8 handling invoked in all Samba users of ldb mean this is impossible to exploit on the LDAP server.

This may explain the difficultly we have had in exploiting this, technically the read is beyond the memory, but the talloc_realloc() only shrinks if there is a 1k to save.
Comment 14 Andrew Bartlett 2021-03-12 03:55:31 UTC 
(In reply to Andrew Bartlett from comment #13)
Under valgrind we could make this blow up with 10000 spaces on a local ldbsearch, so we think this is real and exploitable in at least some conditions.
Comment 15 Douglas Bagnall 2021-03-12 06:46:19 UTC 
(In reply to Andrew Bartlett from comment #14)
Confirming it works over ldap:

$ bin/ldbsearch -H ldap://$SERVER -Ujoe%Secret007 \
    "(objectClass=*$(printf '%*sr' 100000 ' '))"

search error - LDAP client internal error: NT_STATUS_CONNECTION_DISCONNECTED
Comment 16 Karolin Seeger 2021-03-12 08:56:27 UTC 
Created attachment 16531 [details]
Updated advisory v2 incl. versions

Add versions, fix typo
Comment 17 Karolin Seeger 2021-03-15 10:19:42 UTC 
Planned release date 2021-03-24.
Comment 18 Samba QA Contact 2021-03-24 08:56:30 UTC 
This bug was referenced in samba v4-14-stable (Release samba-4.14.1):

1d966cb12e7882f9cfb230195e4eff3de0f4e135
50e44877c3df8098658bd4b1fdad25b8aaadf6f3
fab6b79b7724f0b636963be528483e3e946884aa
Comment 19 Samba QA Contact 2021-03-24 08:57:52 UTC 
This bug was referenced in samba v4-13-stable (Release samba-4.13.6):

736cdfad05c283af805f0ee343163e0d59b72efa
309b18d53c105f9538e8eb0fb7bf7de52f1a5da5
e0901deb3148673aad072ca018cc9f9fb1bd5916
Comment 20 Samba QA Contact 2021-03-24 08:58:53 UTC 
This bug was referenced in samba v4-12-stable (Release samba-4.12.13):

bc967501aeb5788856f4915fec296557ae4da290
93d0e1cbc2753a3556ec35ffaca5c1f3c6a92324
719c8484bf59653bb25acf2b85dccd0a853f31c6
Comment 21 Samba QA Contact 2021-03-24 10:37:43 UTC 
This bug was referenced in samba v4-12-test:

bc967501aeb5788856f4915fec296557ae4da290
93d0e1cbc2753a3556ec35ffaca5c1f3c6a92324
719c8484bf59653bb25acf2b85dccd0a853f31c6
Comment 22 Samba QA Contact 2021-03-24 10:53:27 UTC 
This bug was referenced in samba v4-13-test:

736cdfad05c283af805f0ee343163e0d59b72efa
309b18d53c105f9538e8eb0fb7bf7de52f1a5da5
e0901deb3148673aad072ca018cc9f9fb1bd5916
Comment 23 Samba QA Contact 2021-03-24 10:56:46 UTC 
This bug was referenced in samba v4-14-test:

1d966cb12e7882f9cfb230195e4eff3de0f4e135
50e44877c3df8098658bd4b1fdad25b8aaadf6f3
fab6b79b7724f0b636963be528483e3e946884aa
Comment 24 Stefan Metzmacher 2021-03-24 11:02:51 UTC 
Note the following additional releases were made in order to
have standalone ldb releases with the fixes and let Samba depend on
them when using system libraries:

samba-4.12.14 (with ldb-2.1.5)
samba-4.13.7  (with ldb-2.2.1)
samba-4.14.2  (with ldb-2.3.0)
Comment 25 Samba QA Contact 2021-03-24 13:12:12 UTC 
This bug was referenced in samba master:

ea4bd2c437fbb5801fb82e2a038d9cdb5abea4c0
1fe8c790b2294fd10fe9c9c6254ecf2b6c00b709
Comment 26 Karolin Seeger 2021-03-25 10:01:55 UTC 
Samba 4.14.2 (4.14.1), 4.13.7 (4.13.6) and 4.12.14 (4.12.13) have been shipped to address this defect.
Closing out bug report.

Thanks!
Comment 27 Andrew Bartlett 2021-03-25 19:55:53 UTC 
Removing embargo and cc of samba-vendor, please CC individually if you wish to follow any further updates.

Fixes to bug 14656 will now follow.
Comment 28 Nagendra.V.S 2021-04-01 05:34:32 UTC 
Mitre still shows this CVE as reserved 

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-20277