Home page logo
/

nmap-dev logo Nmap Development mailing list archives

Re: [PATCH] Prevent useless buffer allocs and unpredictable payload lengths
From: David Fifield <david () bamsoftware com>
Date: Tue, 30 Jun 2009 13:51:14 -0600

On Wed, Jun 24, 2009 at 09:44:39AM +0200, Luis M. wrote:
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

This is a good idea. I think you can simplify the implementation a bit.
Don't use the aux variable and just do your calculations in
o.extra_payload_length. Keep o.extra_payload_length an int; you're
already doing bounds checking so you don't need to use a smaller data
type. Be careful about indentation. The mix of spaces and tabs leads to
this with 8-space tabs:

+    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);

Either copy the tab/space mix from surrounding lines exactly, or expand
all the tabs to spaces so indentation is uniform. But if you adjust the
indentation, always do it in a separate commit from any functional
changes.

With those modifications, this is good to commit.

David Fifield

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


  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]