Re: Parfait analysis of nmap 6.25

[David Fifield <david () bamsoftware com>]

On Mon, Dec 10, 2012 at 08:10:15AM -0800, Rich Burridge wrote:
> I'm in the process of updating the version of nmap that's in
> the Oracle Solaris O/S to 6.25. Part of this update requires
> a security audit, including running the Parfait static code analysis
> tool on the nmap code.

Thank you for the test results. I have gone through them and made code
changes for some. The changes have already been applied to trunk. I
attached a patch for the same changes against 6.25.

> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 'fdn'
>         at line 328 of components/nmap/build/amd64/ncat/ncat_core.c in function 'blocking_fdinfo_send'.
>           Function 'get_fdinfo' may return constant 'NULL' at line 615, called at line 366 in function 
> 'ncat_broadcast'.
>           Constant 'NULL' passed into function 'blocking_fdinfo_send', argument 'fdn', from call at line 367.
>           Null pointer introduced at line 615 of components/nmap/build/amd64/ncat/util.c in function 'get_fdinfo'.
>         at line 330 of components/nmap/build/amd64/ncat/ncat_core.c in function 'blocking_fdinfo_send'.
>           Function 'get_fdinfo' may return constant 'NULL' at line 615, called at line 366 in function 
> 'ncat_broadcast'.
>           Constant 'NULL' passed into function 'blocking_fdinfo_send', argument 'fdn', from call at line 367.
>           Null pointer introduced at line 615 of components/nmap/build/amd64/ncat/util.c in function 'get_fdinfo'.
> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 'fdn'
>         at line 946 of components/nmap/build/amd64/ncat/ncat_listen.c in function 'shutdown_sockets'.
>           Function 'get_fdinfo' may return constant 'NULL' at line 615, called at line 945.
>           Null pointer introduced at line 615 of components/nmap/build/amd64/ncat/util.c in function 'get_fdinfo'.

This dereference couldn't happen, I think, if other parts of the code
work correctly, but I added some additional asserts to make sure.

> Error: File Leak
>    File Descriptor Leak: Leaked File Descriptor s
>         at line 522 of components/nmap/build/amd64/ncat/ncat_proxy.c in function 'handle_connect'.
>           s initialized at line 450 with Socket
>           s leaks when socket_errno() != 0 at line 478.

This looks like a false positive to me. The socket is always closed when
the while (!socket_errno() || socket_errno() == EINTR) loop finishes. I
don't see a way for the function to return without closing.

> Error: Null pointer dereference (CWE 476)
>    Read from null pointer '((int*)&Target::v4hostip(tpreq->targ)->S_un)'
>         at line 544 of components/nmap/build/amd64/nmap_dns.cc in function 'process_result(unsigned int, char*, int, 
> unsigned short)'.
>           Function 'Target::v4hostip() const' may return constant 'NULL' at line 285, called at line 544.
>           Null pointer introduced at line 285 of components/nmap/build/amd64/Target.cc in function 
> 'Target::v4hostip() const'.
> Error: Null pointer dereference (CWE 476)
>    Read from null pointer '((int*)&Target::v4hostip((*hostI))->S_un)'
>         at line 1189 of components/nmap/build/amd64/nmap_dns.cc in function 'nmap_mass_rdns_core(Target**, int)'.
>           Function 'Target::v4hostip() const' may return constant 'NULL' at line 285, called at line 1189.
>           Null pointer introduced at line 285 of components/nmap/build/amd64/Target.cc in function 
> 'Target::v4hostip() const'.
> Error: Null pointer dereference (CWE 476)
>    Read from null pointer '((int*)&Target::v4hostip(hss->target)->S_un)'
>         at line 2112 of components/nmap/build/amd64/osscan2.cc in function 
> 'HostOsScan::send_closedudp_probe(HostOsScanStats*, int, unsigned short, unsigned short)'.
>           Function 'Target::v4hostip() const' may return constant 'NULL' at line 285, called at line 2112.
>           Null pointer introduced at line 285 of components/nmap/build/amd64/Target.cc in function 
> 'Target::v4hostip() const'.
> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 
> '((int*)&Target::v4hostip((*std::_List_iterator<HostOsScanInfo*>::operator*(&hostI))->target)->S_un)'
>         at line 3383 of components/nmap/build/amd64/osscan2.cc in function 
> 'OsScanInfo::findIncompleteHost(sockaddr_storage*)'.
>           Function 'Target::v4hostip() const' may return constant 'NULL' at line 285, called at line 3383.
>           Null pointer introduced at line 285 of components/nmap/build/amd64/Target.cc in function 
> 'Target::v4hostip() const'.

The v4hostip method returns non-NULL when the target has an IPv4
address, which is the case in all these places. It's perhaps not
immediately clear that this is the case, and the protections against a
non-IPv4 address sneaking in are perhaps not as strong as they could be
(for example, OSScan::os_scan splits up IPv4 and IPv6 targets, but I
don't know if the address family is enforced at a lower level), but I
think it's fine to leave these alone.

> Error: Buffer overrun
>    Read outside array bounds (CWE 125): In pointer dereference of ipids[(i - 1)] with index '(i - 1)'
>       Pointer size is 6 elements (of 4 bytes each), index is 29
>         at line 232 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.
>         at line 235 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of
> components/nmap/build/amd64/idle_scan.cc in function 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, 
> scan_lists const*)' with ipids = ipids.
>         at line 236 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.
>         at line 238 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.
> Error: Buffer overrun
>    Read outside array bounds (CWE 125): In pointer dereference of ipids[i] with index 'i'
>       Pointer size is 6 elements (of 4 bytes each), index is 30
>         at line 232 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.
>         at line 235 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.
>         at line 236 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.
>         at line 238 of components/nmap/build/amd64/osscan2.cc in function 'get_ipid_sequence(int, int*, int)'.
>         called at line 504 of components/nmap/build/amd64/idle_scan.cc in function 
> 'initialize_idleproxy(idle_proxy_info*, char*, in_addr const*, scan_lists const*)' with ipids = ipids.

These I believe are false positives. Parfait may think that the index
can be as big as 32 because of the check
        u16 ipid_diffs[32];
        assert(numSamples < (int) (sizeof(ipid_diffs) / 2));
but it also will be no larger than the numSamples passed as an argument,
which is smaller.

> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 'hsi'
>         at line 459 of components/nmap/build/amd64/osscan2.cc in function 'doSeqTests(OsScanInfo*, HostOsScan*)'.
>           Function 'OsScanInfo::nextIncompleteHost()' may return constant 'NULL' at line 3398, called at line 458.
>           Null pointer introduced at line 3398 in function 'OsScanInfo::nextIncompleteHost()'.
> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 'hsi'
>         at line 629 of components/nmap/build/amd64/osscan2.cc in function 'doTUITests(OsScanInfo*, HostOsScan*)'.
>           Function 'OsScanInfo::nextIncompleteHost()' may return constant 'NULL' at line 3398, called at line 628.
>           Null pointer introduced at line 3398 in function 'OsScanInfo::nextIncompleteHost()'.

These look okay to me. The list of incomplete hosts is checked to be
nonempty in both places. The code could be more clear but I think it's
safe.

> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 'port'
>         at line 344 of components/nmap/build/amd64/portlist.cc in function 'PortList::setServiceProbeResults(unsigned 
> short, int, serviceprobestate, char const*, service_tunnel_type, char const*, char const*, char const*, char const*, 
> char const*, char const*, std::vector<char const*, std::allocator<char const*> > const*, char const*)'.
>           Function 'PortList::createPort(unsigned short, unsigned char)' may return constant 'NULL' at line 671, 
> called at line 343.
>           Null pointer introduced at line 671 in function 'PortList::createPort(unsigned short, unsigned char)'.
> Error: Null pointer dereference (CWE 476)
>    Write to null pointer 'current'
>         at line 520 of components/nmap/build/amd64/portlist.cc in function 'PortList::setPortState(unsigned short, 
> unsigned char, int)'.
>           Function 'PortList::createPort(unsigned short, unsigned char)' may return constant 'NULL' at line 671, 
> called at line 518.
>           Null pointer introduced at line 671 in function 'PortList::createPort(unsigned short, unsigned char)'.
> Error: Null pointer dereference (CWE 476)
>    Write to null pointer 'answer'
>         at line 880 of components/nmap/build/amd64/portlist.cc in function 'PortList::setStateReason(unsigned short, 
> unsigned char, unsigned short, unsigned char, sockaddr_storage const*)'.
>           Function 'PortList::createPort(unsigned short, unsigned char)' may return constant 'NULL' at line 671, 
> called at line 877.
>           Null pointer introduced at line 671 in function 'PortList::createPort(unsigned short, unsigned char)'.
>         at line 885 of components/nmap/build/amd64/portlist.cc in
> function 'PortList::setStateReason(unsigned short, unsigned char, unsigned short, unsigned char, sockaddr_storage 
> const*)'.
>           Function 'PortList::createPort(unsigned short, unsigned char)' may return constant 'NULL' at line 671, 
> called at line 877.
>           Null pointer introduced at line 671 in function 'PortList::createPort(unsigned short, unsigned char)'.

It was not actually possible for PortList::createPort to return NULL,
but I made some code changes to make this more clear.

> Error: Null pointer dereference (CWE 476)
>    Read from null pointer 'ServiceNFO::currentProbe(this)'
>         at line 1813 of components/nmap/build/amd64/service_scan.cc in function 
> 'ServiceNFO::currentprobe_timemsleft(timeval const*)'.
>           Function 'ServiceNFO::currentProbe()' may return constant 'NULL' at line 1707, called at line 1813.
>           Null pointer introduced at line 1707 in function 'ServiceNFO::currentProbe()'.

I believe that at this point, there was already known to be a current
probe, so that currentProbe would not return NULL. This wasn't very
clear though, and I made some code changes to clarify it.

David Fifield

Attachment: nmap-6.25-parfait.patch
Description:

_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/