Home page logo
/

nmap-dev logo Nmap Development mailing list archives

Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup
From: David Fifield <david () bamsoftware com>
Date: Sat, 14 Feb 2009 08:43:10 -0700

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

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]
AlienVault