Re: Patch development advice: better xml output support

[David Fifield <david () bamsoftware com>]

On Wed, Jan 13, 2010 at 01:19:04AM +0100, Till Maas wrote:
> I just starting writing some patches for a bette xml output support.
> Currently the XML file breaks if nmap is interrupted or killed. I just
> wrote a POC patch to run printfinaloutput() on SIGINT and SIGTERM.

Thanks, and please forgive the late reply. It's good that you are
interested in this enough to write a patch!

So far we've just been living with the broken XML output. Sometimes you
can fix up the output manually. Or you can write tools to be tolerant of
it. Our scripts that derive nmap-services from scan results work with a
partial XML file, for example.

I can think of some difficulties with just calling printfinalouput. One
is that printfinaloutput only close the nmaprun element. If the output
is in the middle of a different XML element, the output will still be
broken. I don't know if there's anywhere when Nmap keeps an element open
for more than a few milliseconds, but even assuming it doesn't, a signal
can be delivered at any time. Here's a paper with some information:

http://lcamtuf.coredump.cx/signals.txt

Another problem is that it's not safe to call output function (and many
other functions) in a signal handler. I think it would be a big change
to add correct termination signal handling everywhere in Nmap that needs
it. Every one of its processing loops would need to include a check
against a global flag set by the signal handler, and then take
appropriate action. We already have checks in most of the loops to call
the keyWasPressed function, so maybe it won't be so bad.

> But I would also like to enhance the nmap.dtd with a <interrupted> tag
> that is used instead of the <finished> tag in case the scan did not
> complete, to show this in XML. For this I want to pass an argument to
> printfinaloutput. Do you prefer to use a charstring like "finished" or
> "interrupted" or to use a int as boolean value or maybe usen enum or constant?
>
> Also to sync the other outputs with the XML output, I would change the
> "nmap done at..." line to "nmap finished at..." or "nmap interrupted
> at...". How do you like this? It would break the stdout parsing, but
> according to the documentation, this is not necessarly a bad thing.

It used to be "Nmap finished", but Fyodor changed it to "Nmap done" in
r5688 for brevity. There's no reason to sync other output formats to the
XML.

> The next enhancement to the dtd and xml output would be to add a
> parameter to <nmaprun> that indicates that nmap resumed with a certain
> host. E.g. <nmaprun ... resume_after="LAST_SCANNED"> or <nmaprun ...
> resume_with="FIRST_TO_BE_SCANNED">. What's your opinion about this?

What if a scan is resumed twice? I think better would be an inline
element showing where the scan was resumed, maybe something like our
current taskprogress element:

<taskprogress task="SYN Stealth Scan" time="1267225099" percent="5.90" remaining="32" etc="1267225131" />

> More far away is a patch to allow to resume using the XML file. I guess
> it should be work at least as good as the current way, if just the
> lastentry like
> <address addr="137.226.139.32" addrtype="ipv4" />
> is used to determine which host was scanned last. But I did not really
> look into the code to do this.

Being able to resume from XML would be a nice feature. For this, though,
I think it would be best to have a parser that can tolerate unterminated
XML, and can figure out from the partial parsing where to restart the
scan. It could write a new XML file to replace the old if necessary.

> Then another idea is to create a new tag like <nmaplog> that is the new
> most outer xml tag used in the XML file. Then appending to the XML file
> could be done as follows to ensure a valid xml file:
> given xml file:
> <?xml version="1.0" ?>
> <?xml-stylesheet href="file:///usr/share/nmap/nmap.xsl"
> type="text/xsl"?>
> <nmaplog>
> <!-- Nmap 5.00 scan initiated ..-->
> <nmaprun ...>
> ...
> </nmaprun ...>
> </nmaplog>
>
> - Now remove the last line (</nmaplog>)
> - create only a new <nmaprun ...>
> - in printfinaloutput end the log again with </nmaplog>
> I hope it is clear what I mean. The downside of this would be to break
> all current nmap XML parsers.

I appreciate the thought, but I'm sure you realize that the downside is
way too big to make this possible.

> Last but not least: A kind of related problem is, that the sourcecode
> is not indented according to the style guide in HACKING. There are
> several tabs included at some lines. Do you mind to apply a patch to
> remove them? I would then generate one before creating the other
> patches.

Code style patches are welcome. For this, it's probably best to do one
file at a time, to reduce the chance that the patch will conflict with
other development. Or, if you have an indent command that works, just
tell us what to run and we'll do it. I haven't been completely happy
with GNU indent on Nmap source code, because 1) it doesn't understand
C++ fully, and 2) it doesn't understand typedefs and so it can give
strange results with type names. There is a command-line option that can
solve the second problem, but it requires making a file that separately
enumerates all the typedefs we use (including the implicit typedefs of
C++ structs and classes).

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