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

[Daniel Roethlisberger <daniel () roe ch>]

David Fifield <david () bamsoftware com> 2009-02-09:
> On Mon, Feb 09, 2009 at 12:11:53AM +0100, Daniel Roethlisberger wrote:
> > I am still failing to fully understand all the nitty-gritty
> > details of some of the code in scan_engine.cc.  There is a lot of
> > copy-pasted duplicate code, sometimes with minor differences
> > between the various appearances of the same code...
> >
> > Would the attached patch break anything?
> >
> > - Remove instances of `goodseq' which appear to be now defunct
> >   remainders of previous trynum/pingseq decoding code (cf. TCP
> >   sections of the functions).
> > - Move the declarations of `goodseq' into the only context (code
> >   block) which actually uses it.  This makes the code slightly
> >   easier to understand, since it makes it obvious that `goodseq'
> >   is only of local interest.
>
> I applied these two parts of the patch. Cleanups are always welcome.

Mental note to self: remember to separate cleanup from functional
changes when submitting patches...

> > - 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? I'm thinking of tests like
>
> nmap -F -sU scanme.nmap.org
> nmap -p1-65535 -sU localhost
> nmap -F -sU -iL up-hosts
>
> where up-hosts is a dozen hosts with responsive UDP ports,
> found with nmap -n -sP -PU -iR 500. For each test, record the
> time taken and the number of ports in each state.

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.

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.

What do you think?

-- 
Daniel Roethlisberger
http://daniel.roe.ch/

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