https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20454

Detailed Report: https://oss-fuzz.com/testcase?key=5427967222349824

Note that this is very similar to the already public
 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=19736#c3
as that issue was mistakenly marked as fixed when a much more minor issue was addressed.

Created attachment 15889 [details]
Proposed patch for master

Currently running CI

Comment on attachment 15889 [details]
Proposed patch for master

+ * Note that this test needs to be run with ASAN enabled, as it checks a stack
+ * overflow condition.

I think this is not correct, the test now fails on all hosts, right?

Comment on attachment 15889 [details]
Proposed patch for master

Ok this patch is obsolete, am going to move tests into asn1_start_tag.
So will get reworked.

Yeah that comment is not quite correct will reword it.

Created attachment 15893 [details]
a packet for ldap_decode that should fail with the new restriction

Created attachment 15894 [details]
packet with 10,000 OR expressions (crashes Samba with restricted stack)

When samba is started under ulimit -s '120', this packet crashes the LDAP server.

prefork_child_pipe_handler: Parent 3372, Child 3408 terminated with signal 11

There is no current protection in Samba that should mean a larger packet cannot be created to crash a more realistic stack, but larger packets are difficult to create with our client tools.

Created attachment 15895 [details]
'exploit' script

Created attachment 15896 [details]
A script to use as SAMBA_VALGRIND=/tmp/ulimit-samba to run samba under a ulimit trivially

This script helps testing this issue by running 'samba' but not the rest of our selftest system under a restricted stack.

Testing combinations of stack limits and filter complexity, it seems Samba uses up to 600 bytes of stack for each or element, so for each byte of input (an OR is two bytes of ASN.1 as I understand it) we get ~300 bytes of stack uses.

So a 27k packet can use 8.1MB of stack (the default), presumably. 

I got (SIGSEGV with disconnect)

OR    stack (kb)
  450    120
 5000   1200
10000   2400

and SIGSEGV after disconnect (presumably in the talloc_free())

5000    2400
2000    1200

Just fyi, on x86-64 I see 224 bytes of stack per OR in OpenLDAP. That means our crash limit is around 37449 nested ORs with the default thread stack size that we use. I suppose we ought to impose a limit here as well.

What's your timeline for publicizing this? I can patch this pretty quickly in OpenLDAP. When will a patch for this be visible in the Samba source tree, and when will this bug report be public? I'll file a corresponding bug in OpenLDAP
and publish concurrently.

(In reply to Howard Chu from comment #13)
Thanks.  Very happy to co-ordinate.   

Our process is here: https://wiki.samba.org/index.php/Security_release_process

Release dates are selected by our Release Manager (Karolin), but it is safe to say we have a few weeks to run yet.

If 10 days notice be enough for you then you can take your cue from the announcement that will be posted here when we open this up to our vendors.  Otherwise let us know what you need and we will do our best to work with you.

As to dates and deadlines: the next oss-fuzz report to be made public by Google will be on 30 April or so, with a possible 14 day extension possible.

CVSSv3.1: AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H (7.5)

I've confirmed 389ds does not appear to be impacted, they seem to have had, since the CVS import in 2005, a parameter nsslapd-max-filter-nest-level, which may have a default of 40 (hard to tell exactly with a grep).

Windows 2019 as an AD DC resets the connection with not adverse impacts noticed if more than 500 OR statements are used.  I've got a general query in with them for this limit, but I'm going to assume there is no need to co-ordinate with Microsoft on this issue.

Created attachment 15906 [details]
Proposes patch for master v2

Currently running CI

Created attachment 15907 [details]
Proposed patch for master V3

Looking really good.

As ldapsrv_check_packet_size() will be called a *lot*, please re-write it so that it calls security_token_is_anonymous() only if the packet is larger than the anonymous limit.  

In ldapsrv_packet_check() remove the talloc_get_type(), while good safety again this is in a very hot path.

I don't really like that in the cmocka test binaries that finding the data file is done by reference to the current working directory, but I can't find a better example.

Created attachment 15910 [details]
Proposed patch for master V4

CI running now, incorporates review feedback.

Comment on attachment 15910 [details]
Proposed patch for master V4

Looks great.

Created attachment 15914 [details]
Patch for Master (v5)

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/202825

Created attachment 15915 [details]
Patch for V4.12 (v5)

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/202839

Created attachment 15916 [details]
Patch for V4.11 (v5)

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/202863

Created attachment 15917 [details]
Patch for V4.10 (v5)

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/203092

Comment on attachment 15917 [details]
Patch for V4.10 (v5)

This looks OK, but I don't see how the Python2 compatibility patch works.

The 'pass' statement is a no-op, implemented as as a syntax aid just like an extra ; in C.  The comment is repeated in the second test.  You mean self.skipTest("Can't run on Python2"), but as it is a no-op, have you instead fixed the test somehow?

If you can look at this again please that would be great. 

Thanks!

Question: Shouln't we abort in

@@ -703,6 +713,9 @@ bool asn1_end_tag(struct asn1_data *data)
 {
 	struct nesting *nesting;
 
+	if (data->depth > 0) {
+		data->depth--;
+	}

if data->depth is already 0?

Not for this bug of course, but in a later patch.

Created attachment 15918 [details]
Advisory v1 with release versions

(In reply to Andrew Bartlett from comment #28)
It looks like I've attached the wrong patch. I'll take some time this morning and check it in detail.

(In reply to Volker Lendecke from comment #29)
Yeah that makes sense, I'll do that once this lot lands.  Am also planning to write some more tests.

Created attachment 15920 [details]
Advisory v2 with release versions

Created attachment 15922 [details]
Patch for V4.10 (v6)

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/203092

Patch generated from the correct branch.

Created attachment 15923 [details]
Patch for V4.5 (v5)

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/203102

Tests not backported, manually tested that nested queries and large requests were rejected.

Assigning to Karolin for the next security release.

Planned release date Tuesday, April 28th 2020.
Opening bug report for vendors.

Samba 4.12.2, 4.11.8 and 4.10.15 have been released to address this defect.

Pushed to autobuild-master.

Removing embargo, vendors please CC yourself individually as I've removed the samba-vendor CC to avoid spamming you. 

Note a similar issue with OpenLDAP announced with 2.4.50 as ITS#9202 / CVE-2020-12243 here: 

https://lists.openldap.org/hyperkitty/list/openldap-announce@openldap.org/thread/FUOYA6YCHBXMLANBJMSO22JD2NB22WGC/

Pushed to all branches.
Closing out bug report.

Thanks!