Re: Nmap (4.76) out-of-bounds memory access bug & patch

["ithilgore.ryu.L () gmail com" <ithilgore.ryu.l () gmail com>]

Kris Katterjohn wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/20/2008 07:33 PM, ithilgore.ryu.L () gmail com wrote:
> > Hey everyone.
> >
> > I've been looking at the nmap source code lately and found a nasty
> > out-of-bounds memory access bug due to insufficient validation of packet
> > lengths. The problem is a bit complex so I 'll start describing it step 
> > by step. Note that is applies to the latest Nmap version: 4.76. In the 
> > end I 've included a patch that eliminates the bug.
>
> Nice find!
>
> This is my validation code, but I wasn't able to do any thorough testing since
> I was in an apartment at the time (this past summer) with only one computer.
>
> I'm curious: can you trigger this bug across the internet or just on a LAN?  I
> notice you say to suppose you're on the same subnet, but don't specifically
> state (that I remember) one way or another.  I only ask because routers don't
> typically forward very obviously broken packets, and I'm wonder if they let
> packets like this slide.
>
> I briefly flipped through my TCP/IP Illustrateds looking for types of things
> routers won't forward, but to no avail yet.  I may have been discussing this
> type of thing with Brandon Enright instead of reading it somewhere, though.

Good point. Actually it depends on the implementation. I just looked at 
both TCP/IP Illustrated Vol 2 (the BSD code) and the Linux kernel 
(2.6.26) sources (/usr/src/linux-2.6.26/net/ipv4/ip_input.c : ip_rcv())
and both of them check if the whole datagram length (the one specified 
in the mbuf/sk_buf structs) is less than the one mentioned in the IP 
header (ip_len). If it is, they drop it. So normally, since a router 
does these checks before moving on to the forwarding functions, it would 
drop this kind of packets. However, that doesn't mean that every router 
does this. There might be crapy implementations out there, that don't 
bother doing these checks in order to gain efficiency or because the 
developers were lazy.


> > Of course, if we want to be more strict we could also reject any packet 
> > that has len != iplen. But the above check is enough so that no troubles 
> > are caused on our side.
>
> Here's my comment in the code:
>
> * Checking the IP total length (iplen) to see if its at least as large as the
> * number of bytes read (len) does not work because things like the Ethernet
> * CRC also get captured and are counted in len.  Therefore, after the IP total
> * length is considered reasonable, iplen is used instead of len.  readip_pcap
> * fixes the length on it's end after this is validated.
>
> My proposed fix is to do any further length tests based on len instead of
> iplen, even though we could get garbage data instead of actual packet.  This
> is of course safer than the current way, and if for some reason a person is
> generated specially crafted replies, nothing horrible will happen.

I guess this way is even safer indeed. However, I think that finding a 
generic way to calculate how much of the length that pcap returns, 
belongs to the actual IP datagram and how much belongs to things like 
the Ethernet CRC, would be more robust. I guess the problem starts when 
one considers the fact that we don't have to deal with Ethernet only but 
other data-link layer protocols as well. The whole thing would need a 
bit of research and pcap code searching.


> Can you try my attached patch (without comment change yet) and see if that
> works for you?

I tried your patch and it worked fine!

Cheers,
ithilgore





_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org