Re: Match ICMP echo reply to request in scan_engine.cc

[David Fifield <david () bamsoftware com>]

On Wed, Jul 24, 2013 at 10:41:58AM -0400, Chris Johnson wrote:
> Use this patch instead: http://git.io/nmap-cjohnson.diff (also attached)
>
> I've updated the patch to prevent interference from concurrent pings.
>
> For example, run the following commands, quickly enough so that the
> background processes run concurrently.
>
>     sudo tc qdisc add dev eth0 root netem delay 1800ms
>     sudo nmap nmap.org -sn -PE -n --privileged --max-retries 1 --min-rtt-timeout 1.5s --max-rtt-timeout 1.5s &
>     sudo nmap nmap.org -sn -PE -n --privileged --max-retries 1 --min-rtt-timeout 1.5s --max-rtt-timeout 1.5s &
>     sleep 4
>     sudo tc qdisc del dev eth0 root
>
> Both nmap command results should show latency around 1.8s (plus
> natural latency to nmap.org). Without the patch, one will have a much
> lower (bogus) latency.

Thanks for this. I was initially worried about this patch--during host
discovery we want to err on the side of receiving replies, even if they
do not exactly match the probe we sent--liveness is a more important
thing to get right than RTT. But on investigation it appears that we
already apply a similar test to replies to TCP probes.
get_ping_pcap_result has this for TCP replies:

        if (!tcp_probe_match(USI, probe, hss, tcp, &hdr.src, &hdr.dst, hdr.ipid))
          continue;

tcp_probe_match extracts the tryno and pingseq and then checks them:

  if (o.magic_port_set) {
    goodseq = seq32_decode(USI, ntohl(tcp->th_ack) - 1, &tryno, &pingseq)
              || seq32_decode(USI, ntohl(tcp->th_ack), &tryno, &pingseq)
              || seq32_decode(USI, ntohl(tcp->th_seq), &tryno, &pingseq);
  } else {
    sport_decode(USI, base_port, ntohs(tcp->th_dport), &tryno, &pingseq);
    goodseq = true;
  }

  /* Make sure that trynum and pingseq match the values in the probe. */
  if (!probe->check_tryno_pingseq(tryno, pingseq))
    return false;

Finally, UltraProbe::check_tryno_pingseq does something like what you
have in your patch:

  /* Returns true if the given tryno and pingseq match those within this
     probe. */
  bool check_tryno_pingseq(unsigned int tryno, unsigned int pingseq) const {
    return (pingseq == 0 && tryno == this->tryno) || (pingseq > 0 && pingseq == this->pingseq);
  }

I think your patch is not quite right, though.

> +          if (ntohs(ping->seq) != probe->tryno)
> +            continue;

I don't think that ICMP probes are being sent with a seq equal to tryno.
The sending code hardcodes 0 where it calls build_icmp_raw, and I always
see seq == 0 in a packet trace:

SENT (0.0359s) ICMP [192.168.1.145 > 192.168.1.30 Echo request (type=8/code=0) id=1035 seq=0] IP [ttl=57 id=57697 
iplen=28 ]
SENT (1.0369s) ICMP [192.168.1.145 > 192.168.1.30 Echo request (type=8/code=0) id=31982 seq=0] IP [ttl=58 id=8792 
iplen=28 ]

So perhaps you need to cause the seq to be encoded in the packet?
Currently I think the patch will cause Nmap never to match any
retransmitted probe.

> +  u16 icmp_ident;
> ...
> +      probe->icmp_ident = icmp_ident;

Storing the icmp_ident is a good idea for matching. I wonder if this
information would fit better in struct probespec_icmpdata, because it is
something specific to ICMP and not the more general UltraProbe?

Finally, it would be nice if your patch introduced a helper function
like tcp_probe_match to encapsulate the extraction and matching of this
information.

David Fifield
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/