Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup

[David Fifield <david () bamsoftware com>]

On Fri, Feb 13, 2009 at 03:51:39PM +0100, Daniel Roethlisberger wrote:
> David Fifield <david () bamsoftware com> 2009-02-11:
> > On Mon, Feb 09, 2009 at 11:19:03PM +0100, Daniel Roethlisberger wrote:
> > > If you look at check_tryno_pingseq(), there is this condition:
> > >
> > > return (pingseq == 0 && tryno >= this->tryno) ||
> > >        (pingseq > 0  && pingseq == this->pingseq);
> > >
> > > Accepting any tryno greater than or equal to the stored tryno
> > > only makes a difference for the TCP case, and even then only if
> > > the tryno was encoded in the seq field, not the sport.
> > >
> > > Is the tryno >= this->tryno the correct behaviour?  If yes, then
> > > we should not compare the response dport against the stored
> > > sport, and instead decode the tryno from the port and use
> > > check_tryno_pingseq(), for both UDP and TCP.  If no, we should
> > > change the tryno condition to ==.  What we have now is different
> > > scan engine behaviour based on whether magic_port is set or not.
> >
> > I can explain why it's tryno >= this->tryno. Way back during the
> > massping migration I factored out some common tryno/pingseq code into
> > check_tryno_pingseq:
> > svn log -r 5297 svn://svn.insecure.org/nmap-exp/david/nmap-massping-test@5297
> > svn diff -c 5297 svn://svn.insecure.org/nmap-exp/david/nmap-massping-test@5297
> > The previous code tested (trynum < probe->tryno); I had inverted the
> > sense of the test so I inverted the condition: (trynum >= this->tryno).
> > However now that I consider it more carefully, I think the original
> > condition should have been (trynum != probe->tryno), because if we get
> > back a reply with tryno == 1 we don't want to handle it as if it came
> > from our probe with tryno == 0. That would ignore a drop.
> >
> > So I think the check_tryno_pingseq test should be changed to 
> >   return (pingseq == 0 && tryno == this->tryno) ||
> >          (pingseq > 0  && pingseq == this->pingseq);
> > Do you want to write a patch to effect that and then we'll test it?
>
> Here we go.  I cannot see any regressions using this patch, but
> please test too.

Remember I said I had learned to test even the smallest functional
change to scan_engine.cc? Here's why. I didn't have any reason to think
the patch would negatively affect performance or accuracy. I ran the
standard benchmark that I set up for the nmap-perf tests. Results are
here:

http://www.bamsoftware.com/wiki/Nmap/PerformanceNotes#tryno-equal

Take some time to understand the information in the table and what scans
were run. In particular note that the X–Y–Z triples represent
up–open–closed. The top three rows are without the patch, and the bottom
three are with.

Surprisingly, with the patch Nmap misses a lot of open and closed ports
in -F scans of random Internet hosts—between 19% and 28% fewer open and
6% and 12% fewer closed.

I don't have an explanation for it yet. Does anyone know why?

David Fifield

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