Home page logo
/

nmap-dev logo Nmap Development mailing list archives

Re: Match ICMP echo reply to request in scan_engine.cc
From: David Fifield <david () bamsoftware com>
Date: Tue, 6 Aug 2013 22:01:42 -0700

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/


  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]