Nmap Development mailing list archives
Re: payload file prototype
From: David Fifield <david () bamsoftware com>
Date: Fri, 29 Jan 2010 14:40:11 -0700
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/
Current thread:
- payload file prototype Jay Fink (Jan 18)
- Re: payload file prototype David Fifield (Jan 29)
- Re: payload file prototype Jay Fink (Jan 29)
- Re: payload file prototype Jay Fink (Jan 31)
- Re: payload file prototype David Fifield (Feb 01)
- Re: payload file prototype Jay Fink (Feb 01)
- Re: payload file prototype Jay Fink (Feb 04)
- Re: payload file prototype David Fifield (Jan 29)
- <Possible follow-ups>
- Re: payload file prototype David Fifield (Feb 09)
- Re: payload file prototype Jay Fink (Feb 11)
- Re: payload file prototype David Fifield (Feb 12)
- Re: payload file prototype Jay Fink (Feb 12)
- Re: payload file prototype Jay Fink (Feb 14)
- Re: payload file prototype Jay Fink (Feb 11)