checksum bug in 1.1.2

"Beattie, David" <[email protected]> Fri, 15 Oct 2004 04:21:16 -0500
Newsgroups gmane.comp.security.libnet
Message-ID <[email protected]>
Hi all,

In libnet-1.1.2, one of the changelog items says:
    Fixed a bug on big endian boxes that had TCP and UDP checksums with
odd
     payloads come out incorrect

I just upgraded to libnet-1.1.2, and I started immediately having
trouble with checksums on my (little-endian, but that's irrelevant as
you will see) box, with odd-sized tcp payloads.  So, I decided to look
at the code, and found it to be clearly wrong-it can overrun the end of
an odd-sized buffer and grab a random value from underlying memory and
add it into the checksum.  Here's a possible fix with minimal changes
from the current released code (this is what I'm using now):

libnet_in_cksum(u_int16_t *addr, int len)
{
    int sum;
    union {
        u_int16_t s;
        u_int8_t b[2];
    } pad;

    sum = 0;

    while (len > 1)
    {
        sum += *addr++;
        len -= 2;
    }
    if (len == 1)
    {
        pad.b[0] = *(u_int8_t *)addr;
        pad.b[1] = 0;
        sum += pad.s;
    }

    return (sum);
}

I haven't tested on any big-endian systems, but I imagine that it would
work.  It definitely solves my problem on my little-endian system, and
seems to be fundamentally correct, if my assumption that the compiler
will always pack elements in a union at the start of the union's memory
is correct.  Of course, if the compiler can't do the union in a register
(anybody know?), then my way involves a memory-to-memory copy, and
therefore is not efficient... so the really efficient way may be an
#ifdef block per endian style.
 
As a suggestion, in my opinion, the parameter to libnet_in_cksum would
more naturally be a char * or u_int8_t *, and the cast to a u_int16_t *
would be done inside libnet_in_cksum while
dereferencing/autoincrementing.  This would save a bunch of casting
elsewhere in this file.  It's not important though-just a question of
style and personal preference.

--David