Bug 14970 - SIGSEGV in resolve_oids_parse_tree_need when using paged_results with "anr"
Summary: SIGSEGV in resolve_oids_parse_tree_need when using paged_results with "anr"
Status: NEW
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: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-07 17:42 UTC by Arvid Requate
Modified: 2022-03-29 18:33 UTC (History)
4 users (show)

See Also:


Attachments
gdb.log (9.85 KB, text/plain)
2022-02-07 17:42 UTC, Arvid Requate
no flags Details
0001-Fix-segfault-in-paged_results.patch (1.95 KB, patch)
2022-02-07 17:43 UTC, Arvid Requate
no flags Details
WIP patches for master (3.27 KB, patch)
2022-02-07 23:44 UTC, Stefan Metzmacher
no flags Details
0001-paged_results-Avoid-ldb_oom-if-no-attrs-are-specified.patch (1.28 KB, patch)
2022-02-08 15:38 UTC, Arvid Requate
no flags Details
gdb-2.log (22.77 KB, text/plain)
2022-02-10 18:08 UTC, Arvid Requate
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate 2022-02-07 17:42:32 UTC
Created attachment 17150 [details]
gdb.log

Bug 14952 fixed a segmentation fault in paged_results which was introduced by 4d99cab6172 while fixing CVE-2020-10730.

We found another issue in the paged_results module, which causes segmentation faults that are triggered in real life e.g. when an Administrator uses ADUC to perform wildcard searches for accounts. In that case we see LDAP search requests like this in the LDB traces:

ldbsearch -H "ldap://$(hostname -f)"  -s sub  '(&(|(&(objectCategory=person)
(objectSid=*)(!(samAccountType:1.2.840.113556.1.4.804:=3)))
(&(objectCategory=person)(!(objectSid=*)))(&(objectCategory=group)(groupType:
1.2.840.113556.1.4.804:=14)))(anr=a*))' objectClass userAccountControl name 
description -P  --controls "paged_results:0:16" ## or similar

Metze suggested debugging this with a direct ldbsearch against the sam.ldb and we found that simple paged searches like

ldbsearch -H sam.ldb -s sub '(|(foo=bar)(anr=c*))' \
          --controls "paged_results:1:1"

ldbsearch -H sam.ldb -s sub '(!(anr==Administrator))' foo \
          --controls "paged_results:1:1"

triggers undefined behavior that frequently ends up in a segmentation fault. As usual with memory corruption, the exact behavior varies on details like the exact search filter and the attributes requested.

The common thing is, that the `anr` (Ambiguous Name Resolution) is used in the search filter and that paged_results are requested and return more than a single page. In that situation, the core dump is triggered in a subsequent call to resolve_oids, which attempts to use a nested ldb_parse_tree structure that was generated by anr.c and stored by paged_results in its private_data for later reference. Unfortunately the code of paged_results attaches the required talloc memory chunk to the `req`, which seems to get freed in between requested result pages.

The attached gdb output shows the backtrace (generated with Samba 4.13.13, but the issue persists in later versions too).

I'll attach a proposal for a patch.
Comment 1 Arvid Requate 2022-02-07 17:43:59 UTC
Created attachment 17151 [details]
0001-Fix-segfault-in-paged_results.patch

The attached patch fixed the problem in my tests.
Comment 2 Stefan Metzmacher 2022-02-07 23:44:53 UTC
Created attachment 17152 [details]
WIP patches for master

Hi Arvid,

thanks for the information!

Can you try if this patch would also fix the problem for you?
Comment 3 Arvid Requate 2022-02-08 08:45:04 UTC
Yes, works and avoids the memory leak that I proposed.
Comment 4 Arvid Requate 2022-02-08 15:38:24 UTC
Created attachment 17153 [details]
0001-paged_results-Avoid-ldb_oom-if-no-attrs-are-specified.patch

The attached patch fixes one regression from your patch: In case I don't specify any particular attributes with ldbsearch, with the new patch the search aborts like this:

root@test:~/ ldbsearch  -H sam.ldb '(|(foo=bar)(anr=a*))' \
              --controls=paged_results:1:1  
search failed - ldb out of memory at ../../source4/dsdb/samdb/ldb_modules/paged_results.c:751


Using the --trace option I see:

ldb_trace_request: SEARCH
 dn: DC=ucs50domain,DC=net
 scope: UNKNOWN
 expr: (|(foo=bar)(anr=a*))
 attr: <ALL>
 control: 1.2.840.113556.1.4.319  crit:1  data:yes

So, in this case req->op.search.attrs is NULL and that should not be mistaken for a talloc oom condition. The attached patch fixed this in my test.
Comment 5 Arvid Requate 2022-02-10 18:08:07 UTC
Created attachment 17156 [details]
gdb-2.log

Unfortunately Metzes patch (Comment 2 plus mine from Comment 4) does not fix the issues completely. A colleague of mine chose a different page size of 2 and then the issue occurred again, e.g. with:

ldbsearch -H /var/lib/samba/private/sam.ldb \
 "(|(objectclass=person)(anr=Administrator))" objectClass \
 --controls paged_results:0:2

Attached you can find a gdb bt full plus some observations throughout some points in the call stack.

I checked again with my initial path proposal (which may be bad for other reasons) and it fixed the issue.
Comment 6 Joseph Sutton 2022-02-14 00:55:57 UTC
In my testing, this bug is fixed by commit 5f0590362c5c0c5ee20503a67467f9be2d50e73b:

    CVE-2021-3670 dsdb/anr: Do a copy of the potentially anr query before starting to modify it
    
    RN: Do not modify the caller-supplied memory in the anr=* handling to
    allow clear logging of the actual caller request after it has been processed.

which appears in Samba 4.14 and 4.15, but is not in 4.13.
Comment 7 Stefan Metzmacher 2022-03-28 11:41:01 UTC
(In reply to Joseph Sutton from comment #6)

Arvid, can you please check it 5f0590362c5c0c5ee20503a67467f9be2d50e73b
works for 4.13 as well? So that we can close this?
Comment 8 Arvid Requate 2022-03-29 16:28:09 UTC
Segfaults are still happening with ldbsearches like

ldbsearch \
  -H /var/lib/samba/private/sam.ldb \
  "(|(foo=bar)(anr=a))" \
  objectClass \
  --controls paged_results:0:2

The only case this patch fixed is the ldbsearch with anr and page size 1. 
With every other number of page sizes the samba process still crashes. 

Since the default page size in ADUAC uses 64, this patch won't help IRL.

I wanted to add some test cases in source4/dsdb/tests/python/vlv.py to make it reproducible but was unable to figure out where/how/when these test cases get called. If you could give a hint, I could offer adding the tests as a patch. Btw, what do you prefer, attaching patches here or creating a pull request in Gitlab?
Comment 9 Andrew Bartlett 2022-03-29 18:33:51 UTC
Patches for master should start in GitLab per https://wiki.samba.org/index.php/Contribute