16060109 Address parfait errors found in nmap 6.25
authorRich Burridge <rich.burridge@oracle.com>
Tue, 08 Jan 2013 13:38:27 -0800
changeset 1109 70ed240042b1
parent 1108 3aa5ccc8d070
child 1110 cc8ec81665a7
16060109 Address parfait errors found in nmap 6.25
components/nmap/patches/nmap-6.25-parfait.patch
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/components/nmap/patches/nmap-6.25-parfait.patch	Tue Jan 08 13:38:27 2013 -0800
@@ -0,0 +1,350 @@
+As part of the update of nmap to version 6.25, a 3PSC form was created:
+http://psarc.us.oracle.com/arc/PSARC/2012/376/nmap_6.25_3PSC_20121218.txt
+
+To answer one of the questions, a Parfait code analysis of the nmap 6.25
+code was run. The results were reported upstream:
+http://seclists.org/nmap-dev/2012/q4/412
+
+The nmap maintainers analyzed these results and responded:
+http://seclists.org/nmap-dev/2012/q4/504
+
+This is the patch they generated, with just 'nmap-6.25/' prepended to
+the filenames on the "^--- " and "+++ " diff lines.
+
+
+
+From a632df5fbeecf6271aadbd3bcc1c927977a7ba2c Mon Sep 17 00:00:00 2001
+From: David Fifield <[email protected]>
+Date: Thu, 20 Dec 2012 22:22:49 -0800
+Subject: [PATCH 1/4] Add an ncat_assert macro.
+
+This is an assert that will remain even if NDEBUG is defined.
+---
+ ncat/util.h |    7 +++++++
+ 1 file changed, 7 insertions(+)
+
+diff --git ncat/util.h ncat/util.h
+index bf9b42e..18de755 100644
+--- nmap-6.25/ncat/util.h
++++ nmap-6.25/ncat/util.h
[email protected]@ -127,6 +127,13 @@ void logdebug(const char *fmt, ...)
+      __attribute__ ((format (printf, 1, 2)));
+ 
+ /* handle errors */
++
++#define ncat_assert(expr) \
++do { \
++        if (!(expr)) \
++                bye("assertion failed: %s", #expr); \
++} while (0)
++
+ void die(char *);
+ 
+ void bye(const char *, ...)
+-- 
+1.7.10.4
+
+From cf5b5e2f5fdd4f7744e91c097d0395736e0744ab Mon Sep 17 00:00:00 2001
+From: David Fifield <[email protected]>
+Date: Thu, 20 Dec 2012 22:32:03 -0800
+Subject: [PATCH 2/4] Assert that get_fdinfo doesn't return NULL.
+
+Resolves these Parfait reports
+(http://seclists.org/nmap-dev/2012/q4/412).
+
+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'.
+---
+ ncat/ncat_core.c   |    1 +
+ ncat/ncat_listen.c |    6 ++++--
+ 2 files changed, 5 insertions(+), 2 deletions(-)
+
+diff --git ncat/ncat_core.c ncat/ncat_core.c
+index e6cb16c..42cf389 100644
+--- nmap-6.25/ncat/ncat_core.c
++++ nmap-6.25/ncat/ncat_core.c
[email protected]@ -364,6 +364,7 @@ int ncat_broadcast(fd_set *fds, const fd_list_t *fdlist, const char *msg, size_t
+             continue;
+ 
+         fdn = get_fdinfo(fdlist, i);
++        ncat_assert(fdn != NULL);
+         if (blocking_fdinfo_send(fdn, msg, size) <= 0) {
+             if (o.debug > 1)
+                 logdebug("Error sending to fd %d: %s.\n", i, socket_strerror(socket_errno()));
+diff --git ncat/ncat_listen.c ncat/ncat_listen.c
+index 5a0b502..3e0a104 100644
+--- nmap-6.25/ncat/ncat_listen.c
++++ nmap-6.25/ncat/ncat_listen.c
[email protected]@ -293,6 +293,7 @@ static int ncat_listen_stream(int proto)
+                 FD_CLR(i, &master_readfds);
+                 FD_CLR(i, &master_writefds);
+                 fdi = get_fdinfo(&client_fdlist, i);
++                ncat_assert(fdi != NULL);
+                 switch (ssl_handshake(fdi)) {
+                 case NCAT_SSL_HANDSHAKE_COMPLETED:
+                     /* Clear from sslpending_fds once ssl is established */
[email protected]@ -535,7 +536,7 @@ int read_socket(int recv_fd)
+     int nbytes, pending;
+ 
+     fdn = get_fdinfo(&client_fdlist, recv_fd);
+-    assert(fdn != NULL);
++    ncat_assert(fdn != NULL);
+ 
+     nbytes = 0;
+     do {
[email protected]@ -838,7 +839,7 @@ static void read_and_broadcast(int recv_fd)
+     int pending;
+ 
+     fdn = get_fdinfo(&client_fdlist, recv_fd);
+-    assert(fdn);
++    ncat_assert(fdn != NULL);
+ 
+     /* Loop while ncat_recv indicates data is pending. */
+     do {
[email protected]@ -943,6 +944,7 @@ static void shutdown_sockets(int how)
+             continue;
+ 
+         fdn = get_fdinfo(&broadcast_fdlist, i);
++        ncat_assert(fdn != NULL);
+         shutdown(fdn->fd, how);
+     }
+ }
+-- 
+1.7.10.4
+
+From b7b822c8a0f4810d19d5b061c6fbaac0a2ad5723 Mon Sep 17 00:00:00 2001
+From: David Fifield <[email protected]>
+Date: Fri, 21 Dec 2012 12:22:30 -0800
+Subject: [PATCH 3/4] Make PortList::mapPort return void.
+
+Contrary to the doc comment, this function could never return false but
+only cause a fatal error.
+
+Resolves these Parfait reports
+(http://seclists.org/nmap-dev/2012/q4/412).
+
+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)'.
+---
+ portlist.cc |   17 +++++------------
+ portlist.h  |    2 +-
+ 2 files changed, 6 insertions(+), 13 deletions(-)
+
+diff --git portlist.cc portlist.cc
+index 0c8af80..cd40d04 100644
+--- nmap-6.25/portlist.cc
++++ nmap-6.25/portlist.cc
[email protected]@ -629,8 +629,8 @@ Port *PortList::nextPort(const Port *cur, Port *next,
+ }
+ 
+ /* Convert portno and protocol into the internal indices used to index
+-   port_list. Returns false on error, true otherwise. */
+-bool PortList::mapPort(u16 *portno, u8 *protocol) const {
++   port_list. */
++void PortList::mapPort(u16 *portno, u8 *protocol) const {
+   int mapped_portno, mapped_protocol;
+ 
+   mapped_protocol = INPROTO2PORTLISTPROTO(*protocol);
[email protected]@ -638,7 +638,6 @@ bool PortList::mapPort(u16 *portno, u8 *protocol) const {
+   if (*protocol == IPPROTO_IP)
+     assert(*portno < 256);
+   if(port_map[mapped_protocol]==NULL || port_list[mapped_protocol]==NULL) {
+-    assert(0);
+     fatal("%s(%i,%i): you're trying to access uninitialized protocol", __func__, *portno, *protocol);
+   }
+   mapped_portno = port_map[mapped_protocol][*portno];
[email protected]@ -648,14 +647,10 @@ bool PortList::mapPort(u16 *portno, u8 *protocol) const {
+ 
+   *portno = mapped_portno;
+   *protocol = mapped_protocol;
+-
+-  return true;
+ }
+ 
+ const Port *PortList::lookupPort(u16 portno, u8 protocol) const {
+-  if (!mapPort(&portno, &protocol))
+-    return NULL;
+-
++  mapPort(&portno, &protocol);
+   return port_list[protocol][portno];
+ }
+ 
[email protected]@ -667,8 +662,7 @@ Port *PortList::createPort(u16 portno, u8 protocol) {
+ 
+   mapped_portno = portno;
+   mapped_protocol = protocol;
+-  if (!mapPort(&mapped_portno, &mapped_protocol))
+-    return NULL;
++  mapPort(&mapped_portno, &mapped_protocol);
+ 
+   p = port_list[mapped_protocol][mapped_portno];
+   if (p == NULL) {
[email protected]@ -688,8 +682,7 @@ int PortList::forgetPort(u16 portno, u8 protocol) {
+ 
+   log_write(LOG_PLAIN, "Removed %d\n", portno);
+ 
+-  if (!mapPort(&portno, &protocol))
+-    return -1;
++  mapPort(&portno, &protocol);
+ 
+   answer = port_list[protocol][portno];
+   if (answer == NULL)
+diff --git portlist.h portlist.h
+index 8eaee1c..d7faf71 100644
+--- nmap-6.25/portlist.h
++++ nmap-6.25/portlist.h
[email protected]@ -290,7 +290,7 @@ class PortList {
+   bool hasOpenPorts() const;
+ 
+  private:
+-  bool mapPort(u16 *portno, u8 *protocol) const;
++  void mapPort(u16 *portno, u8 *protocol) const;
+   /* Get Port structure from PortList structure.*/
+   const Port *lookupPort(u16 portno, u8 protocol) const;
+   Port *createPort(u16 portno, u8 protocol);
+-- 
+1.7.10.4
+
+From 536eb67f24f9f101cd4aa1c82510ebbe8086923d Mon Sep 17 00:00:00 2001
+From: David Fifield <[email protected]>
+Date: Fri, 21 Dec 2012 13:09:56 -0800
+Subject: [PATCH 4/4] Make ServiceNFO::currentprobe_timemsleft take a probe
+ argument.
+
+It seems that this function was usually called after having called
+currentProbe outside the call to currentprobe_timemsleft, with the call
+to currentProbe inside the function having the same result. This is a
+bit tenuous, so make the probe we're talking about explicit.
+
+Resolves these Parfait reports
+(http://seclists.org/nmap-dev/2012/q4/412).
+
+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()'.
+---
+ service_scan.cc |   25 ++++++++++++++-----------
+ 1 file changed, 14 insertions(+), 11 deletions(-)
+
+diff --git service_scan.cc service_scan.cc
+index b800665..9428a9a 100644
+--- nmap-6.25/service_scan.cc
++++ nmap-6.25/service_scan.cc
[email protected]@ -191,7 +191,7 @@ public:
+   // Number of milliseconds left to complete the present probe, or 0 if
+   // the probe is already expired.  Timeval can omitted, it is just there 
+   // as an optimization in case you have it handy.
+-  int currentprobe_timemsleft(const struct timeval *now = NULL);
++  int probe_timemsleft(const ServiceProbe *probe, const struct timeval *now = NULL);
+   enum serviceprobestate probe_state; // defined in portlist.h
+   nsock_iod niod; // The IO Descriptor being used in this probe (or NULL)
+   u16 portno; // in host byte order
[email protected]@ -1799,7 +1799,7 @@ void ServiceNFO::resetProbes(bool freefp) {
+ }
+ 
+ 
+-int ServiceNFO::currentprobe_timemsleft(const struct timeval *now) {
++int ServiceNFO::probe_timemsleft(const ServiceProbe *probe, const struct timeval *now) {
+   int timeused, timeleft;
+ 
+   if (now)
[email protected]@ -1810,7 +1810,11 @@ int ServiceNFO::currentprobe_timemsleft(const struct timeval *now) {
+     timeused = TIMEVAL_MSEC_SUBTRACT(tv, currentprobe_exec_time);
+   }
+ 
+-  timeleft = currentProbe()->totalwaitms - timeused;
++  // Historically this function was always called with the assumption that
++  // probe == currentProbe(). Check that this remains the case.
++  assert(probe == currentProbe());
++
++  timeleft = probe->totalwaitms - timeused;
+   return (timeleft < 0)? 0 : timeleft;
+ }
+ 
[email protected]@ -1941,7 +1945,7 @@ static void adjustPortStateIfNecessary(ServiceNFO *svc) {
+     probestring = probe->getProbeString(&probestringlen);
+     assert(probestringlen > 0);
+     // Now we write the string to the IOD
+-    nsock_write(nsp, nsi, servicescan_write_handler, svc->currentprobe_timemsleft(), svc,
++    nsock_write(nsp, nsi, servicescan_write_handler, svc->probe_timemsleft(probe), svc,
+ 		(const char *) probestring, probestringlen);
+     return 0;
+   }
[email protected]@ -1967,7 +1971,7 @@ static void startNextProbe(nsock_pool nsp, nsock_iod nsi, ServiceGroup *SG,
+       svc->currentprobe_exec_time = *nsock_gettimeofday();
+       send_probe_text(nsp, nsi, svc, probe);
+       nsock_read(nsp, nsi, servicescan_read_handler, 
+-		 svc->currentprobe_timemsleft(nsock_gettimeofday()), svc);
++		 svc->probe_timemsleft(probe, nsock_gettimeofday()), svc);
+     } else {
+       // Should only happen if someone has a highly perverse nmap-service-probes
+       // file.  Null scan should generally never be the only probe.
[email protected]@ -2016,7 +2020,7 @@ static void startNextProbe(nsock_pool nsp, nsock_iod nsi, ServiceGroup *SG,
+ 	send_probe_text(nsp, nsi, svc, probe);
+ 	// Now let us read any results
+ 	nsock_read(nsp, nsi, servicescan_read_handler, 
+-		   svc->currentprobe_timemsleft(nsock_gettimeofday()), svc);
++		   svc->probe_timemsleft(probe, nsock_gettimeofday()), svc);
+       }
+     } else {
+       // No more probes remaining!  Failed to match
[email protected]@ -2261,7 +2265,7 @@ static void servicescan_connect_handler(nsock_pool nsp, nsock_event nse, void *m
+     svc->currentprobe_exec_time = *nsock_gettimeofday();
+     send_probe_text(nsp, nsi, svc, probe);
+     // Now let us read any results
+-    nsock_read(nsp, nsi, servicescan_read_handler, svc->currentprobe_timemsleft(nsock_gettimeofday()), svc);
++    nsock_read(nsp, nsi, servicescan_read_handler, svc->probe_timemsleft(probe, nsock_gettimeofday()), svc);
+   } else if (status == NSE_STATUS_TIMEOUT || status == NSE_STATUS_ERROR) {
+       // This is not good.  The connect() really shouldn't generally
+       // be timing out like that.  We'll mark this svc as incomplete
[email protected]@ -2421,13 +2425,12 @@ static void servicescan_read_handler(nsock_pool nsp, nsock_event nse, void *myda
+       // to timeout.  For now I'll limit it to 4096 bytes just to
+       // avoid reading megs from services like chargen.  But better
+       // approach is needed.
+-      if (svc->currentprobe_timemsleft() > 0 && readstrlen < 4096) { 
+-	nsock_read(nsp, nsi, servicescan_read_handler, svc->currentprobe_timemsleft(), svc);
++      if (svc->probe_timemsleft(probe) > 0 && readstrlen < 4096) { 
++	nsock_read(nsp, nsi, servicescan_read_handler, svc->probe_timemsleft(probe), svc);
+       } else {
+ 	// Failed -- lets go to the next probe.
+ 	if (readstrlen > 0)
+-	  svc->addToServiceFingerprint(svc->currentProbe()->getName(), readstr, 
+-				       readstrlen);
++	  svc->addToServiceFingerprint(probe->getName(), readstr, readstrlen);
+ 	startNextProbe(nsp, nsi, SG, svc, false);
+       }
+     }
+-- 
+1.7.10.4
+