[lwip-users] lwIP not properly checking packet lengths

[Marc Boucher]

Hi,

I would like to draw everyone's attention to the fact that lwIP
doesn't do sufficient sanity checking on the fields of incoming packets,
especially lengths, meaning that short or corrupted packets accidentally or
intentionally sent to a lwIP host may lead to crashes, memory corruption,
data leaks, or exploitable security holes.

The patch below attempts to add a minimal TCP header length check (which
is better than nothing but not perfect since it might be variable),
and a comment in the dhcp_recv() routine to point out the missing check
there (people who care about the DHCP code should fix this ASAP).

Also, the practice of avoiding unused argument warnings with
an if statement is confusing and should be avoided. I would
suggest phasing it out in favor of just casting the variable to (void),
like it is already done in parts of the code..

Regards
Marc

diff -ur cvs-20030518/lwip/src/core/dhcp.c cvs-20030518-mb/lwip/src/core/dhcp.c
--- cvs-20030518/lwip/src/core/dhcp.c   2003-05-12 14:18:14.000000000 +0200
+++ cvs-20030518-mb/lwip/src/core/dhcp.c        2003-05-18 16:47:20.000000000 
+0200
@@ -1111,6 +1111,7 @@
 {
   struct netif *netif = (struct netif *)arg;
   struct dhcp *dhcp = netif->dhcp;
+  /* TODO: check length before accessing payload!!! */
   struct dhcp_msg *reply_msg = (struct dhcp_msg *)p->payload;
   u8_t *options_ptr;
   u8_t msg_type;
@@ -1122,8 +1123,10 @@
     (u8_t)(ntohl(addr->addr) & 0xff), port));
   DEBUGF(DHCP_DEBUG | DBG_TRACE, ("pbuf->len = %u\n", p->len));
   DEBUGF(DHCP_DEBUG | DBG_TRACE, ("pbuf->tot_len = %u\n", p->tot_len));
-  /* prevent warning */
-  if (pcb || addr || port);
+  /* prevent warnings */
+  (void)pcb;
+  (void)addr;
+  (void)port;
   dhcp->p = p;
   if (reply_msg->op != DHCP_BOOTREPLY) {
     DEBUGF(DHCP_DEBUG | DBG_TRACE | 1, ("not a DHCP reply message, but type 
%u\n", reply_msg->op));
diff -ur cvs-20030518/lwip/src/core/ipv6/icmp6.c 
cvs-20030518-mb/lwip/src/core/ipv6/icmp6.c
--- cvs-20030518/lwip/src/core/ipv6/icmp6.c     2003-05-12 14:18:14.000000000 
+0200
+++ cvs-20030518-mb/lwip/src/core/ipv6/icmp6.c  2003-05-18 16:38:47.000000000 
+0200
@@ -56,6 +56,8 @@
   ++lwip_stats.icmp.recv;
 #endif /* ICMP_STATS */
 
+  /* XXX TODO: check length before accessing payload! */
+
   type = ((char *)p->payload)[0];
 
   switch (type) {
@@ -103,7 +105,7 @@
                 iphdr->hoplim, IP_PROTO_ICMP, inp);
     break; 
   default:
-    DEBUGF(ICMP_DEBUG, ("icmp_input: ICMP type not supported.\n"));
+    DEBUGF(ICMP_DEBUG, ("icmp_input: ICMP type %d not supported.\n", 
(int)type));
 
 #ifdef ICMP_STATS
     ++lwip_stats.icmp.proterr;
diff -ur cvs-20030518/lwip/src/core/tcp.c cvs-20030518-mb/lwip/src/core/tcp.c
--- cvs-20030518/lwip/src/core/tcp.c    2003-05-12 14:18:14.000000000 +0200
+++ cvs-20030518-mb/lwip/src/core/tcp.c 2003-05-18 17:50:55.000000000 +0200
@@ -293,7 +286,10 @@
 static err_t
 tcp_accept_null(void *arg, struct tcp_pcb *pcb, err_t err)
 {
-  if (arg || pcb || err);
+  (void)arg;
+  (void)pcb;
+  (void)err;
+
   return ERR_ABRT;
 }
 #endif /* LWIP_CALLBACK_API */
diff -ur cvs-20030518/lwip/src/core/tcp_in.c 
cvs-20030518-mb/lwip/src/core/tcp_in.c
--- cvs-20030518/lwip/src/core/tcp_in.c 2003-05-12 14:18:14.000000000 +0200
+++ cvs-20030518-mb/lwip/src/core/tcp_in.c      2003-05-18 16:54:21.000000000 
+0200
@@ -113,7 +113,7 @@
   iphdr = p->payload;
   tcphdr = (struct tcp_hdr *)((u8_t *)p->payload + IPH_HL(iphdr) * 4);
  
-  if (pbuf_header(p, -((s16_t)(IPH_HL(iphdr) * 4)))) {
+  if (pbuf_header(p, -((s16_t)(IPH_HL(iphdr) * 4))) || (p->tot_len < 
sizeof(struct tcp_hdr))) {
     /* drop short packets */
     DEBUGF(TCP_INPUT_DEBUG, ("tcp_input: short packet (%u bytes) discarded\n", 
p->tot_len));
 #ifdef TCP_STATS