From 332e8aa176182061c7e53c89eb7084c3a924ced1 Mon Sep 17 00:00:00 2001 From: kris Date: Sat, 26 Jul 2008 00:25:24 +0000 Subject: [PATCH] (The last paragraph of this message has an implementation re-work suggestion) o Fixed host discovery probe matching when looking at the returned TCP data in an ICMP error message. This could lead to incorrectly discarded responses and the debugging error message: "Bogus trynum or sequence number in ICMP error message" [Kris] Fyodor was getting the error message "Got ICMP error with a TCP header that was too short" while scanning, and looked at the code to see a comment I made about requiring 12 bytes of TCP data in an ICMP error message instead of the minimum RFC requirement of 8 bytes. I made this comment and requirement because tcp_trynum_pingseq_decode() was being called on the TCP data, and was using the ACK field (which is just past the 8 byte range). However, upon further inspection, we came to the conclusion that this code was broken because examining the ACK field should only be done on a TCP response, not on our own probe (which is what we're looking at in the ICMP data). This assumes that -g is used (the only reason that the SEQ/ACK is checked since the source port number is used otherwise), but the code is also broken without it because the *_decode() function checks the destination port number rather than the source port (which should be checked since it's our own probe we're looking at). So I've removed the 12-byte requirement and pingseq checking calls, and just check that the received SEQ number matches the probe SEQ number. Should we just work with the SEQ/ACK matching when using TCP and leave the pingseq/trynum port number encoding to UDP? This means behavior won't change with the use of -g, and it should be guaranteed to be there since we'll only be looking at whole TCP headers rather than any smaller chunks. Plus, the SEQ number is already getting encoded with the pingseq/trynum info, we're just not decoding the ACK responses unless -g is used. --- CHANGELOG | 5 +++++ scan_engine.cc | 21 +-------------------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index e8501761a..3e7540864 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -52,6 +52,11 @@ o [NSE] script_scan_result structure has been changed to a class, o [NSE] The runlevel structure has been placed in the thread record structure so we no longer need to manage the runlevel explicitly on the heap. [Patrick] +o Fixed host discovery probe matching when looking at the returned TCP data in + an ICMP error message. This could lead to incorrectly discarded responses + and the debugging error message: "Bogus trynum or sequence number in ICMP + error message" [Kris] + o Nsock now supports binding to a local address and setting IPv4 options with nsi_set_localaddr() and nsi_set_ipoptions(), respectively. [Kris] diff --git a/scan_engine.cc b/scan_engine.cc index 0d13dcfe4..bd45c5d23 100644 --- a/scan_engine.cc +++ b/scan_engine.cc @@ -4197,16 +4197,6 @@ static int get_ping_pcap_result(UltraScanInfo *USI, struct timeval *stime) { error("Got ICMP error referring to TCP msg which we did not send"); continue; } - /* We need to check for a few more bytes because - * tcp_trynum_pingseq_decode() below can use th_ack (which is beyond - * the +8 bytes checked for above) - */ - requiredbytes += 4U; - if (bytes < requiredbytes) { - if (o.debugging) - error("Got ICMP error with a TCP header that was too short"); - continue; - } struct tcp_hdr *tcp = (struct tcp_hdr *) (((char *) ip2) + 4 * ip2->ip_hl); /* Now ensure this host is even in the incomplete list */ memset(&sin, 0, sizeof(sin)); @@ -4233,19 +4223,10 @@ static int get_ping_pcap_result(UltraScanInfo *USI, struct timeval *stime) { /* Ensure the connection info matches. */ if (probe->dport() != ntohs(tcp->th_dport) || probe->sport() != ntohs(tcp->th_sport) + || probe->tcpseq() != ntohl(tcp->th_seq) || hss->target->v4sourceip()->s_addr != ip->ip_dst.s_addr) continue; - goodseq = tcp_trynum_pingseq_decode(USI, tcp, &trynum, &pingseq); - if (!goodseq) { - if (o.debugging) - error("Bogus trynum or sequence number in ICMP error message"); - continue; - } - - if (!probe->check_tryno_pingseq(trynum, pingseq)) - continue; - /* If we made it this far, we found it. We don't yet know if it's going to change a host state (goodone) or not. */ break;