Bug 13876 - NDR crash and memory allocation on large array values from untrustworthy servers
Summary: NDR crash and memory allocation on large array values from untrustworthy servers
Status: RESOLVED FIXED
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:
: 13878 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-01 21:48 UTC by Andrew Bartlett
Modified: 2020-04-17 02:17 UTC (History)
5 users (show)

See Also:


Attachments
WIP patch for master (4.29 KB, patch)
2019-11-15 05:43 UTC, Andrew Bartlett
no flags Details
fix for NDR_ERR_TOKEN issue (1.68 KB, patch)
2019-11-15 07:05 UTC, Andrew Bartlett
no flags Details
WIP: Add restriction to number of stored tokens to reduce abuse (8.93 KB, patch)
2019-11-15 19:08 UTC, Andrew Bartlett
no flags Details
patch set for master (26.91 KB, patch)
2019-11-21 05:04 UTC, Andrew Bartlett
no flags 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:48:25 UTC
Reported by Michael Hanselmann 

echo -n 'MpR8MQAAAAAAAAAAAAAAASMRH2axvfBfYlldKSkpXQTKWV0pKQA=' | \
  base64 -d | bin/ndrdump spoolss spoolss_EnumPrintProcessorDataTypes out
Endless loop in librpc/gen_ndr/ndr_spoolss.c:ndr_pull___spoolss_EnumPrintProcessorDataTypes


This is in pure PIDL-generated code so the same issue may appear elsewhere
Comment 1 hansmi 2019-04-09 19:17:57 UTC
I ended up finding more cases in spoolss, but they appear to stem from the same underlying issue. Samples confirmed against a build from commit 377d27359ccdb8f2680fda36ca388f44456590e5:

echo -n 'AAAAAQAAAAAAAAAAAAAQAACpqakAAA==' | base64 -d | bin/ndrdump spoolss spoolss_EnumForms out

Relevant backtrace:
#6  0x00007f593c9c13ce in _talloc_realloc_array (ctx=0x611000002120, ptr=0x7f592a35b860, el_size=16, count=<optimized out>, 
    name=0x7f593ca8ca80 <.str.55> "struct ndr_token") at ../../lib/talloc/talloc.c:2786
#7  0x00007f593ca63ae9 in ndr_token_store (mem_ctx=0x611000002120, list=<optimized out>, key=0x7f5930efab20, value=0) at ../../librpc/ndr/ndr.c:971
#8  0x00007f593917359b in ndr_pull___spoolss_EnumForms (ndr=<optimized out>, flags=<optimized out>, r=<optimized out>) at librpc/gen_ndr/ndr_spoolss.c:29719
#9  0x00007f59387e2913 in ndr_pull_spoolss_EnumForms (ndr=0x611000001fe0, flags=32, r=0x60e000000ea0) at ../../librpc/ndr/ndr_spoolss_buf.c:322
#10 0x000055953f8f57c1 in main (argc=640, argv=0x618000001480) at ../../librpc/tools/ndrdump.c:456

echo -n 'AAAAAQAAAAAAAAAAQAAKAABMAQAAAAAA1wAAQAA=' | base64 -d | bin/ndrdump spoolss spoolss_EnumPrinterDrivers out

Relevant backtrace:
#5  0x00007f08b67e83ce in _talloc_realloc_array (ctx=0x611000002120, ptr=0x7f089c954860, el_size=16, count=<optimized out>, 
    name=0x7f08b68b3a80 <.str.55> "struct ndr_token") at ../../lib/talloc/talloc.c:2786
#6  0x00007f08b688aae9 in ndr_token_store (mem_ctx=0x611000002120, list=<optimized out>, key=0x7f08a9b37360, value=0) at ../../librpc/ndr/ndr.c:971
#7  0x00007f08b2f5768e in ndr_pull___spoolss_EnumPrinterDrivers (ndr=<optimized out>, flags=<optimized out>, r=<optimized out>)
    at librpc/gen_ndr/ndr_spoolss.c:26890
#8  0x00007f08b26015ee in ndr_pull_spoolss_EnumPrinterDrivers (ndr=0x611000001fe0, flags=32, r=0x60f000000460) at ../../librpc/ndr/ndr_spoolss_buf.c:292
#9  0x00005654ce6767c1 in main (argc=640, argv=0x618000001480) at ../../librpc/tools/ndrdump.c:456

echo -n 'AAAAAQAAAAAAAAAAAO0JAf//////AA==' | base64 -d | bin/ndrdump spoolss spoolss_EnumPrintProcessors out

Relevant backtrace:
#5  0x00007f21ca6793ce in _talloc_realloc_array (ctx=0x611000002120, ptr=0x7f21b4500860, el_size=16, count=<optimized out>, 
    name=0x7f21ca744a80 <.str.55> "struct ndr_token") at ../../lib/talloc/talloc.c:2786
#6  0x00007f21ca71bae9 in ndr_token_store (mem_ctx=0x611000002120, list=<optimized out>, key=0x7f21b9157fe0, value=0) at ../../librpc/ndr/ndr.c:971
#7  0x00007f21c6e0479b in ndr_pull___spoolss_EnumPrintProcessors (ndr=<optimized out>, flags=<optimized out>, r=<optimized out>)
    at librpc/gen_ndr/ndr_spoolss.c:27790
#8  0x00007f21c64b32fe in ndr_pull_spoolss_EnumPrintProcessors (ndr=0x611000001fe0, flags=32, r=0x60f000000460) at ../../librpc/ndr/ndr_spoolss_buf.c:408
#9  0x0000561afc91f7c1 in main (argc=640, argv=0x618000001480) at ../../librpc/tools/ndrdump.c:456
Comment 2 Andrew Bartlett 2019-11-08 18:43:07 UTC
I'll triage this next week.
Comment 3 Andrew Bartlett 2019-11-08 19:39:17 UTC
This is a generic problem in NDR.

The issue is that if we have a array of unions, the switch value for that union is specified for each element.

If the array is of only buffers, then the attacker can force an allocation for 'count' without sending any more data, we will prepare the tokens to store the switch values without checking first if we have been sent that many array elements.

As the whole array has the same switch value, we should only store this once, or better still just pass it as a function argument.
Comment 4 Andrew Bartlett 2019-11-15 05:28:40 UTC
We still need to stop an array being allocated for data that isn't sent (eg do a realloc loop not just a single allocation up-front) but this helps a lot.
Comment 5 Andrew Bartlett 2019-11-15 05:43:52 UTC
Created attachment 15621 [details]
WIP patch for master

echo -n 'AAAAAQAAAAAAAAAAAO0JAf//////AA==' | base64 -d | bin/ndrdump spoolss spoolss_EnumPrintProcessors out --stop-on-parse-failure
pull returned NT_STATUS_NO_MEMORY
not printing because --stop-on-parse-failure

becomes

abartlet@abartlet-pc:/data/samba/SECURITY/samba-security-wip$ echo -n 'AAAAAQAAAAAAAAAAAO0JAf//////AA==' | base64 -d | bin/ndrdump spoolss spoolss_EnumPrintProcessors out --stop-on-parse-failure
pull returned NT_STATUS_INTERNAL_ERROR
not printing because --stop-on-parse-failure

We still have an issue that calling ndr_pull_spoolss_PrintProcessorInfo does not advance the offset, but that is because it has no scalar elements.  So we don't know the caller is bluffing until we get to NDR_BUFFERS parsing.  

This much may need to be restricted in the IDL, rather than in PIDL.
Comment 6 Andrew Bartlett 2019-11-15 05:52:47 UTC
There are not many arrays of unions in Samba, but there are some in spoolss.idl.

Most are 'out', but spoolss_RemoteFindFirstPrinterChangeNotifyEx() contains (eventually)  spoolss_NotifyOptionType which is likely an issue.
Comment 7 Andrew Bartlett 2019-11-15 07:04:58 UTC
(In reply to Andrew Bartlett from comment #5)
The NT_STATUS_INTERNAL_ERROR is NDR_ERR_TOKEN.  This is just how this structure will parse with a 0 switch, due to non-security behaviour in PIDL.

However it does show that the same issue (allocation of multiple ndr_token structures) will happen for the relative pointers as well.
Comment 8 Andrew Bartlett 2019-11-15 07:05:28 UTC
Created attachment 15622 [details]
fix for NDR_ERR_TOKEN issue
Comment 9 Andrew Bartlett 2019-11-15 19:08:02 UTC
Created attachment 15623 [details]
WIP: Add restriction to number of stored tokens to reduce abuse
Comment 10 Andrew Bartlett 2019-11-17 23:06:30 UTC
*** Bug 13878 has been marked as a duplicate of this bug. ***
Comment 11 Andrew Bartlett 2019-11-17 23:16:50 UTC
(In reply to Andrew Bartlett from comment #6)
Bug 13878 shows that arrays of unions using foo[bar] are also an issue.  A quick grep show that out blocks in rap.idl are also using this, as well as more in out blocks in spoolss.
Comment 12 Andrew Bartlett 2019-11-21 05:04:55 UTC
Created attachment 15626 [details]
patch set for master

Attached is my patch set for master.  Would anyone like to take on preparing a security release for this or should we just merge into master?

As far as I can tell, the worst that can happen is memory consumption or a NULL pointer de-reference in the spoolss server (eg smbd child) or the long-lived spoolss deamon if that is in use.

A server could also do the same to a client.

I've tried to reproduce the only server-side case but my raw DCE/RPC foo isn't good enough yet. 

Thanks!
Comment 13 Andrew Bartlett 2019-11-21 08:59:14 UTC
Huzaifa,

Another for your evaluation: do you think this is worth a CVE or should we just fix it?  

This one is harder, there appears to be a server-side path to a NULL pointer de-reference, but only in special configurations would it be in a long-lived (rather than per-client) process. 

We do however understand that the server could crash a client. 

To be clear, in comment #4 I mention also being able to force the server to allocate memory during the parse of the packet (in excess of the data provided).  However that isn't something we can easily fix, so I want to focus on the crash only here.  

Thankfully talloc imposes a limit of 256MB per allocation.

Thanks!
Comment 14 Huzaifa Sidhpurwala 2019-11-21 09:13:55 UTC
(In reply to Andrew Bartlett from comment #13)
You mean the server could crash the client? Again not a security flaw, unless you can execute code on the client. would also need a malicious server i assume?
Comment 15 Andrew Bartlett 2019-11-21 09:17:18 UTC
(In reply to Huzaifa Sidhpurwala from comment #14)
Yes, the examples in (eg) comment #1 are the server crashing the client.

('out' in IDL is the part of the packet from the server to the client, the client's NDR parser is the part faulting under the fuzzer here). 

I have found one path, being spoolss_RemoteFindFirstPrinterChangeNotifyEx() where a malicious client could, in theory, crash the server.  I've not finished building a reproducer for that yet however.
Comment 16 Huzaifa Sidhpurwala 2019-11-21 09:26:19 UTC
(In reply to Andrew Bartlett from comment #15)
If client can crash server than its a security flaw and an important one perhaps, but not the other way around.
Comment 17 Andrew Bartlett 2019-11-21 09:31:53 UTC
(In reply to Huzaifa Sidhpurwala from comment #16)
Thanks.  Hopefully someone has the time to pick this up and run from here.

Most of the time the DoS would be a self-DoS of the client's own process, but there exists in Samba a 'spoolssd' mode where a long-lived process is created for spoolss and that case would be more serious.
Comment 18 Andrew Bartlett 2019-11-30 19:31:49 UTC
I've examined this issue further, and the major issue (that a client can cause CPU and memory use in excess of the data supplied) by means of NDR token allocation in our parser is only true for a union that defines an empty default.

Otherwise, the client has to supply data, and in our DCE/RPC server we have limits on the stub size of 16MB.

Thankfully arrays of unions are rare on server input, and the operation that we were most worried about, spoolss_RemoteFindFirstPrinterChangeNotifyEx, has a default, but with a field that much be included.

Additionally, because that operation has no buffers, we already use

commit 7261433fe19fde19353ee42c17607cf04af47a1c
Author: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date:   Tue Mar 1 12:26:33 2016 +1300

    librpc ndr: add ndr_pull_steal_switch_value()

Which avoids the allocation of a long token list. 

I think we should, either in a maintenance or security release include:

[PATCH 1/7] libndr: Do not overwrite token list with NULL on
 allocation failure

That will cover us against a crash in our client or in some other part of the server we didn't find in this audit, while not impacting on any parse behaviour. 

We should also include:

[PATCH 2/7] ndr: Restrict size of ndr_token lists to avoid memory
 abuse by malicious clients

But with a higher cap, say 512k, as a caution. 

The proper algorithmic fixes can then be included in master.
Comment 19 Andreas Schneider 2019-12-02 15:28:49 UTC
Yes, we fork spoolssd, but this is a small memory footprint process which preforks childs, so if you kill a spoolssd child, a new one will be spawned.
Comment 20 Andrew Bartlett 2019-12-04 01:07:40 UTC
This was CVE-2019-14908.  Downgraded and un-emabargoed after further triage.
Comment 21 Andrew Bartlett 2019-12-04 04:31:45 UTC
Comment on attachment 15626 [details]
patch set for master

Moved to https://gitlab.com/samba-team/samba/merge_requests/964
Comment 22 Douglas Bagnall 2020-04-17 02:17:21 UTC
fixed in 82aff583b7f7e018ad4a1db92dc635df8e5ebe7b