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
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
I'll triage this next week.
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.
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.
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.
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.
(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.
Created attachment 15622 [details] fix for NDR_ERR_TOKEN issue
Created attachment 15623 [details] WIP: Add restriction to number of stored tokens to reduce abuse
*** Bug 13878 has been marked as a duplicate of this bug. ***
(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.
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!
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!
(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?
(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.
(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.
(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.
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.
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.
This was CVE-2019-14908. Downgraded and un-emabargoed after further triage.
Comment on attachment 15626 [details] patch set for master Moved to https://gitlab.com/samba-team/samba/merge_requests/964
fixed in 82aff583b7f7e018ad4a1db92dc635df8e5ebe7b