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
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
Depends on:
Blocks: 14656 14661
  Show dependency treegraph
Reported: 2021-03-05 09:50 UTC by Douglas Bagnall
Modified: 2021-04-01 05:34 UTC (History)
6 users (show)

See Also:

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+
patch backported to 4.14 (v2) (9.91 KB, patch)
2021-03-12 01:13 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
patch backported to 4.13 (v2) (9.91 KB, patch)
2021-03-12 01:13 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
patch backported to 4.12 (v2) (9.91 KB, patch)
2021-03-12 01:14 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
Advisory v1 (1.99 KB, text/plain)
2021-03-12 03:22 UTC, Andrew Bartlett
dbagnall: review+
Updated advisory v2 incl. versions (2.00 KB, text/plain)
2021-03-12 08:56 UTC, Karolin Seeger
dbagnall: review+
abartlet: review+

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

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):

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):

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):

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

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

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

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:

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.

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