Re: [PATCH] TCP Idle Scan in IPv6

[Mathias Morbitzer <m.morbitzer () student ru nl>]

> Thanks for all this code. This looks like it could be a nice new
> feature.

I'm happy to hear that :) The last weeks, I've been working on an improved version of my patched, so good to know that 
there is interest. 

> Is it possible for you to store your development history online in
> some
> form, for example in a public github repository? You can fork from
> https://github.com/nmap/nmap, or follow the instructions from
> https://secwiki.org/w/Using_Git_with_Nmap.

Sure, shouldn't be an issue. I'm quite busy this week, I will give it a try next week. 

> > + if (proxy->host.af() == AF_INET6){
> > + int rc = resolve(proxy->host.HostName(), 0, &ss, &sslen, o.pf());
> > + if (rc != 0) {
> > + fatal("Could not resolve idle scan zombie host \"%s\": %s",
> > proxy->host.HostName(), gai_strerror(rc));
> > + }
> > + ping = build_tcp_raw_ipv6(proxy->host.v6sourceip(),
> > proxy->host.v6hostip(),
> > + 0x00, 0x0000, o.ttl,
> > + base_port + tries, proxy->probe_port,
> > + seq_base + (packet_send_count++ * 500) + 1, ack,
> > + 0, TH_SYN | TH_ACK, 0, 0,
> > + (u8 *) "\x02\x04\x05\xb4", 4,
> > + NULL, 0, &packetlen);
> > + send_ip_packet(proxy->rawsd, proxy->ethptr, &ss, ping, packetlen);
>
> I don't see why you resolve a host name when proxy->host.v6hostip() is
> available? You can use that value to fill ss to give to
> send_ip_packet.

I didn't know that. Perfect, that solves one of my TODOs. 

> > + if (proxy->host.af() == AF_INET6)
> > + ip = (struct ip *) readip_pcap(proxy->pd, &bytes, to_usec,
> > &rcvdtime, NULL, true);
> > + else
> > + ip = (struct ip *) readipv4_pcap(proxy->pd, &bytes, to_usec,
> > &rcvdtime, NULL, true);
>
> Is is possible to just call readip_pcap here, and filter on address
> family later on? You have to filter on address family anyway--the code
> that follow looks incorrect in that an IPv4 packet could be processed
> in
> IPv6 mode. Your condition should be
> if (o.af() == AF_INET6) {
> if (ip->ip_v != 6)
> error
> }
> not
> if (ip->ip_v == 6)

I guess that's doable. From what I remember, the original code was using readipv4_pcap, that is why I added 
readip_pcap. But I guess I can change the code so that this works with only readip_pcap. 

> > + fatal("IPv6 packet without fragmentation header received - issues
> > with the zombie?");
>
> You can't just kill the whole Nmap process for one weird packet :)
> Find
> another way to handle this case, maybe by ignoring it.

The problem here is that the idle host should append an extension header for fragmentation to every single IPv6 packet 
it sends to us. If we receive a packet without the extension header for fragmentation, this means we run into some 
issues manipulating the idle host. 
This is a situation which should not happen, that is why I'm killing the process here. 
A possible alternative would be do build something like a counter. If we receive a certain amount of packets from the 
idle host without the extension header for fragmentation, then we are sure there is something fishy, and only then we 
kill the process.

> > + //for some reason we need to add 2 here, have to figure out why
> > + frag_header = (struct ip6_ext_data_fragment *)((u_char *)ip6 +
> > IP6_HDR_LEN + 2);
> > + //TODO: check if the next header in the frag header says TCP
> > + //for now, we do this manually, ipv6_get_data_any sometimes return
> > null...
> > + tcp = (struct tcp_hdr *)((u_char * )ip6 + IP6_HDR_LEN + 8);
> > + //ip6data = (void *) ipv6_get_data_any(ip6, &packetlen, &ip6hdr);
>
> The two bytes are the Next Header and Hdr Ext Len fields. You can't
> assume that the the Fragmentation header is the first or only
> extension
> header. 

Yes, i figured that out by now - changed in the new version of my patch which will be ready soon. 

If ipv6_get_data_any returns NULL, there is no payload, and
> you
> can't treat what follows as a TCP packet. You should use ipv6_get_data
> instead, because TCP is one of the protocols it recognizes as an
> upper-layer protocol.

I still had sometimes the problem that ipv6_get_data returned null although wireshark showed me a payload in the packet 
i was looking at. What I do by know is to check if the next header is a tcp header, and only then continue. 
But to improve the code, I will have a look at the things you mentioned. 

> > + //for me, htonl is not necessary, instead diving by 2
> >    if (seqclass == IPID_SEQ_BROKEN_INCR) {
> >      /* Convert to network byte order */
> > - startid = htons(startid);
> > - endid = htons(endid);
> > - return endid - startid;
> > + //startid = htonl(startid);
> > + //endid = htonl(endid);
> > + return (endid - startid)/2;
> >    }
>
> You seem to be treating IPID_SEQ_BROKEN_INCR as meaning "counts by 2,"
> but it really means "counts by 1 but byteswapped." That is, instead of
> counting
> 0000,0001,0002,...,00ff,0100,0101,0102,...,feff,ff00,ff01,...
> it counts
> 0000,0100,0200,...,ff00,0001,0101,0201,...,fffe,00ff,01ff,...
> You probably need to add a new IPID_SEQ define for "counts by 2."
>
> That said, calling htons to swap bytes will not work on big-endian
> platforms, which is a bug in the current code.

You are right, I understood that wrong. During research in IPv4, I found some printers which increment by 9. Do you 
think it would be possible to create a more general IPID_SEQ which is something like "counts by x" instead of having 
one for "counts by 1", another one for "counts by 2", another one for "counts by 9"...

> > + //TODO: If we have an IPv6 address, we don't know if the last
> > colon-seperated part is the port or part of the address
> > + /* In IPv6, we have colons in the address, so we just look for the
> > last one */
>
> You should force users to use square brackets with IPv6 addresses,
> like
> RFC 3986 recommends.

Thats already changes in the new version of my patch. Either the idle host is stated without a port number, our in the 
format [ipv6-addr]:port. For this, I have to strip of the []'s before handing it to other function, because otherwise 
Nmap would complain that illegal characters are used in the hostname. 

> > + p = strdup(proxy->host.targetipstr());
> > + /* We need something that works for IPv4 and IPv6 */
> > + q = strdup(proxy->host.sourceipstr());
> > + Snprintf(filter, sizeof(filter), "icmp6 and src host %s and dst
> > host %s", p, q);
> > + free(p);
> > + free(q);
>
> I think it's unnecessary to strdup here.

I actually think that is something I took from the original code. But I will have a look at that. 

> > + /* libpcap doesn't find the source port in IPv6 if theres a
> > fragmentation header. Therefore we use this ugly hack to state the
> > source port */
> > + //TODO: what do we do if there are multiple extension headers?
> > + Snprintf(filter, sizeof(filter), "tcp and src host %s and dst host
> > %s and src port %hu or (ip6[6]=44 and ip6[40]=6 and ip6[48:2]=%hu)",
> > p, q, proxy->probe_port, proxy->probe_port);
>
> Here's another case where you will have to use a less specific filter
> and do filtering manually. Like your TODO says, you can't assume there
> there are more than one (or even one) extension header.

I did this because I wanted to filter for source and destination port, as this is done in the original code. But yes, 
you are right, I will try to use a less specific filter and filter out manually. 


Overall, thanks for all the comments. I'm not that experienced with C++ and Nmap, so its great to get some feedback 
from somebody with more experience. 

I hope I will soon be able to provide a new version of my code. 


Regards,
Mathias
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/