Bug 14242 - [FUZZ] nmblib handle_name_ptrs reading beyond the buffer
Summary: [FUZZ] nmblib handle_name_ptrs reading beyond the buffer
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-19 02:03 UTC by Douglas Bagnall
Modified: 2020-03-19 09:31 UTC (History)
2 users (show)

See Also:


Attachments
A proposed patch (1.24 KB, patch)
2020-01-19 02:25 UTC, Douglas Bagnall
no flags Details
patch for Samba 4.12 (cherry-picked from master patch) (1.53 KB, patch)
2020-02-21 00:19 UTC, Andrew Bartlett
abartlet: review? (dbagnall)
jra: review+
Details
patch for Samba 4.11 (cherry-picked from master patch) (1.53 KB, patch)
2020-02-21 00:24 UTC, Andrew Bartlett
abartlet: review? (dbagnall)
jra: review+
Details
patch for Samba 4.10 (cherry-picked from master patch) (1.53 KB, patch)
2020-02-21 00:28 UTC, Andrew Bartlett
abartlet: review? (dbagnall)
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2020-01-19 02:03:01 UTC
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.
Comment 1 Douglas Bagnall 2020-01-19 02:25:19 UTC
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.
Comment 2 Jeremy Allison 2020-01-23 23:29:09 UTC
Oh I *hate* this code. I remember trying to look at it in the past, and it broke my brain :-).
Comment 3 Jeremy Allison 2020-01-24 01:06:30 UTC
Going to try and dig in and evaluate your patch tomorrow. Thanks Douglas !
Comment 4 Douglas Bagnall 2020-02-04 20:42:10 UTC
(In reply to Jeremy Allison from comment #3)
ping.
Comment 5 Jeremy Allison 2020-02-06 23:19:28 UTC
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.
Comment 6 Douglas Bagnall 2020-02-07 02:43:27 UTC
(In reply to Jeremy Allison from comment #5)
Yes.

Passed CI.
https://gitlab.com/samba-team/samba/-/merge_requests/1123
Comment 7 Andrew Bartlett 2020-02-21 00:18:35 UTC
Merged into master as 3bc7acc6.  Removing embargo.
Comment 8 Andrew Bartlett 2020-02-21 00:19:02 UTC
Created attachment 15809 [details]
patch for Samba 4.12 (cherry-picked from master patch)
Comment 9 Andrew Bartlett 2020-02-21 00:24:14 UTC
Created attachment 15810 [details]
patch for Samba 4.11 (cherry-picked from master patch)
Comment 10 Andrew Bartlett 2020-02-21 00:28:36 UTC
Created attachment 15811 [details]
patch for Samba 4.10 (cherry-picked from master patch)
Comment 11 Jeremy Allison 2020-03-06 19:42:16 UTC
Comment on attachment 15809 [details]
patch for Samba 4.12 (cherry-picked from master patch)

LGTM.
Comment 12 Jeremy Allison 2020-03-06 19:42:29 UTC
Comment on attachment 15810 [details]
patch for Samba 4.11 (cherry-picked from master patch)

LGTM.
Comment 13 Jeremy Allison 2020-03-06 19:42:38 UTC
Comment on attachment 15811 [details]
patch for Samba 4.10 (cherry-picked from master patch)

LGTM.
Comment 14 Jeremy Allison 2020-03-06 19:43:03 UTC
Re-assigning to Karolin for inclusion in 4.12.next, 4.11.next.
Comment 15 Karolin Seeger 2020-03-18 10:29:10 UTC
(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.)
Comment 16 Karolin Seeger 2020-03-19 09:31:28 UTC
(In reply to Karolin Seeger from comment #15)
Pushed to all branches.
Closing out bug report.

Thanks!