Bug 14254 - [FUZZING] Stack-overflow in ndr_pull_drsuapi_DsaAddressListItem_V1
Summary: [FUZZING] Stack-overflow in ndr_pull_drsuapi_DsaAddressListItem_V1
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL: https://bugs.chromium.org/p/oss-fuzz/...
Depends on:
Reported: 2020-01-28 20:14 UTC by Gary Lockyer
Modified: 2020-04-17 00:54 UTC (History)
1 user (show)

See Also:

WIP patch (411.70 KB, patch)
2020-01-31 00:02 UTC, Gary Lockyer
gary: ci-passed+
Proposed patch for master (422.74 KB, patch)
2020-02-09 19:53 UTC, Gary Lockyer
gary: review? (abartlet)
gary: ci-passed+

Note You need to log in before you can comment on or make changes to this bug.
Description Gary Lockyer 2020-01-28 20:14:05 UTC
Detailed Report: https://oss-fuzz.com/testcase?key=5705660323069952

Project: samba
Fuzzing Engine: libFuzzer
Fuzz Target: fuzz_ndr_drsuapi_TYPE_OUT
Job Type: libfuzzer_asan_samba
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7ffcb4cc2ff8
Crash State:
Sanitizer: address (ASAN)

Crash Revision: https://oss-fuzz.com/revisions?job=libfuzzer_asan_samba&revision=202001050517

Reproducer Testcase: https://oss-fuzz.com/download?testcase_id=5705660323069952
Comment 1 Gary Lockyer 2020-01-28 20:15:52 UTC
This is a recursive call to:
As the code builds up a linked list of drsuapi_DsaAddressListItem_V1 elements.

This causes a stack overflow detected by ASAN, although the depth that it triggers at 253 seems a little low.
Comment 2 Gary Lockyer 2020-01-28 20:22:53 UTC
I've added a depth element to the ndr_push structure, and added a "depth" attribute to the pidl code.

- [depth(250)] drsuapi_DsaAddressListItem_V1 *next;

If an element has a depth element, the depth value is checked against the attribute value. The depth value is incremented on the call and decremented on the return.

A couple of questions.

1) Is this a reasonable approach.

2) I'm not convinced that depth is a good name.

3) What error code should be returned, currently using NDR_ERR_BUFSIZE.
   Should a new error code be added.
Comment 3 Gary Lockyer 2020-01-28 23:58:04 UTC
Summary of an off-line conversation with Andrew Bartlet.

1) rename the "depth" attribute "max_recursion"
2) rename the depth "field" on ndr_pull "recursion_depth"
3) add "global_max_recursion" to ndr_pull
   This will override the value specified for max_recusion
4) modify the fuzzer to set global_max_recursion to 128 to avoid
   stack overflow when running under ASAN
6) Check the idl for "next" and add max_recursion property
7) If possible, and maybe later. See if pidl can be altered to require
   max_recursion for recursive structures.
Comment 4 Gary Lockyer 2020-01-31 00:02:18 UTC
Created attachment 15757 [details]
WIP patch

WIP proposed patch, need to add tests for the new macros and reword the commits.
Comment 5 Gary Lockyer 2020-02-09 19:53:55 UTC
Created attachment 15777 [details]
Proposed patch for master

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/188868
Comment 6 Andrew Bartlett 2020-02-21 00:37:59 UTC
I've looked over this carefully, and thankfully none of these are parsed server-side.  Indeed most are not used, eg the drsblobs.idl structures. 

The ioctl.idl changes are only used in torture clients. 

So we can make it public.  Gary, can you do a normal MR for these?
Comment 7 Douglas Bagnall 2020-04-17 00:54:06 UTC
fixed in 23d285d34900270fe171f06f3fbad9879004d4a4

The commits refer to oss-fuzz issue 19280, which should be 19820.