Re: [PATCH] TCP Idle Scan in IPv6

[David Fifield <david () bamsoftware com>]

On Fri, Aug 30, 2013 at 03:06:40PM +0200, Mathias Morbitzer wrote:
> > On Thu, Aug 15, 2013 at 02:12:45PM +0200, Mathias Morbitzer wrote:
> > 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 think we are talking about the same thing. You should do it without
duplicating code. It's not good to have two almost identical functions.
What you should do instead is factor out the part of get_ipid_sequence
that calculates the first differences:

  for (i = 1; i < numSamples; i++) {
    if (ipids[i - 1] != 0 || ipids[i] != 0)
      allipideqz = 0; /* All IP.ID values do *NOT* equal zero */

    if (ipids[i - 1] <= ipids[i]) {
        ipid_diffs[i - 1] = ipids[i] - ipids[i - 1];
    } else {
        ipid_diffs[i - 1] = (u16) (ipids[i] - ipids[i - 1] + 65536);
    }

This should be split into two functions, one for 16-bit and one for
32-bit. What I mean with applying boolean and, is that I think you can
define the 16-bit function in terms of the 32-bit function, to further
avoid code duplication. Like this pseudocode:

func calc_ipid_diffs_32(u32 ipids[]) {
  for (i = 1; i < length(ipids); i++) {
    diffs[i-1] = ipids[i] - ipids[i-1]
  }
  return diffs
}
func calc_ipid_diffs_16(u32 ipids[]) {
  diffs = calc_ipid_diffs_32(ipids)
  for (i = 0; i < length(diffs); i++) {
    diffs[i] = diffs[i] & 0xffff
  }
  return diffs
}

You may have to make another few refactoring changes. Try to do it
without copy-pasting code, is my main point.

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