"tcpwrapped" false positives

[nnposter]

I am regularly observing incorrect "tcpwrapped" results where the targeted service is simply killing the null probe connection before nmap itself does. (Most recently I have noticed it on ArubaOS Management WebUI, which is HTTPS-based and it terminates the connection after 5 seconds.)

Looking for a solution, I came across a year-old post that pinpoints the relevant nmap code and proposes an enhancement but the attached patch seems to be based on incorrect understanding of the nmap data structures.

The patch below follows the same enhancement logic (a service is considered "tcpwrapped" only if the connection is closed quickly) but the implementation is different than the original patch. In a nut shell:

1. Defines TCPWRAPPED_TIMEOUT=2000. Connections closed after this timeout expires are not considered "tcpwrapped".
2. Implements new method ServiceNFO::probe_timemsused(), which returns how long the probe connection lasted. The method is a logical complement of existing sibling probe_timemsleft(). This old method has been reimplemented as well to avoid code duplication.
3. Adds check probe_timemsused() < TCPWRAPPED_TIMEOUT in two locations.

While the patch seems working fine for me I have to admit that I am not very familiar with this area of nmap internals so the patch might not be up to snuff. I would very much appreciate feedback from more versed developers.

--- a/service_scan.cc
+++ b/service_scan.cc
@@ -218,6 +218,9 @@
   // when SSL is detected -- we redo all probes through SSL.  If freeFP, any
   // service fingerprint is freed too.
   void resetProbes(bool freefp);
+  // Number of milliseconds used so far to complete the present probe.  Timeval
+  // can omitted, it is just there as an optimization in case you have it handy.
+  int probe_timemsused(const ServiceProbe *probe, const struct timeval *now = NULL);
   // 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.
@@ -1816,9 +1819,8 @@
   probe_state = PROBESTATE_INITIAL;
 }

-
-int ServiceNFO::probe_timemsleft(const ServiceProbe *probe, const struct timeval *now) {
-  int timeused, timeleft;
+int ServiceNFO::probe_timemsused(const ServiceProbe *probe, const struct timeval *now) {
+  int timeused;

   if (now)
     timeused = TIMEVAL_MSEC_SUBTRACT(*now, currentprobe_exec_time);
@@ -1832,7 +1834,16 @@
   // probe == currentProbe(). Check that this remains the case.
   assert(probe == currentProbe());

-  timeleft = probe->totalwaitms - timeused;
+  return timeused;
+}
+
+int ServiceNFO::probe_timemsleft(const ServiceProbe *probe, const struct timeval *now) {
+
+  // Historically this function was always called with the assumption that
+  // probe == currentProbe(). Check that this remains the case.
+  assert(probe == currentProbe());
+
+  int timeleft = probe->totalwaitms - probe_timemsused(probe, now);
   return (timeleft < 0)? 0 : timeleft;
 }

@@ -2480,7 +2491,7 @@
     if (readstrlen > 0)
       svc->addToServiceFingerprint(svc->currentProbe()->getName(), readstr,
                                    readstrlen);
-    if (probe->isNullProbe() && readstrlen == 0) {
+    if (probe->isNullProbe() && readstrlen == 0 && svc->probe_timemsused(probe) < TCPWRAPPED_TIMEOUT) {
       // TODO:  Perhaps should do further verification before making this assumption
       end_svcprobe(nsp, PROBESTATE_FINISHED_TCPWRAPPED, SG, svc, nsi);
     } else {
@@ -2498,7 +2509,7 @@
                        // BSD sometimes gives it
     case ECONNABORTED:
       // Jerk hung up on us.  Probably didn't like our probe.  We treat it as with EOF above.
-      if (probe->isNullProbe()) {
+      if (probe->isNullProbe() && svc->probe_timemsused(probe) < TCPWRAPPED_TIMEOUT) {
         // TODO:  Perhaps should do further verification before making this assumption
         end_svcprobe(nsp, PROBESTATE_FINISHED_TCPWRAPPED, SG, svc, nsi);
       } else {
--- a/service_scan.h
+++ b/service_scan.h
@@ -146,6 +146,7 @@
 #define SERVICEMATCH_REGEX 1
 // #define SERVICEMATCH_STATIC 2 -- no longer supported

+#define TCPWRAPPED_TIMEOUT 2000   // connections closed after this timeout are not considered "tcpwrapped"
 /**********************  STRUCTURES  ***********************************/

 // This is returned when we find a match

[dmiller-nmap]

This issue deserves more discussion. I replied on the development mailing list here: http://seclists.org/nmap-dev/2015/q1/52

[dmiller-nmap]

I am committing this patch with changes to support the algorithm we discussed, namely preserving the possibility of marking a service tcpwrapped if it closes the connection within the longer totalwaitms without sending data for every probe attempted. Also, I chose 3000 ms as the default timeout for tcpwrapped, to be a bit more conservative. I tested with these ncat commands:

• tcpwrapped with a short timeout: ncat -k -l -v --sh-exec "sleep 2" 8888
• tcpwrapped with a longer timeout: ncat -k -l -v --sh-exec "sleep 4" 8888
• Not tcpwrapped, just listens "forever": ncat -k -l -v --sh-exec "sleep 7" 8888
• Not tcpwrapped, but delay before banner: ncat -k -l -v --sh-exec "sleep 4;echo 'SSH-2.0-'" 8888
• Not tcpwrapped, but shorter read timeout: ncat -k -l -v --sh-exec "sleep 4; read x && echo 'SSH-2.0-'" 8888

[nnposter]

FWIW, these changes did remediate the original false-positives that lead to submitting this issue. Kudos to Dan for coming up with architecturally clean way to address the problem.