Bug 14600 - infinite loop in tldap filter parsing
Summary: infinite loop in tldap filter parsing
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.12.5
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Stefan Metzmacher
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-19 00:50 UTC by Douglas Bagnall
Modified: 2023-08-08 05:36 UTC (History)
2 users (show)

See Also:


Attachments
simple fix (760 bytes, patch)
2020-12-19 23:31 UTC, Douglas Bagnall
no flags Details
the fuzz target that finds this (4.69 KB, patch)
2020-12-19 23:40 UTC, Douglas Bagnall
no flags Details
tldap push fuzzer with a parsing fix (4.88 KB, patch)
2022-05-12 03:48 UTC, Douglas Bagnall
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2020-12-19 00:50:38 UTC
Filters like "(x=\))" will loop forever at source3/lib/tldap.c:1240

	/* find terminator */
	while (*s) {
		s = strchr(s, ')');
		if (s && (*(s - 1) == '\\')) {
			continue;
		}
		break;
	}

because *s never advances past the first ')'.

I am marking as private for now, in case there is some way in which attackers can control a tldap filter string (it looks like not, so far).


According to RFCs and Technet, the correct escaping for ')' is "\29", not "\)". I have not tested what Windows accepts.
Comment 1 Douglas Bagnall 2020-12-19 23:31:52 UTC
Created attachment 16370 [details]
simple fix
Comment 2 Douglas Bagnall 2020-12-19 23:40:05 UTC
Created attachment 16371 [details]
the fuzz target that finds this

This was found using Honggfuzz and the attached fuzz target.

Honggfuzz took half a second to find this, but it has found no more after a day and a half.
Comment 3 Douglas Bagnall 2020-12-23 00:20:56 UTC
(In reply to Douglas Bagnall from comment #0)
> I am marking as private for now, in case there is some way in which attackers can control a tldap filter string (it looks like not, so far).

So my reasoning for thinking this is not a security issue is the ultimate callers of tldap_search* are:

tldap_gensec_bind_send()        with fixed filter.
tldap_fetch_rootdse_send()      with fixed filter.
torture/torture.c
idmap searches                  constructed filters from known parts
pdb_samba_dsdb_set_aliasinfo    within '#if 0', completely broken

Some of the idmap filters contain string representations of sids, but even if these are not trusted, they are escaped in the \hh form (using ldb_binary_encode).

Metze and slow: have I missed anything?
Comment 4 Andrew Bartlett 2021-03-25 20:50:35 UTC
Removing embargo on Douglas's analysis.
Comment 5 Douglas Bagnall 2022-05-12 03:48:27 UTC
Created attachment 17283 [details]
tldap push fuzzer with a parsing fix

Metze: did you have a different patch in mind?

Otherwise I'd like to open an MR for the fuzzer and simple fix.
Comment 6 Samba QA Contact 2023-08-08 05:36:06 UTC
This bug was referenced in samba master:

f0e0ff262ab720e2e0cd48aa82549ad9c5ed69ec