Bug 13874 - ndr_pull_string_array() looping in drsblobs decode_Packages()
Summary: ndr_pull_string_array() looping in drsblobs decode_Packages()
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: PIDL and libndr (show other bugs)
Version: 4.10.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-01 21:24 UTC by Andrew Bartlett
Modified: 2019-12-04 00:37 UTC (History)
6 users (show)

See Also:


Attachments
wip patch for master (797 bytes, patch)
2019-11-06 23:18 UTC, Andrew Bartlett
no flags Details
proposed tests (5.43 KB, patch)
2019-12-02 02:56 UTC, Gary Lockyer
no flags Details
Updated tests (5.76 KB, patch)
2019-12-03 01:26 UTC, Gary Lockyer
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-04-01 21:24:39 UTC
drsblobs decode_Packages() is only called on privileged input, but the pattern that causes a crash there may happen elsewhere. 

This bug will help us track the issue until we either find no unprivileged vectors or can assign a CVE and severity. 

To reproduce:
echo 'aw==' | base64 -d | bin/ndrdump drsblobs decode_Packages in
Comment 1 Andrew Bartlett 2019-04-01 21:25:15 UTC
Reported by Michael Hanselmann
Comment 2 Andrew Bartlett 2019-04-01 22:05:34 UTC
Assigning to Jeremy as he has the best grip on this one.
Comment 3 hansmi 2019-04-09 19:08:34 UTC
Another sample confirmed on a build from commit 377d27359ccdb8f2680fda36ca388f44456590e5:

echo -n 'AAAAAq8=' | base64 -d | bin/ndrdump --ndr64 drsblobs decode_Packages in

Relevant backtrace for this particular sample:

#8  0x00007fa082e5f510 in _talloc_realloc (context=<optimized out>, ptr=0x7fa06f956860, size=212152, name=0x7fa082f319c0 <.str> "const char *")
    at ../../lib/talloc/talloc.c:2036
#9  0x00007fa082e6a3ce in _talloc_realloc_array (ctx=0x608000000280, ptr=0x7fa06f956860, el_size=8, count=<optimized out>, 
    name=0x7fa082f319c0 <.str> "const char *") at ../../lib/talloc/talloc.c:2786
#10 0x00007fa082ec5046 in ndr_pull_string_array (ndr=0x611000001fe0, ndr_flags=256, _a=0x60b0000051c0) at ../../librpc/ndr/ndr_string.c:418
#11 0x00007fa07dc9a25a in ndr_pull_package_PackagesBlob (ndr=0x611000001fe0, ndr_flags=<optimized out>, r=0x60b0000051c0)
    at librpc/gen_ndr/ndr_drsblobs.c:2144
#12 0x00007fa07dcf271b in ndr_pull_decode_Packages (ndr=0x611000001fe0, flags=<optimized out>, r=0x60b0000051c0) at librpc/gen_ndr/ndr_drsblobs.c:5621
#13 0x000056208baca7c1 in main (argc=640, argv=0x618000001480) at ../../librpc/tools/ndrdump.c:456
Comment 4 Douglas Bagnall 2019-10-30 01:22:10 UTC
I can't reporduce this on ef58222616fc3175f189417ce878d8413ba2d294 with these commands:

echo 'aw==' | base64 -d | bin/ndrdump drsblobs   package_PackagesBlob struct

echo -n 'AAAAAq8=' | base64 -d | bin/ndrdump --ndr64 drsblobs package_PackagesBlob struct

The command has changed because unused decode functions have been removed from IDL. I might have it wrong.
Comment 5 Andrew Bartlett 2019-11-06 23:18:37 UTC
Created attachment 15602 [details]
wip patch for master

I've reproduced the issue and provided a fix.
Comment 6 Andrew Bartlett 2019-11-07 04:27:40 UTC
Douglas and I suspect this might be a real-world security issue (due to memory exhaustion) in spoolss as string_array is used in some of the calls there.

If so, this may need to be included in a security release.
Comment 7 Andrew Bartlett 2019-11-08 03:10:16 UTC
I've carefully checked all the possible callers, and this is not network-accessible as a server.

There are some places (dfs_domain_referral in dfsblobs.idl, some in spoolss.idl) where a malicious server could possibly cause a client to spin and consume memory, but I don't think we can pay a CVE for those.

If we did, it would score as:

CVSS:3.1/AV:N/AC:L/PR:H/UI:R/S:U/C:N/I:N/A:L (2.4)

Therefore I plan to just fix this next week, unless there are objections.
Comment 8 Andrew Bartlett 2019-11-08 18:47:31 UTC
Huzaifa,

do you think this is worth a CVE or should we just fix it?  There are no server-side network-accessible calls that I could find that hit this.  The easiest way to see the possible problems is to grep our IDL for string_array and nstring_array.

In short, is server-initiated memory use on the client worth a CVE?
Comment 9 Andrew Bartlett 2019-11-21 05:08:22 UTC
(In reply to Andrew Bartlett from comment #8)
I really think we should just merge the patch to master.  Can anybody provide a good case for doing a CVE for this?

Thanks!
Comment 10 Huzaifa Sidhpurwala 2019-11-21 06:11:58 UTC
(In reply to Andrew Bartlett from comment #8)
Hi Andrew,

Sorry for the late reply, seems like i missed it earlier. If the vuln code is not reachable via some API, i think its worth calling it hardening and not security.
Comment 11 Andrew Bartlett 2019-11-21 06:21:27 UTC
(In reply to Huzaifa Sidhpurwala from comment #10)
It is reachable, but not server-side from an unprivileged context.

It is reachable by a malicious server on a client (DFS redirects, the spoolss printing client).

It is reachable server-side if you have access to set the raw passwords (eg very high privilege, far easier ways to do damage)
Comment 12 Huzaifa Sidhpurwala 2019-11-21 06:51:39 UTC
(In reply to Andrew Bartlett from comment #11)
You mean, if you connect to a malicious server, the sever can cause code to be executed on the client?
Comment 13 Andrew Bartlett 2019-11-21 07:38:08 UTC
(In reply to Huzaifa Sidhpurwala from comment #12)
No code execution, just CPU and memory use until the process runs out of memory.
Comment 14 Huzaifa Sidhpurwala 2019-11-21 08:32:33 UTC
(In reply to Andrew Bartlett from comment #13)
i would consider this has a hardening or even a very minor flaw. The server can cause much more harm to the client i think than this, if connected to a malicious server.
Comment 15 Andrew Bartlett 2019-11-21 08:43:04 UTC
(In reply to Huzaifa Sidhpurwala from comment #14)
Thanks, removing embargo.
Comment 16 Andrew Bartlett 2019-11-26 18:04:12 UTC
To progress this to master what we need is an expected value torture test for the un-terminated string and array looping case as well as the happy case. 

The current unit tests are too limited to use as regression tests for this change. 

Then we should just include the example in the description as an ndrdump blackbox test to be sure.
Comment 17 Gary Lockyer 2019-12-02 02:56:59 UTC
Created attachment 15659 [details]
proposed tests
Comment 18 Andrew Bartlett 2019-12-02 03:03:32 UTC
Comment on attachment 15659 [details]
proposed tests

Thanks.

If you also assert on ndr->offset then we can be sure that looping is impossible as we know the offset is advancing.
Comment 19 Gary Lockyer 2019-12-03 01:26:02 UTC
Created attachment 15661 [details]
Updated tests

Tests fail if proposed patch not applied, and pass once the patch is applied.
Comment 20 Andrew Bartlett 2019-12-03 01:28:36 UTC
Comment on attachment 15661 [details]
Updated tests

Thanks, looks good to me.  Can you make a MR with these?
Comment 21 Gary Lockyer 2019-12-03 01:47:37 UTC
Andrew do you want me to add an ndrdump black box test as well?
Comment 22 Andrew Bartlett 2019-12-03 01:53:11 UTC
A test of this would be worthwhile I think:

ndrdump drsblobs   package_PackagesBlob struct --input='aw==' --base64-input
Comment 23 Gary Lockyer 2019-12-04 00:37:54 UTC
Done MR 963

https://gitlab.com/samba-team/samba/merge_requests/963