Re: Patches for a log_output bugfix, and a --no-clobber implementation

[Dominic White <dominic () sensepost com>]

Thanks for the helpful feedback David, you made a first time post easy. Some questions, if you wouldn't mind inline.

On 14 Sep 2011, at 6:38 PM, David Fifield wrote:
> On Wed, Sep 14, 2011 at 02:00:05PM +0200, Dominic White wrote:
> > I'd like to submit 2 patches for consideration by the nmap dev team.
> > The first fixes what appears to be a minor bug in the log_open
> > function in output.{h|cc} where the append parameter is passed but
> > never used, rather the append switch is read from the options global.
>
> This was already fixed in r23967.

I'm confused as to why the fix would still pass append as a parameter, when the option is available in the NmapOps o 
structure under o.append_output. Is this an nmap or "right" way of doing things? It seems other NmapOps are read this 
way in output.cc.

> > The second set of patches provide two methods of implementing a
> > --no-clobber function. I regularly find myself overwriting scans by
> > hitting the up key in my CLI, editing the options but forgetting to
> > update the -oA argument. I know this is "human error" and the --append
> > flag technically caters for this. However, --append doesn't fit
> > cleanly, as a user usually wouldn't mean to append to that file in the
> > case of error. People who, like me, have this problem can alias "nmap"
> > to "nmap --no-clobber" in their shell environment.
>
> For this, have you considered using %D and %T in your log file names?
> Those magic sequences get replaced by the current date or time. If you
> run
>       nmap -oA log-%T
> then Nmap won't clobber files, unless you do it more than once a second.

I have, the problem is when I am being somewhat careless in the thick of a pentest. Hence the want to have it alias'ed 
in my shell, to prevent such carelessness.

> +    else if (o.no_clobber) {
> +      o.logfd[i] = fopen(filename, "wx");
> +      if (errno == EEXIST)
> +               fatal("You specified --no-clobber and the %s output file %s already exists",
> +            logtypes[i], filename);
>
> Here you should either set errno to 0 before calling fopen, or check for
> o.logfd[i] being NULL, before testing errno == EEXIST. The reason is
> that errno might contain EEXIST from some earlier unrelated call (fopen
> doesn't clear errno to 0 on success).

Good point, thank you. Patch updated and attached, this time against a more recent r26521.

Attachment: noclobber-fail-updated.patch
Description:

> What would happen if, using noclobber-changefilename.patch, you ran
> these commands in succession:
>       1. nmap --no-clobber -oN log.nmap
>       2. nmap --no-clobber -oA log
>       3. nmap --no-clobber -oA log
> Would you end up with
>       log.nmap
> belonging to the first command,
>       log.nmap.1, log.xml, and log.gnmap
> belonging to the second command, and
>       log.nmap.2, log.xml.1, and log.gnmap.1
> belonging to the third command? It might be confusing if the numbers in
> the suffix don't match.

Given your point about the use of %{D|T} above, and your good point here, I'll drop this patch from consideration.

> David Fifield

Dominic

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