Bug 14656 - spaces incorrectly collapsed in ldb attributes
Summary: spaces incorrectly collapsed in ldb attributes
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: CVE-2021-20277
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-05 20:12 UTC by Douglas Bagnall
Modified: 2021-04-14 02:34 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2021-03-05 20:12:30 UTC
This is the bug described in comment 1 of bug 14655, which is not a security issue in itself, but cannot easily be fixed separately. I'm putting it here to track it. When bug 14655 is public, this one can be public.

See attachment 16501 [details]

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 1 Douglas Bagnall 2021-03-24 23:05:28 UTC
https://gitlab.com/samba-team/samba/-/merge_requests/1865

Removing the embargo, which was blocked by CVE-2021-20277 (bug #14655).
Comment 2 Samba QA Contact 2021-04-07 03:17:11 UTC
This bug was referenced in samba master:

24ddc1ca9cad95673bdd8023d99867707b37085f
Comment 3 Douglas Bagnall 2021-04-14 02:34:30 UTC
Let's not backport. There is more pressing work, and people don't seem to hit often this in practice (presumably because they use a single space).