Re: [PATCH] Prevent useless buffer allocs and unpredictable payload lengths

[David Fifield <david () bamsoftware com>]

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