Re: payload file prototype

[David Fifield <david () bamsoftware com>]

On Mon, Jan 18, 2010 at 06:29:47PM -0500, Jay Fink wrote:
> Attached is a prototype for moving payloads into an external file.
> Using it is easy:
>
>   tar xjf prototype-nmap-payload.tar.bz2
>   cd prototype-nmap-payload/
>   make
>   ./payload <udp|tcp> <port>
>
> There are no tcp ones but feel free to fake some up.

I don't think we're ever going to have TCP data in this file. There's no
need for this kind of thing with TCP. I think of having the "udp"
keyword as only leaving the door open for some future protocols we may
wish to support.

> This is a draft I wrote independent of the source tree to more or less
> whack the module into shape before integrating it. At the end of the
> day I want the module to:
> - set or return the payload
> - set or return a source port if there is a preference

I want to thank you for taking the lead on this. I tried it and it works
as advertised. There are some changes that would make it better.

First, the getpayload function should not be doing any file I/O.
Instead, parse the whole file in advance and then retrieve values from a
data structure you build. Take the example in service_scan.cc. There's a
function parse_nmap_service_probe_file that stores the service probes in
a global AllProbes structure.

I'm thinking of something like an std::map mapping (proto, port) pairs
to structs like
        struct Payload {
                char *data;
                ssize_t len;
                int sourceport;
        };

Instead of using a custom matchport function, you'll want to make use of
something like ServiceProbe::setPortVector, which supports - ranges in
addition to comma separation.

There are some memory problems in the parsing, as you suspected. First,
it's almost never safe to read untrusted strings with sscanf without
specifying a field width, like this:
        char str[256];
        sscanf(buffer, "%256s", str);
You also need to check the return value of sscanf to make sure something
was actually read.

I ran
        valgrind ./payload udp 53
and got some errors:
==9066== Conditional jump or move depends on uninitialised value(s)
==9066==    at 0x4026CD3: strcmp (mc_replace_strmem.c:412)
==9066==    by 0x80488AA: getpayload (in /home/david/prototype-nmap-payload/payload)
==9066==    by 0x8048AC7: main (in /home/david/prototype-nmap-payload/payload)
==9066==
==9066== Conditional jump or move depends on uninitialised value(s)
==9066==    at 0x4026CFF: strcmp (mc_replace_strmem.c:412)
==9066==    by 0x80488AA: getpayload (in /home/david/prototype-nmap-payload/payload)
==9066==    by 0x8048AC7: main (in /home/david/prototype-nmap-payload/payload)
==9066==
==9066== Conditional jump or move depends on uninitialised value(s)
==9066==    at 0x80488AD: getpayload (in /home/david/prototype-nmap-payload/payload)
==9066==    by 0x8048AC7: main (in /home/david/prototype-nmap-payload/payload)
==9066==
==9066== Conditional jump or move depends on uninitialised value(s)
==9066==    at 0x402684D: strncat (mc_replace_strmem.c:202)
==9066==    by 0x80489E7: getpayload (in /home/david/prototype-nmap-payload/payload)
==9066==    by 0x8048AC7: main (in /home/david/prototype-nmap-payload/payload)
==9066==

I don't like the way field_proto is used to hold both a protocol name
and the "source" keyword. I think you could benefit from a parsing
algorithm that looks more like the format of the file: a loop that first
reads a protocol and port number, an inner loop that concatenates quoted
strings, then a check if the next token is "source" and handle if it is,
then continue with the outer loop. Instead of reading line by line, I
would write a get_token function that ignores comments and returns the
next discrete keyword or quoted string (with a special return value for
end of file).

> Right now what it does is:
> - set payload
> - return source port (if there is a preference) , 0 or -1

Having the function return the source port isn't a good design. Remember
we may want to have other keywords in file, and you need a way to return
those somehow too. But I want to emphasize that you don't need to
implement an API to return the source port. We don't have a way to make
use of it yet. All I want is that the file format doesn't prohibit us
from adding it in the future. The parser should parse and understand the
keyword, but it doesn't yet need a way to return the value.

Copy the get_udp_payload prototype from payload.cc and make your program
use that, unchanged. That's exactly the API I want. You can have that
function call whatever helpers you want, but the top level has to look
like that.

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