Re: payload file prototype

[Jay Fink <jay.fink () gmail com>]

On Fri, Jan 29, 2010 at 4:40 PM, David Fifield <david () bamsoftware com> wrote:

> 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.

Okay
>

> 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;
>        };

Excellent, I was thinking about doing something similar but I wasn't
sure if it would be overkill for this function. Good to know!
> 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==

Thanks for pointers here! This is exactly what I was looking for!
> 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.

These points were stuck in my head - I forgot to mention them in the
original email.
I was 99.9% sure setting a string and returning a number was utterly
wrong - a structure that gets populated (as you mentioned above) is
better.

> 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.

Excellent - thanks again for the feedback and especially the
turnaround - I know your busy with the current release(s) and didn't
expect to hear anything for awhile!

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