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
Current thread:
- [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup Daniel Roethlisberger (Feb 08)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup David Fifield (Feb 09)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup Daniel Roethlisberger (Feb 09)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup David Fifield (Feb 11)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup Daniel Roethlisberger (Feb 13)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup David Fifield (Feb 14)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup Daniel Roethlisberger (Feb 17)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup Daniel Roethlisberger (Feb 09)
- Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup David Fifield (Feb 09)