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
Reported by Michael Hanselmann
Assigning to Jeremy as he has the best grip on this one.
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
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.
Created attachment 15602 [details] wip patch for master I've reproduced the issue and provided a fix.
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.
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.
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?
(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!
(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.
(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)
(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?
(In reply to Huzaifa Sidhpurwala from comment #12) No code execution, just CPU and memory use until the process runs out of memory.
(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.
(In reply to Huzaifa Sidhpurwala from comment #14) Thanks, removing embargo.
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.
Created attachment 15659 [details] proposed tests
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.
Created attachment 15661 [details] Updated tests Tests fail if proposed patch not applied, and pass once the patch is applied.
Comment on attachment 15661 [details] Updated tests Thanks, looks good to me. Can you make a MR with these?
Andrew do you want me to add an ndrdump black box test as well?
A test of this would be worthwhile I think: ndrdump drsblobs package_PackagesBlob struct --input='aw==' --base64-input
Done MR 963 https://gitlab.com/samba-team/samba/merge_requests/963
Merged into master as d15a3797 for Samba 4.12