Nmap Development mailing list archives
Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup
From: David Fifield <david () bamsoftware com>
Date: Wed, 11 Feb 2009 14:56:39 -0700
On Mon, Feb 09, 2009 at 11:19:03PM +0100, Daniel Roethlisberger wrote:
David Fifield <david () bamsoftware com> 2009-02-09:On Mon, Feb 09, 2009 at 12:11:53AM +0100, Daniel Roethlisberger wrote:- Add decoding and verification of trynum/pingseq to UDP response handling (if no magic port was set).This looks like the right thing to do too, but experience has taught me always to test even small changes to scan_engine.cc. Can you do some tests to ensure that the change doesn't affect speed or accuracy negatively?Actually, I see now that the change does not make sense, since we compare the source port anyway. There is no point in extracting something from the source port and comparing that extracted thing to stored values if we already compared the source port itself.+ /* Replace this with a call to probe_check_trynum_pingseq or similar. */ + if (o.magic_port_set) { + trynum = probe->tryno; + pingseq = probe->pingseq; + } else { + sport_decode(USI, o.magic_port, ntohs(udp->uh_dport), &trynum, &pingseq); + } + + /* Make sure that trynum and pingseq match the values in the probe. */ + if (!probe->check_tryno_pingseq(trynum, pingseq)) + continue; +There's another place in the code with "Replace this with a call...", but it doesn't set pingseq if o.magic_port_set and it's not followed by the "Make sure that..." block. If this change is beneficial, do you think the code should be the same in both places, or is there a reason for the difference?The only reason for decoding the trynum for the UDP ping probe case (starting from line 4547) is the debug message on line 4567. 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.
I now see what you mean. The reason it doesn't make a difference is because in every case before calling check_tryno_pingseq we check that probe->sport() == ntohs(tcp->th_dport); if those differed, then tryno and pingseq would differ. The exception is when o.magic_port_set--then it can be the case that probe->sport() == ntohs(tcp->th_dport) and tryno and pingseq differ. Even if o.magic_port_set, it is never the case that tryno > this->tryno, for a more subtle reason. The list of probes is traversed backwards, so those with the highest tryno are seen first. I added the assertion assert(tryno == this->tryno && pingseq == this->pingseq); to check_tryno_pingseq and I could only make it trip when I used the -g option to set the magic port and I modified the code to walk the probe list forwards.
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? 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)