[PATCH] Prevent useless buffer allocs and unpredictable payload lengths

["Luis M." <luis.mgarc () gmail com>]

Hi. While working on Nping I found a small bug in Nmap that could be
fixed quite
easily.

The bug is related to option --data-length which allows the user to include
payloads with random data in the packets that are sent by nmap.

The code in nmap_main() that parses option --data-lenght is the following:

} else if (optcmp(long_options[option_index].name, "data-length") == 0) {
    o.extra_payload_length = atoi(optarg);
    if (o.extra_payload_length < 0) {
      fatal("data-length must be greater than 0");
    } else if (o.extra_payload_length > 0) {
      o.extra_payload = (char *) safe_malloc(o.extra_payload_length);
      get_random_bytes(o.extra_payload, o.extra_payload_length);
    }
}


Vars extra_payload_length extra_payload are defined in NmapOps.h as:

- int extra_payload_length;
- char *extra_payload;


So the problem with all this is that we are allowing the user to call
nmap like
this:

sudo nmap localhost --data-length 999999999

letting nmap_main() allocate 999999999 bytes (most of them useless!) and
then
passing that value directly to build_XXX_raw() functions.

For example, calls in scan_engine.cc like:

 packet = build_udp_raw(&o.decoys[decoy], hss->target->v4hostip(),
                 o.ttl, ipid, IP_TOS_DEFAULT, false,
                 o.ipoptions, o.ipoptionslen,
                 sport, pspec->pd.udp.dport,
                 o.extra_payload, o.extra_payload_length,
                 &packetlen);

are passing o.extra_payload_length without any checks. Fortunately, in those
functions, payload length is defined as "u16 datalen". The "conversion"
performed by my compiler (gcc 4.2.4) consists of keeping the 16 less
significant
bits stripping everything else so when whe pass "12345678" (0x00BC614E),
build_udp_raw() gets 0x614E (24910). This gets worse when the supplied
value is
something like 286392319 (0x1111FFFF) becuase in that case 
build_udp_raw() gets
0xFFFF (65535), and, as we all know, no IP packet can carry payloads bigger
than 65535-20, so we get this:

nmap: scan_engine.cc:814: void UltraProbe::setIP(u8*, u32, const
probespec*): Assertion `iplen == (u32) (__extension__ ({ register
unsigned short int __v, __x = (ipv4->ip_len); if (__builtin_constant_p
(__x)) __v = ((((__x) >> 8) & 0xff) | (((__x) & 0xff) << 8)); else
__asm__ ("rorw $8, %w0" : "=r" (__v) : "0" (__x) : "cc"); __v; }))' failed.
Aborted


I attach a patch that should solve the problem by checking user-supplied
value
before allocating the buffer, and changing the type of
extra_payload_length so
we don't get surprises when converting from int to u16.

Index: nmap.cc
===================================================================
--- nmap.cc     (revision 13870)
+++ nmap.cc     (working copy)
@@ -554,7 +554,7 @@
 
 int nmap_main(int argc, char *argv[]) {
   char *p, *q;
-  int i, arg;
+  int i, aux, arg;
   long l;
   unsigned int targetno;
   FILE *inputfd = NULL, *excludefd = NULL;
@@ -900,13 +900,12 @@
        o.setVersionTrace(true);
        o.debugging++;
       } else if (optcmp(long_options[option_index].name, "data-length") == 0) {
-       o.extra_payload_length = atoi(optarg);
-       if (o.extra_payload_length < 0) {
-         fatal("data-length must be greater than 0");
-       } else if (o.extra_payload_length > 0) {
-         o.extra_payload = (char *) safe_malloc(o.extra_payload_length);
-         get_random_bytes(o.extra_payload, o.extra_payload_length);
-       }
+    aux = atoi(optarg);        
+       if (aux < 0 || aux > MAX_PAYLOAD_ALLOWED )
+         fatal("data-length must be between 0 and %d", MAX_PAYLOAD_ALLOWED);   
+    o.extra_payload_length=(u16)aux;
+    o.extra_payload = (char *) safe_malloc(o.extra_payload_length);
+       get_random_bytes(o.extra_payload, o.extra_payload_length);
       } else if (optcmp(long_options[option_index].name, "send-eth") == 0) {
        o.sendpref = PACKET_SEND_ETH_STRONG;
       } else if (optcmp(long_options[option_index].name, "send-ip") == 0) {
Index: nmap.h
===================================================================
--- nmap.h      (revision 13870)
+++ nmap.h      (working copy)
@@ -399,6 +399,10 @@
 #define MAXHOSTNAMELEN 64
 #endif
 
+/* Max payload: Worst case is IPv4 with 40bytes of options and TCP with 20
+ * bytes of options. */
+#define MAX_PAYLOAD_ALLOWED 65535-60-40
+
 #ifndef recvfrom6_t
 #  define recvfrom6_t int
 #endif
Index: NmapOps.h
===================================================================
--- NmapOps.h   (revision 13870)
+++ NmapOps.h   (working copy)
@@ -245,7 +245,7 @@
 
   int max_ips_to_scan; // Used for Random input (-iR) to specify how 
                        // many IPs to try before stopping. 0 means unlimited.
-  int extra_payload_length; /* These two are for --data-length op */
+  u16 extra_payload_length; /* These two are for --data-length op */
   char *extra_payload;
   unsigned long host_timeout;
   /* Delay between probes, in milliseconds */


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