Bug fixes and reproduction code for ip_offset accounting problem in libnet_build_ipv4

What happens is the ip_offset doesn't get correctly updated, and the checksum
is written into invalid memory. Depending on your architecture, and the
location of invalid memory, you might segv, silently produce packets with bad
checksums or the checksum overwriting some other part of the header, or corrupt
glib's internal alloc data structures.

See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=418975#47 for more information.

9 files changed, 73 insertions, 260 deletions

diff --git a/.gitignore b/.gitignore
index 3008a76..dece2c8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -52,6 +52,7 @@ libnet/sample/stp
 libnet/sample/synflood
 libnet/sample/synflood6
 libnet/sample/synflood6_frag
+libnet/sample/test_ipv4
 libnet/sample/tcp1
 libnet/sample/tcp2
 libnet/sample/tftp
diff --git a/libnet/include/libnet/libnet-functions.h b/libnet/include/libnet/libnet-functions.h
index c4fe599..99b1139 100644
--- a/libnet/include/libnet/libnet-functions.h
+++ b/libnet/include/libnet/libnet-functions.h
@@ -794,6 +794,7 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag);
 /**
  * Builds a version 4 RFC 791 Internet Protocol (IP) header.
  * @param len total length of the IP packet including all subsequent data
+ *   FIXME There is no reason this can't be calculated if zero is passed.
  * @param tos type of service bits
  * @param id IP identification number
  * @param frag fragmentation bits and offset
@@ -2080,10 +2081,10 @@ u_int8_t type);
   * Function updates referer used to compute the checksum. All
   * pblock need to know where is their referer (ie IP header).
   * So, this function is called each time a new IP header is inserted.
-  * It updates the ip_pos field (referer) of each subsequent pblock.
+  * It updates the ip_offset field (referer) of each previous pblock.
   */
 void
-libnet_pblock_record_ip_offset(libnet_t *l, u_int32_t offset);
+libnet_pblock_record_ip_offset(libnet_t *l, libnet_pblock_t *p);
 
 /*
  * [Internal] 
diff --git a/libnet/include/libnet/libnet-structures.h b/libnet/include/libnet/libnet-structures.h
index 38aff5d..eb7de76 100644
--- a/libnet/include/libnet/libnet-structures.h
+++ b/libnet/include/libnet/libnet-structures.h
@@ -79,8 +79,19 @@ struct libnet_protocol_block
     u_int8_t *buf;                      /* protocol buffer */
     u_int32_t b_len;                    /* length of buf */
     u_int16_t h_len;                    /* header length (for checksumming) */
-    u_int32_t ip_offset;                /* offset to IP header for csums */
-    u_int32_t copied;                   /* bytes copied */
+       /* Unused for IPV4_H block types.
+        * For protocols that sit on top of IP, it should be the the amount of
+        * buf that is the header, and will be included in the checksum.
+        */
+    u_int32_t ip_offset;                /* offset from end of pkt to beginning of IP header for csums */
+       /* Unused for IPV4_H block types.
+        * For protocols that sit on top of IP (UDP, ICMP, ...), they often
+        * include some information from the IP header (in the form of a "pseudo
+        * header") in their own checksum calculation. To build that
+        * pseudo-header, thet need to find the real header.
+        */
+    u_int32_t copied;                   /* bytes copied - the amount of data copied into buf */
+       /* Used and updated by libnet_pblock_append(). */
     u_int8_t type;                      /* type of pblock */
 /* this needs to be updated every time a new packet builder is added */
 #define LIBNET_PBLOCK_ARP_H             0x01    /* ARP header */
diff --git a/libnet/sample/Makefile.am b/libnet/sample/Makefile.am
index 05cccbb..3186f5d 100644
--- a/libnet/sample/Makefile.am
+++ b/libnet/sample/Makefile.am
@@ -14,7 +14,7 @@ noinst_PROGRAMS = arp cdp dhcp_discover get_addr icmp_timestamp icmp_unreach \
                   smurf dot1x dns rpc_tcp rpc_udp mpls icmp_timeexceed \
                   fddi_tcp1 fddi_tcp2 tring_tcp1 tring_tcp2 icmp_redirect \
                   bgp4_hdr bgp4_open bgp4_update bgp4_notification gre \
-                  synflood6_frag tftp ip_link ip_raw sebek
+                  synflood6_frag tftp ip_link ip_raw sebek test_ipv4
 
 arp_SOURCES             = arp.c
 cdp_SOURCES             = cdp.c
@@ -59,5 +59,6 @@ gre_SOURCES		= gre.c
 ip_raw_SOURCES          = ip_raw.c
 ip_link_SOURCES		= ip_link.c
 sebek_SOURCES           = sebek.c
+test_ipv4_SOURCES       = test_ipv4.c
 
 LDADD 			= $(top_srcdir)/src/libnet.la
diff --git a/unmerged/test_ipv4.c b/libnet/sample/test_ipv4.c
index cf1646b..cf1646b 100644
--- a/unmerged/test_ipv4.c
+++ b/libnet/sample/test_ipv4.c
diff --git a/libnet/src/libnet_build_ip.c b/libnet/src/libnet_build_ip.c
index 3d3675b..e0e760d 100644
--- a/libnet/src/libnet_build_ip.c
+++ b/libnet/src/libnet_build_ip.c
@@ -45,7 +45,6 @@ libnet_build_ipv4(u_int16_t len, u_int8_t tos, u_int16_t id, u_int16_t frag,
 u_int8_t ttl, u_int8_t prot, u_int16_t sum, u_int32_t src, u_int32_t dst,
 u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
 {
-    int offset;
     u_int32_t h, n, i, j;
     libnet_pblock_t *p, *p_data, *p_temp;
     struct libnet_ipv4_hdr ip_hdr;
@@ -58,9 +57,12 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
 
     n = LIBNET_IPV4_H;                      /* size of memory block */
     h = len;                                /* header length */
+    // WRONG - this is total len of ip packet, and is put into the IP header
     ptag_data = 0;                          /* used if options are present */
+    // WRONG - is used if there is ipv4 payload
 
     if (h + payload_s > IP_MAXPACKET)
+    // WRONG - h is the total length, it already includes payload_s
     {
          snprintf(l->err_buf, LIBNET_ERRBUF_SIZE,
                 "%s(): IP packet too large\n", __func__);
@@ -97,6 +99,9 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
             ip_hdr.ip_hl += j;
         }
     }
+    // Note that p->h_len is not adjusted. This seems a bug, but it is because
+    // it is not used!  libnet_do_checksum() is passed the h_len (as `len'),
+    // but for IPPROTO_IP it is ignored in favor of the ip_hl.
 
     ip_hdr.ip_tos        = tos;                       /* IP tos */
     ip_hdr.ip_len        = htons(h);                  /* total length */
@@ -123,7 +128,10 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
     }
 
     /* find and set the appropriate ptag, or else use the default of 0 */
-    offset = payload_s;
+    /* When updating the ipv4 block, we need to find the data block, and
+     * adjust our ip_offset if the new payload size is different from what
+     * it used to be.
+     */
     if (ptag_hold && p->prev)
     {
         p_temp = p->prev;
@@ -136,9 +144,13 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
 
         if (p_temp->type == LIBNET_PBLOCK_IPDATA)
         {
+            int offset = payload_s;
+
             ptag_data = p_temp->ptag;
             offset -=  p_temp->b_len;
-            p->h_len += offset;
+            //p->h_len += offset;
+            // WRONG h_len is unused for checksum for IPv4, and even if it was used,
+            // the h_len doesn't depend on the payload size.
         }
         else
         {
@@ -157,6 +169,16 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
     if (payload && payload_s)
     {
         /* update ptag_data with the new payload */
+        // on create:
+        //    b_len = payload_s
+        //    l->total_size += b_len
+        //    h_len = 0
+        // on update:
+        //    b_len = payload_s
+        //    h_len += <diff in size between new b_len and old b_len>
+        //      increments if if b_len goes up, down if it goes down
+        // in either case:
+        //    copied = 0
         p_data = libnet_pblock_probe(l, ptag_data, payload_s,
                 LIBNET_PBLOCK_IPDATA);
         if (p_data == NULL)
@@ -171,6 +193,7 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
 
         if (ptag_data == LIBNET_PTAG_INITIALIZER)
         {
+            // IPDATA's h_len gets set to payload_s in both branches
             if (p_data->prev->type == LIBNET_PBLOCK_IPV4_H)
             {
                 libnet_pblock_update(l, p_data, payload_s,
@@ -180,6 +203,10 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
             }
             else
             {
+                // SR - I'm not sure how to reach this code. Maybe if the first
+                // time we added an ipv4 block, there was no payload, but when
+                // we modify the block the next time, we have payload?
+
                 /* update without setting this as the final pblock */
                 p_data->type  =  LIBNET_PBLOCK_IPDATA;
                 p_data->ptag  =  ++(l->ptag_state);
@@ -187,6 +214,7 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
 
                 /* Adjust h_len for checksum. */
                 p->h_len += payload_s;
+                // WRONG - IPV4 checksum doesn't include the payload_s.
 
                 /* data was added after the initial construction */
                 for (p_temp = l->protocol_blocks;
@@ -238,7 +266,16 @@ u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
      * FREDRAYNAL: as we insert a new IP header, all checksums for headers
      * placed after this one will refer to here.
      */
-    libnet_pblock_record_ip_offset(l, l->total_size);
+    // WRONG - the total_size when updating the pblock will include the link layer
+    // WRONG - it isn't called after adding options, so will be wrong by the amount of ip options
+    // WRONG - it updates the wrong protocol blocks:
+    //   - the first time it runs we set the ip offsets for p (ipv4), and
+    //     ipdata to the total size of just the ip portion
+    //   - the next time, it starts at end, which is the ethernet block, and
+    //     updates everything up to but not including the ipv4 block to the total size, which means it
+    //     changes just the ethernet block, and the offset it sets is the total size including the ethernet
+    //     header.... WTF?
+    libnet_pblock_record_ip_offset(l, p);
 
     return (ptag);
 bad:
@@ -323,7 +360,7 @@ libnet_autobuild_ipv4(u_int16_t len, u_int8_t prot, u_int32_t dst, libnet_t *l)
      * FREDRAYNAL: as we insert a new IP header, all checksums for headers
      * placed after this one will refer to here.
      */
-    libnet_pblock_record_ip_offset(l, l->total_size);
+    libnet_pblock_record_ip_offset(l, p);
     return (ptag);
 
 bad:
diff --git a/libnet/src/libnet_init.c b/libnet/src/libnet_init.c
index 976b44c..58f4df1 100644
--- a/libnet/src/libnet_init.c
+++ b/libnet/src/libnet_init.c
@@ -250,6 +250,7 @@ libnet_getpbuf_size(libnet_t *l, libnet_ptag_t ptag)
 u_int32_t
 libnet_getpacket_size(libnet_t *l)
 {
+    // Why doesn't this return l->total_size?
     libnet_pblock_t *p;
     u_int32_t n;
 
diff --git a/libnet/src/libnet_pblock.c b/libnet/src/libnet_pblock.c
index 5e35aa5..b213b1d 100644
--- a/libnet/src/libnet_pblock.c
+++ b/libnet/src/libnet_pblock.c
@@ -38,6 +38,7 @@
 #else
 #include "../include/win32/libnet.h"
 #endif
+#include <assert.h>
 
 libnet_pblock_t *
 libnet_pblock_probe(libnet_t *l, libnet_ptag_t ptag, u_int32_t n, u_int8_t type)
@@ -500,15 +501,18 @@ libnet_pblock_p2p(u_int8_t type)
 }
 
 void
-libnet_pblock_record_ip_offset(libnet_t *l, u_int32_t offset)
+libnet_pblock_record_ip_offset(libnet_t *l, libnet_pblock_t *p)
 {
-    libnet_pblock_t *p = l->pblock_end;
+    libnet_pblock_t *c;
+    u_int32_t ip_offset = 0;
 
-    do
-    {
-        p->ip_offset = offset;
-        p = p->prev;
-    } while (p && p->type != LIBNET_PBLOCK_IPV4_H);
+    assert(p->type == LIBNET_PBLOCK_IPV4_H);
+
+    for(c = p; c; c = c->prev)
+        ip_offset += c->b_len;
+
+    for(c = p; c; c = c->prev)
+        c->ip_offset = ip_offset;
 }
 
 
diff --git a/unmerged/bug-418975.patch b/unmerged/bug-418975.patch
deleted file mode 100644
index 7376070..0000000
--- a/unmerged/bug-418975.patch
+++ /dev/null
@@ -1,243 +0,0 @@
-Index: src/libnet_init.c
-===================================================================
---- src/libnet_init.c	(revision 374)
-+++ src/libnet_init.c	(working copy)
-@@ -250,6 +250,7 @@
- u_int32_t
- libnet_getpacket_size(libnet_t *l)
- {
-+    // Why doesn't this return l->total_size?
-     libnet_pblock_t *p;
-     u_int32_t n;
- 
-Index: src/libnet_build_ip.c
-===================================================================
---- src/libnet_build_ip.c	(revision 374)
-+++ src/libnet_build_ip.c	(working copy)
-@@ -45,7 +45,6 @@
- u_int8_t ttl, u_int8_t prot, u_int16_t sum, u_int32_t src, u_int32_t dst,
- u_int8_t *payload, u_int32_t payload_s, libnet_t *l, libnet_ptag_t ptag)
- {
--    int offset;
-     u_int32_t h, n, i, j;
-     libnet_pblock_t *p, *p_data, *p_temp;
-     struct libnet_ipv4_hdr ip_hdr;
-@@ -58,9 +57,12 @@
- 
-     n = LIBNET_IPV4_H;                      /* size of memory block */
-     h = len;                                /* header length */
-+    // WRONG - this is total len of ip packet, and is put into the IP header
-     ptag_data = 0;                          /* used if options are present */
-+    // WRONG - is used if there is ipv4 payload
- 
-     if (h + payload_s > IP_MAXPACKET)
-+    // WRONG - h is the total length, it already includes payload_s
-     {
-          snprintf(l->err_buf, LIBNET_ERRBUF_SIZE,
-                 "%s(): IP packet too large\n", __func__);
-@@ -97,6 +99,9 @@
-             ip_hdr.ip_hl += j;
-         }
-     }
-+    // Note that p->h_len is not adjusted. This seems a bug, but it is because
-+    // it is not used!  libnet_do_checksum() is passed the h_len (as `len'),
-+    // but for IPPROTO_IP it is ignored in favor of the ip_hl.
- 
-     ip_hdr.ip_tos        = tos;                       /* IP tos */
-     ip_hdr.ip_len        = htons(h);                  /* total length */
-@@ -123,7 +128,10 @@
-     }
- 
-     /* find and set the appropriate ptag, or else use the default of 0 */
--    offset = payload_s;
-+    /* When updating the ipv4 block, we need to find the data block, and
-+     * adjust our ip_offset if the new payload size is different from what
-+     * it used to be.
-+     */
-     if (ptag_hold && p->prev)
-     {
-         p_temp = p->prev;
-@@ -136,9 +144,13 @@
- 
-         if (p_temp->type == LIBNET_PBLOCK_IPDATA)
-         {
-+            int offset = payload_s;
-+
-             ptag_data = p_temp->ptag;
-             offset -=  p_temp->b_len;
--            p->h_len += offset;
-+            //p->h_len += offset;
-+            // WRONG h_len is unused for checksum for IPv4, and even if it was used,
-+            // the h_len doesn't depend on the payload size.
-         }
-         else
-         {
-@@ -157,6 +169,16 @@
-     if (payload && payload_s)
-     {
-         /* update ptag_data with the new payload */
-+        // on create:
-+        //    b_len = payload_s
-+        //    l->total_size += b_len
-+        //    h_len = 0
-+        // on update:
-+        //    b_len = payload_s
-+        //    h_len += <diff in size between new b_len and old b_len>
-+        //      increments if if b_len goes up, down if it goes down
-+        // in either case:
-+        //    copied = 0
-         p_data = libnet_pblock_probe(l, ptag_data, payload_s,
-                 LIBNET_PBLOCK_IPDATA);
-         if (p_data == NULL)
-@@ -171,6 +193,7 @@
- 
-         if (ptag_data == LIBNET_PTAG_INITIALIZER)
-         {
-+            // IPDATA's h_len gets set to payload_s in both branches
-             if (p_data->prev->type == LIBNET_PBLOCK_IPV4_H)
-             {
-                 libnet_pblock_update(l, p_data, payload_s,
-@@ -180,6 +203,10 @@
-             }
-             else
-             {
-+                // SR - I'm not sure how to reach this code. Maybe if the first
-+                // time we added an ipv4 block, there was no payload, but when
-+                // we modify the block the next time, we have payload?
-+
-                 /* update without setting this as the final pblock */
-                 p_data->type  =  LIBNET_PBLOCK_IPDATA;
-                 p_data->ptag  =  ++(l->ptag_state);
-@@ -187,6 +214,7 @@
- 
-                 /* Adjust h_len for checksum. */
-                 p->h_len += payload_s;
-+                // WRONG - IPV4 checksum doesn't include the payload_s.
- 
-                 /* data was added after the initial construction */
-                 for (p_temp = l->protocol_blocks;
-@@ -238,7 +266,16 @@
-      * FREDRAYNAL: as we insert a new IP header, all checksums for headers
-      * placed after this one will refer to here.
-      */
--    libnet_pblock_record_ip_offset(l, l->total_size);
-+    // WRONG - the total_size when updating the pblock will include the link layer
-+    // WRONG - it isn't called after adding options, so will be wrong by the amount of ip options
-+    // WRONG - it updates the wrong protocol blocks:
-+    //   - the first time it runs we set the ip offsets for p (ipv4), and
-+    //     ipdata to the total size of just the ip portion
-+    //   - the next time, it starts at end, which is the ethernet block, and
-+    //     updates everything up to but not including the ipv4 block to the total size, which means it
-+    //     changes just the ethernet block, and the offset it sets is the total size including the ethernet
-+    //     header.... WTF?
-+    libnet_pblock_record_ip_offset(l, p);
- 
-     return (ptag);
- bad:
-@@ -323,7 +360,7 @@
-      * FREDRAYNAL: as we insert a new IP header, all checksums for headers
-      * placed after this one will refer to here.
-      */
--    libnet_pblock_record_ip_offset(l, l->total_size);
-+    libnet_pblock_record_ip_offset(l, p);
-     return (ptag);
- 
- bad:
-@@ -407,7 +444,7 @@
-     }
- 
-     /* append padding */
--    n = libnet_pblock_append(l, p, "\0\0\0", adj_size - options_s);
-+    n = libnet_pblock_append(l, p, (u_int8_t*) "\0\0\0", adj_size - options_s);
-     if (n == -1)
-     {
-         goto bad;
-Index: src/libnet_pblock.c
-===================================================================
---- src/libnet_pblock.c	(revision 374)
-+++ src/libnet_pblock.c	(working copy)
-@@ -38,6 +38,7 @@
- #else
- #include "../include/win32/libnet.h"
- #endif
-+#include <assert.h>
- 
- libnet_pblock_t *
- libnet_pblock_probe(libnet_t *l, libnet_ptag_t ptag, u_int32_t n, u_int8_t type)
-@@ -496,15 +497,18 @@
- }
- 
- void
--libnet_pblock_record_ip_offset(libnet_t *l, u_int32_t offset)
-+libnet_pblock_record_ip_offset(libnet_t *l, libnet_pblock_t *p)
- {
--    libnet_pblock_t *p = l->pblock_end;
-+    libnet_pblock_t *c;
-+    u_int32_t ip_offset = 0;
- 
--    do
--    {
--        p->ip_offset = offset;
--        p = p->prev;
--    } while (p && p->type != LIBNET_PBLOCK_IPV4_H);
-+    assert(p->type == LIBNET_PBLOCK_IPV4_H);
-+
-+    for(c = p; c; c = c->prev)
-+        ip_offset += c->b_len;
-+
-+    for(c = p; c; c = c->prev)
-+        c->ip_offset = ip_offset;
- }
- 
- 
-Index: include/libnet/libnet-structures.h
-===================================================================
---- include/libnet/libnet-structures.h	(revision 374)
-+++ include/libnet/libnet-structures.h	(working copy)
-@@ -79,8 +79,19 @@
-     u_int8_t *buf;                      /* protocol buffer */
-     u_int32_t b_len;                    /* length of buf */
-     u_int16_t h_len;                    /* header length (for checksumming) */
--    u_int32_t ip_offset;                /* offset to IP header for csums */
--    u_int32_t copied;                   /* bytes copied */
-+       /* Unused for IPV4_H block types.
-+        * For protocols that sit on top of IP, it should be the the amount of
-+        * buf that is the header, and will be included in the checksum.
-+        */
-+    u_int32_t ip_offset;                /* offset from end of pkt to beginning of IP header for csums */
-+       /* Unused for IPV4_H block types.
-+        * For protocols that sit on top of IP (UDP, ICMP, ...), they often
-+        * include some information from the IP header (in the form of a "pseudo
-+        * header") in their own checksum calculation. To build that
-+        * pseudo-header, thet need to find the real header.
-+        */
-+    u_int32_t copied;                   /* bytes copied - the amount of data copied into buf */
-+       /* Used and updated by libnet_pblock_append(). */
-     u_int8_t type;                      /* type of pblock */
- /* this needs to be updated every time a new packet builder is added */
- #define LIBNET_PBLOCK_ARP_H             0x01    /* ARP header */
-Index: include/libnet/libnet-functions.h
-===================================================================
---- include/libnet/libnet-functions.h	(revision 374)
-+++ include/libnet/libnet-functions.h	(working copy)
-@@ -794,6 +794,7 @@
- /**
-  * Builds a version 4 RFC 791 Internet Protocol (IP) header.
-  * @param len total length of the IP packet including all subsequent data
-+ *   FIXME There is no reason this can't be calculated if zero is passed.
-  * @param tos type of service bits
-  * @param id IP identification number
-  * @param frag fragmentation bits and offset
-@@ -2074,10 +2075,10 @@
-   * Function updates referer used to compute the checksum. All
-   * pblock need to know where is their referer (ie IP header).
-   * So, this function is called each time a new IP header is inserted.
--  * It updates the ip_pos field (referer) of each subsequent pblock.
-+  * It updates the ip_offset field (referer) of each previous pblock.
-   */
- void
--libnet_pblock_record_ip_offset(libnet_t *l, u_int32_t offset);
-+libnet_pblock_record_ip_offset(libnet_t *l, libnet_pblock_t *p);
- 
- /*
-  * [Internal]