Bug 8276 - FD_SET out of bounds access crash
Summary: FD_SET out of bounds access crash
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: Nmbd (show other bugs)
Version: 3.5.9
Hardware: x86 FreeBSD
: P5 critical
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-29 11:06 UTC by Viktor Štujber
Modified: 2011-07-04 19:55 UTC (History)
3 users (show)

See Also:


Attachments
backtrace for the nmbd 3.5.9 crash (3.04 KB, text/plain)
2011-06-29 11:12 UTC, Viktor Štujber
no flags Details
samba-3.5.9-sock_array fix (340 bytes, patch)
2011-06-29 11:23 UTC, Alexander Bokovoy
metze: review-
Details
A patch for v3-6-test (1.22 KB, patch)
2011-06-30 15:34 UTC, Stefan Metzmacher
jra: review+
Details
Patch for v3-5-test (4.26 KB, patch)
2011-06-30 15:36 UTC, Stefan Metzmacher
jra: review+
Details
Patch for v3-4-test (978 bytes, patch)
2011-06-30 15:37 UTC, Stefan Metzmacher
jra: review+
Details
Patch for v3-3-test (973 bytes, patch)
2011-06-30 15:38 UTC, Stefan Metzmacher
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Viktor Štujber 2011-06-29 11:06:42 UTC
The nmbd crashes right after starting up and receiving a packet from my machine, all the time everytime. This started after upgrading from 3.5.6 to 3.5.9.

It seems that this crash was made possible by the addition of FD_SET related range checks to fix CVE-2011-0719. By preventing the code to operate on the invalid offsets, they're left uninitialized(?), triggering some other range-check bug, which was dormant until now.

0x08098708 in listen_for_packets (run_election=false) at nmbd/nmbd_packets.c:1958
1958                    if (!FD_ISSET(sock_array[i],&r_fds)) {
        i = 8
        listen_number = 12

(gdb) p *(int(*)[50])sock_array
$13 = {8, -1, 12, 10, 9, -1, 13, 11, -401273741, 0 <repeats 41 times>}
Comment 1 Viktor Štujber 2011-06-29 11:12:49 UTC
Created attachment 6644 [details]
backtrace for the nmbd 3.5.9 crash
Comment 2 Alexander Bokovoy 2011-06-29 11:23:05 UTC
Created attachment 6645 [details]
samba-3.5.9-sock_array fix

Attached is proposed bug fix.

When we fill in sock_array[], we also return its size. However, consumer of the sock_array[] is interested in number of valid entries in the array, not the size of it. The number of actually filled entries could be less than size of the array as we could have filtered out some sockets.

A fix is one-liner.
Comment 3 Viktor Štujber 2011-06-29 11:44:20 UTC
Patch seems to have fixed it - nmbd is still up, when before it crashed immediately after starting. Thank you for looking into this.
Comment 4 Stefan Metzmacher 2011-06-30 08:18:06 UTC
Comment on attachment 6645 [details]
samba-3.5.9-sock_array fix

I think this patch is wrong for 3.5. I'll upload a better one shortly.
Comment 5 Stefan Metzmacher 2011-06-30 15:34:50 UTC
Created attachment 6654 [details]
A patch for v3-6-test

As we should never remove a subnet record, I guess it's not critical
for 3.6.0
Comment 6 Stefan Metzmacher 2011-06-30 15:36:11 UTC
Created attachment 6655 [details]
Patch for v3-5-test

Can you try and see if this patch fixes your problem too?
Comment 7 Stefan Metzmacher 2011-06-30 15:37:02 UTC
Created attachment 6656 [details]
Patch for v3-4-test
Comment 8 Stefan Metzmacher 2011-06-30 15:38:42 UTC
Created attachment 6657 [details]
Patch for v3-3-test

Jeremy, as this fixes a bug introduced by a security release,
I think we should apply this to the next 3.3 security release
if there will be one.
Comment 9 Jeremy Allison 2011-06-30 17:58:52 UTC
Comment on attachment 6654 [details]
A patch for v3-6-test

Shouldn't this patch (the 3.6.0 one) also add the tests for subrec->nmb_sock != -1 and subrec->dgram_sock != -1 into the function create_listen_pollfds() in nmbd/nmbd_packets.c to prevent fd's set to -1 being added to the poll() set ?

Jeremy.
Comment 10 Jeremy Allison 2011-06-30 20:41:31 UTC
Comment on attachment 6657 [details]
Patch for v3-3-test

LGTM for 3.3.next.
Jeremy.
Comment 11 Jeremy Allison 2011-06-30 20:42:26 UTC
Comment on attachment 6656 [details]
Patch for v3-4-test

LGTM for 3.4.next.
Jeremy.
Comment 12 Jeremy Allison 2011-06-30 20:51:01 UTC
Comment on attachment 6654 [details]
A patch for v3-6-test

For 3.6.x don't we also need git commit 1053a24a87f341fcd5578db56bc8b3962e63bb98 from master ?

Metze please comment.

Jeremy.
Comment 13 Jeremy Allison 2011-06-30 20:53:05 UTC
Comment on attachment 6655 [details]
Patch for v3-5-test

LGTM for 3.5.next.
Jeremy.
Comment 14 Stefan Metzmacher 2011-07-01 06:53:09 UTC
(In reply to comment #12)
> Comment on attachment 6654 [details]
> A patch for v3-6-test
> 
> For 3.6.x don't we also need git commit
> 1053a24a87f341fcd5578db56bc8b3962e63bb98 from master ?
> 
> Metze please comment.

If you can explain me in what situation this is needed I'm
fine with it, but I'm currently not seeing the need for this
and it would confuse/mislead people which read this code.
If it's not really needed I'd like to revert that one in master...
Comment 15 Jeremy Allison 2011-07-01 15:18:48 UTC
IMHO this makes the code clearer, as it prevents fd == -1 from ever being put into the poll array. Isn't it needed after close_subnet() has been called ? I disagree it would confuse/mislead people as I am confused mislead as to why this is *not* neccessary :-).

Isn't this just standard defensive programming here ?

Jeremy.
Comment 16 Jeremy Allison 2011-07-01 15:24:44 UTC
Ah - ok - I was missing the point of removal of the
subrec struct from the linked list that the poll
code looks at inside close_subnet().

Ok, I'm fine with you reverting this.

Jeremy.
Comment 17 Jeremy Allison 2011-07-01 15:25:50 UTC
Comment on attachment 6654 [details]
A patch for v3-6-test

Ok, now I'm good with this as I understand it :-).

This is the point of code review I guess, to make sure someone else understands the logic :-).

Jeremy.
Comment 18 Jeremy Allison 2011-07-01 15:26:30 UTC
Re-assigning to Karolin for push to all branches. Karolin I'm ok with this going into 3.6.1 if this is too late for release.

Jeremy.
Comment 19 Karolin Seeger 2011-07-04 19:55:13 UTC
Pushed to all branches.
Closing out bug report.

Thanks!