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

[Daniel Roethlisberger <daniel () roe ch>]

David Fifield <david () bamsoftware com> 2009-02-11:
> 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:
> > > > +        /* 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?

Here we go.  I cannot see any regressions using this patch, but
please test too.

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

Attachment: check_tryno_equals.diff
Description:

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