I don't think this is a security concern, but i have marked it private until someone knowledgeable looks. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20193 The (base64'd) culprit string is: ICAgICAgICAgICAgIExMTExMTExMS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tL/w== #0 0x557a326a1b7f in handle_name_ptrs samba/source3/libsmb/nmblib.c:163:46 #1 0x557a326a0c73 in parse_nmb_name samba/source3/libsmb/nmblib.c:241:8 #2 0x557a3269c0ce in parse_nmb samba/source3/libsmb/nmblib.c:574:12 #3 0x557a3269bb85 in parse_packet samba/source3/libsmb/nmblib.c:797:8 #4 0x557a327d47f0 in LLVMFuzzerTestOneInput samba/lib/fuzzing/fuzz_nmblib_parse_packet.c:36:6 The basic problem seems to be that *offset is not compared against length the first time through the loop, so we end up with *offset 46 into a ubuf 47 long, and we read ubuf[*offset +1] into the new *offset. Then we branch on the new *offset. BUT that branch is only to make sure new *offset is valid. So we can't make it do anything bad with the new *offset, and ubuf[length] is (at least in our case) allocated if not defined, so we can't SEGV. static bool handle_name_ptrs(unsigned char *ubuf,int *offset,int length, bool *got_pointer,int *ret) { int loop_count=0; while ((ubuf[*offset] & 0xC0) == 0xC0) { if (!*got_pointer) (*ret) += 2; (*got_pointer)=True; (*offset) = ((ubuf[*offset] & ~0xC0)<<8) | ubuf[(*offset)+1]; // <--- HERE if (loop_count++ == 10 || (*offset) < 0 || (*offset)>(length-2)) { return False; } } return True; } patch pending.
Created attachment 15743 [details] A proposed patch Could a crafted packet effectively be a switch on the undefined byte, bouncing *offset to meaningfully different locations further back in the packet? Would these results get back to the attacker? It would be a fiddly route to get the value of a byte that is not really of your choosing.
Oh I *hate* this code. I remember trying to look at it in the past, and it broke my brain :-).
Going to try and dig in and evaluate your patch tomorrow. Thanks Douglas !
(In reply to Jeremy Allison from comment #3) ping.
OK, went through this really carefully. I think (a) your patch is correct. RB+ me (but let's put it through gitlab-CI to be sure) and (b) I can't see a way to make this into a CVE. It's a one-byte read of a value possibly off the end of a talloc'ed buffer, not of the attacker's chosing. It's only *possibly* a crash (no way for an attacker to force that either), nothing more. So I'm happy for this to go into master and be back-ported to currently supported releases. Douglas, do you agree ? Cheers, Jeremy.
(In reply to Jeremy Allison from comment #5) Yes. Passed CI. https://gitlab.com/samba-team/samba/-/merge_requests/1123
Merged into master as 3bc7acc6. Removing embargo.
Created attachment 15809 [details] patch for Samba 4.12 (cherry-picked from master patch)
Created attachment 15810 [details] patch for Samba 4.11 (cherry-picked from master patch)
Created attachment 15811 [details] patch for Samba 4.10 (cherry-picked from master patch)
Comment on attachment 15809 [details] patch for Samba 4.12 (cherry-picked from master patch) LGTM.
Comment on attachment 15810 [details] patch for Samba 4.11 (cherry-picked from master patch) LGTM.
Comment on attachment 15811 [details] patch for Samba 4.10 (cherry-picked from master patch) LGTM.
Re-assigning to Karolin for inclusion in 4.12.next, 4.11.next.
(In reply to Jeremy Allison from comment #14) Pushed to autobuild-v4-{12,11,10}-test. (There will be one last 4.10 bugfix release.)
(In reply to Karolin Seeger from comment #15) Pushed to all branches. Closing out bug report. Thanks!