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
@@ -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
@@ -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
@@ -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 */
@@ -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 {
@@ -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 {
@@ -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
@@ -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);
@@ -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];
@@ -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];
}
@@ -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) {
@@ -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
@@ -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
@@ -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
@@ -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)
@@ -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;
}
@@ -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;
}
@@ -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.
@@ -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
@@ -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
@@ -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