Re: [PATCH] TCP Idle Scan in IPv6

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

> On Thu, Aug 15, 2013 at 02:12:45PM +0200, Mathias Morbitzer wrote:
> > > The change to get_ipid_sequence needs some more thought. When you
> > > are
> > > dealing with 16-bit IPv4 ID values, and you start treating them as
> > > 32-bit, you will have calculation errors at the 16-bit boundary.
> > > Imagine
> > > two consecutive IP ID values are 0xfffe and 0x0001.
> > > (u16) 0x0001 - (u16) 0xfffe == 3
> > > but
> > > (u32) 0x0001 - (u32) 0xfffe == 4294901763
> > > Do you have ideas as to how to account for this? Perhaps you need
> > > to
> > > sequence calculation to be aware of whether it is handling 16-bit
> > > or
> > > 32-bit values.
> >
> > Thank you for pointing that out, I did not think about that.
> >
> > As far as I understand, this concerns only these three lines:
> > 235: ipid_diffs[i - 1] = ipids[i] - ipids[i - 1];
> > 237: ipid_diffs[i - 1] = (u16) (ipids[i] - ipids[i - 1] + 65536);
> > 261: ipid_diffs[i] -= 256;
> > 263: ipid_diffs[i]--;
> >
> > Those are the only ones were calculations are done with the
> > ipid_diffs[] values.
> > If so, I would just put a "if (o.af() == AF_INET4) ..." before those
> > lines, and try to handle the values correctly depending on if they
> > are
> > 16 or 32 bits, like this:
> >
> > 237 in IPv4: ipid_diffs[i - 1] = (u16) (ipids[i] - ipids[i - 1] +
> > 65536);
> > 237 in IPv6: ipid_diffs[i - 1] = (u32) (ipids[i] - ipids[i - 1] +
> > 4294967296);
> >
> > 261 in IPv4: ipid_diffs[i] = (u16) ipid_diffs[i] - 256;
> > 261 in IPv6: ipid_diffs[i] -= 256;
> >
> > 263 in IPv4: ipid_diffs[i] = (u16) ipid_diffs - 1;
> > 263 in IPv6: ipid_diffs[i]--;
> >
> > Let me know what you think about that approach.
>
> I don't like to rely on spooky global state (o.af()) in a utility
> function like this one. If o.af() must be used, let it be at a higher
> level. You can call get_ipid_sequence_16 or get_ipid_sequence_32. They
> will have the same interface, taking an array of u32. But
> get_ipid_sequence_16 will do the calculation as if the values are u16.
> Both these functions could be implemented in terms of an auxiliary
> function; in fact I think get_ipid_sequence_16 can be written as a
> postprocessing step that applies boolean AND with 0xffff to the
> results
> of get_ipid_sequence_32. (For instance 4294901763 == 0xffff0003.)

get_ipid_sequence first creates an array ipid_diffs, which contains the difference between the ipids. 
Afterwards, it analyzes this ipid_diffs array to determine what kind if sequence it contains. 

As this is both done in the same function, I don't see where it would be possible to apply the boolean
AND to the results. The result returned by get_ipid_sequence is the type of sequence, so I don't have a chance 
to apply the AND to the ipid_diffs array for 16 bit. 

Instead, I would suggest to create a get_ipid_sequence_16 and get_ipid_sequence_32 function, with both create the 
ipid_diffs for 16 and for 32 bit values. The only line different in those 2 functions would be this one: 

ipid_diffs[i - 1] = (u16) (ipids[i] - ipids[i - 1] + 65536);  for 16 bit and 
ipid_diffs[i - 1] = (u16) (ipids[i] - ipids[i - 1] + 4294967296); for 32 bit. 

> From what I understand, the rest should be the same in both functions. Afterward creating the ipid_diffs array, 
both function could call the generic get_ipid_sequence function, which does the rest, which is analyzing what
kind of sequence the ipid_diffs array contains. 

> > > I saw you added a new IPID_SEQ_INCR_BY_2, which is probably the
> > > right way to do it. However, not only idle scan uses the IPID_SEQ
> > > classifications; OS detection uses them as well. Specifically, we
> > > need to decide what to do with IPID_SEQ_INCR_BY_2 in
> > > make_aval_ipid_seq. This is the code that controls what gets
> > > printed
> > > for the SEQ.TI, SEQ.CI, and SEQ.II IPv4 OS detection tests:
> > > http://nmap.org/book/osdetect-methods.html#osdetect-ti.
> > > We have two options: one is to add a new value for "increment by
> > > 2",
> > > for instance to make TI=2 possible; and the other is to find out
> > > what hosts having "increment by 2" behavior are currently being
> > > classified as, and make hosts that increment by 2 map to this
> > > value,
> > > for instance TI=I or TI=R. What did get_ipid_sequence return for
> > > hosts that increment by 2, before your patch causing them to be
> > > IPID_SEQ_INCR_BY_2?
> >
> > Before my patch, get_ipid_sequence returned "Incremental" instead of
> > "Incremental by 2". (Which caused the scan to fail because it was
> > expecting lower increments) So, hosts which increment by 2 are
> > currently classified as "incremental".
> >
> > It should not be much of an issue to apply the second solution and
> > map
> > the hosts to this value, but in my opinion it would be nicer to add
> > a
> > value for "increment by 2". This might also allow a better OS
> > detection, as we have (in IPv6) for example Windows 7 that
> > increments
> > by 2, and Windows Vista that increments by 1. Also, there are
> > operating systems like windows 7 which increment by 1 in IPv4, and
> > by
> > 2 in IPv6, which should allows allow to limit the pool of possible
> > OS
> > for OS detection.
> >
> > And not to forget, in previous research I stumbled across network
> > printers which assigned the IPID incremental by 9. They should
> > probably also get their own value at a certain point instead of
> > mapping all the "increment by x" to the same, simple "incremental".
>
> The TI=I, CI=I, II=I values are only for IPv4 OS fingerprints. IPv6
> fingerprints look different, they are basically just the packet
> contents. See the difference:
> http://nmap.org/book/osdetect-fingerprint-format.html#osdetect-ex-typical-subject-fprint
> http://nmap.org/book/osdetect-fingerprint-format.html#osdetect-ex-typical-reference-fprint-ipv6
> I agree with you that it would be best to record the size of the
> increment in fingerprints. However, to do so means potentially
> invalidating many of our existing IPv4 fingerprints.
>
> So what I propose is to have make_aval_ipid_seq return "I" for
> incremental by 2. This will maintain compatibility with existing IPv4
> fingerprints. IPv6 OS detection won't be affected. Please provide a
> new
> patch implementing this when you can.

No problem on this part, it is a one-liner. I will try to provide the new patch as soon as we found a solution for 
get_ipid_sequence. 

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