Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 3112] CDP Checksum Calculation Incorrect

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3112





--- Comment #7 from Greg Neujahr <wireshark@xxxxxxxxxxxxxxxxxxxxx>  2008-12-23 09:06:46 PDT ---
Did a little more testing using the samples that Jaap provided (thanks). I'm
going to hook my debugger up to them and figure out what's going on shortly. In
the meantime, because it helps me think, here's just a basic rundown of what's
going on:

Raw bytes in network order are read from the card and stored in a pointer to an
unsigned char. This means we get raw access to the bytes in the order of the
network. In the 'real cdp' checksum function, we convert that pointer to an
unsigned short. What this does, is automatically swap the bytes in the
resulting short. As an example, here's some code that shows it:

  unsigned char  t[] = {0xab, 0xcd};
  unsigned short s   = *((unsigned short *) t);
  unsigned char  *p  = (unsigned char *) &s;
  printf("unsigned short: 0x%x\n", s);
  printf("     raw bytes: 0x%x, 0x%x\n", p[0], p[1]);

And the output:

  unsigned short: 0xcdab
       raw bytes: 0xab, 0xcd

So, we know we put in 0xabcd, but the resulting short is 0xcdab. Following that
logic, we need to make sure that all bytes are byte swapped for addition.
Adding shorts of various byte orderings will cause nonsense to be generated at
the end. If we have an odd number of bytes in the data we need to pad a single
0x00 onto the end. The outstanding question is whether or not this is
byteswapped or left as is. Doing this:

  sum += htons(*((unsigned char *)d));

Converts our short pointer back into an unsigned char pointer, which will give
us the data in raw network order again. Since the htons function is expecting a
short, it will convert the raw 0xaa char into a 0xaa00 short (on a little
endian machine). Since we have byte-swapped everything else, we should also
byte-swap the last padding byte as well to keep the addition correct, so we use
htons to do that. On a little endian machine, htons will swap the bytes and
gives us back 0x00aa. We then add, adjust for carry, and then invert. This
provides us with the correct checksum for CDP.

Now, if we instead do:

  sum += *((unsigned char *)d);

This converts our short back to a char in raw network order. Since sum is a
short, the 0xaa that is stored in this new char pointer will need to be
converted into a short, which will give us 0xaa00 as a short. If we add this in
raw, we are adding in a non-swapped short with a bunch of swapped shorts,
causing the checksum to be incorrect.

Jaap, to confirm some of your findings, we take the raw data from the wire
which is in network order, and convert it into host order by type casting the
pointer to a short. We add everything up in host order until we get to the last
byte in an odd length. Using the htons doesn't convert it back into network
order, it converts the 0xaa00 raw short into host order by byte-swapping the
pad and final byte (it could be rewritten to cast the short* into a char*,
dereference it, then cast it back to a short, which would have the same effect
[something akin to ((unsigned short) *((unsigned char *)d))]. So, in essence,
we never change the ordering, we always keep it the same. The WS code seems to
be the one that changes byte ordering for the odd length data streams.

If we look at the actual code from WS and assume that everything is always
aligned on an even boundry, and that we will never have more than one 'vector',
then we can reduce the whole thing to this (removing all the unrolled loop bits
-- note this wasn't tested, just a mental exercise):

int in_cksum(const unsigned char *data, int len) {
   const unsigned short *w;
   int sum = 0;
   int mlen = 0;

   union {
      unsigned char c[2];
      unsigned short s;
   } s_util;

   w = (const unsigned short *) data;
   mlen = len;

   while((mlen -= 2) >= 0) {
      sum += *w++;
   }

   if (mlen == -1) {
      s_util.c[0] = *(const unsigned char *)w;
      s_util.c[1] = 0;
      sum += s_util.s;
   }

   return (~sum & 0xffff);
}

So, it seems to do a fairly decent job. Takes in the raw network order data,
typecasts to a short, which will change the ordering, adds everything up, and
then, if there's a byte leftover at the end, it puts the byte in an array at
position 0, and then puts the pad in position 1. Being a union and all, they
share the same space. The trick here is that we aren't getting that final
byteswap. Since we didn't do a funky typecast from char to short, the automatic
swapping isn't going to happen (and why would it, since we already have a short
in the union we access directly?).

I think with that, we can see where the problem is. It would appear that
conversion from char* to short* causes the byte swapping to happen when its
derefenced. Since the WS code directly manipulates a short, the padding goes
into the end byte, but it's never byte swapped like the rest of it is, so the
checksum is incorrect.

Looks like the last padding byte that is inserted would have to be byte-swapped
if on a little endian machine. The WS code would appear to be doing so
incorrectly by relying on a mechanism that doesn't work. It may have been that
it once worked, but no longer does, but I can't say that for sure.

Anyone else care to confirm or refute those conclusions?


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.