Segfault in dhcp_parse_reply if no end marker

If no endmarker is present in a dhcp reply a null pointer is potentially
dereferenced.

Add fix and test case as proof of concept.

diff --git a/src/core/dhcp.c b/src/core/dhcp.c
index f0f594f10baa432b3b89cbe1a227bdf23da18cb5..8b2139da3484497898dfb4949c50c43d1a8c10d6 100644 (file)
--- a/src/core/dhcp.c
+++ b/src/core/dhcp.c
@@ -1472,8 +1472,14 @@ decode_next:
     if (offset >= q->len) {
       offset -= q->len;
       offset_max -= q->len;
-      q = q->next;
-      options = (u8_t*)q->payload;
+      if (offset < offset_max && offset_max) {
+        q = q->next;
+        LWIP_ASSERT("next pbuf was null", q);
+        options = (u8_t*)q->payload;
+      } else {
+        // We've run out of bytes, probably no end marker. Don't proceed.
+        break;
+      }
     }
   }
   /* is this an overloaded message? */

diff --git a/test/unit/dhcp/test_dhcp.c b/test/unit/dhcp/test_dhcp.c
index b42c3daf533d57e9ba57336ad9481239681e69e1..39bd1575e549a04100c5fc14e9c92aac6bd3e94f 100644 (file)
--- a/test/unit/dhcp/test_dhcp.c
+++ b/test/unit/dhcp/test_dhcp.c
@@ -118,6 +118,7 @@ static enum tcase {
   TEST_LWIP_DHCP,
   TEST_LWIP_DHCP_NAK,
   TEST_LWIP_DHCP_RELAY,
+  TEST_LWIP_DHCP_NAK_NO_ENDMARKER,
 } tcase;
 
 static int debug = 0;
@@ -796,6 +797,109 @@ START_TEST(test_dhcp_relayed)
 }
 END_TEST
 
+START_TEST(test_dhcp_nak_no_endmarker)
+{
+  struct ip_addr addr;
+  struct ip_addr netmask;
+  struct ip_addr gw;
+
+  u8_t dhcp_nack_no_endmarker[] = {
+    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x54, 0x75,
+    0xd0, 0x26, 0xd0, 0x0d, 0x08, 0x00, 0x45, 0x00,
+    0x01, 0x15, 0x38, 0x86, 0x00, 0x00, 0xff, 0x11,
+    0xc0, 0xa8, 0xc0, 0xa8, 0x01, 0x01, 0xff, 0xff,
+    0xff, 0xff, 0x00, 0x43, 0x00, 0x44, 0x01, 0x01,
+    0x00, 0x00, 0x02, 0x01, 0x06, 0x00, 0x7a, 0xcb,
+    0xba, 0xf2, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x23,
+    0xc1, 0xde, 0xd0, 0x0d, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x63, 0x82,
+    0x53, 0x63, 0x35, 0x01, 0x06, 0x36, 0x04, 0xc0,
+    0xa8, 0x01, 0x01, 0x31, 0xef, 0xad, 0x72, 0x31,
+    0x43, 0x4e, 0x44, 0x30, 0x32, 0x35, 0x30, 0x43,
+    0x52, 0x47, 0x44, 0x38, 0x35, 0x36, 0x3c, 0x08,
+    0x4d, 0x53, 0x46, 0x54, 0x20, 0x35, 0x2e, 0x30,
+    0x37, 0x0d, 0x01, 0x0f, 0x03, 0x06, 0x2c, 0x2e,
+    0x2f, 0x1f, 0x21, 0x79, 0xf9, 0x2b, 0xfc, 0xff,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe2, 0x71,
+    0xf3, 0x5b, 0xe2, 0x71, 0x2e, 0x01, 0x08, 0x03,
+    0x04, 0xc0, 0xa8, 0x01, 0x01, 0xff, 0xeb, 0x1e,
+    0x44, 0xec, 0xeb, 0x1e, 0x30, 0x37, 0x0c, 0x01,
+    0x0f, 0x03, 0x06, 0x2c, 0x2e, 0x2f, 0x1f, 0x21,
+    0x79, 0xf9, 0x2b, 0xff, 0x25, 0xc0, 0x09, 0xd6,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+  };
+
+  tcase = TEST_LWIP_DHCP_NAK_NO_ENDMARKER;
+  setdebug(0);
+
+  IP4_ADDR(&addr, 0, 0, 0, 0);
+  IP4_ADDR(&netmask, 0, 0, 0, 0);
+  IP4_ADDR(&gw, 0, 0, 0, 0);
+
+  netif_add(&net_test, &addr, &netmask, &gw, &net_test, testif_init, ethernet_input);
+
+  dhcp_start(&net_test);
+
+  fail_unless(txpacket == 1); // DHCP discover sent
+  u32_t xid = net_test.dhcp->xid; // Write bad xid, not using htonl!
+  memcpy(&dhcp_offer[46], &xid, 4);
+  send_pkt(&net_test, dhcp_offer, sizeof(dhcp_offer));
+
+  // Interface down
+  fail_if(netif_is_up(&net_test));
+
+  // IP addresses should be zero
+  fail_if(memcmp(&addr, &net_test.ip_addr, sizeof(struct ip_addr)));
+  fail_if(memcmp(&netmask, &net_test.netmask, sizeof(struct ip_addr)));
+  fail_if(memcmp(&gw, &net_test.gw, sizeof(struct ip_addr)));
+
+  fail_unless(txpacket == 1); // Nothing more sent
+  xid = htonl(net_test.dhcp->xid);
+  memcpy(&dhcp_offer[46], &xid, 4); // insert correct transaction id
+  send_pkt(&net_test, dhcp_offer, sizeof(dhcp_offer));
+  
+  fail_unless(net_test.dhcp->state == DHCP_REQUESTING);
+
+  fail_unless(txpacket == 2); // No more sent
+  xid = htonl(net_test.dhcp->xid); // xid updated
+  memcpy(&dhcp_nack_no_endmarker[46], &xid, 4); // insert transaction id
+  send_pkt(&net_test, dhcp_nack_no_endmarker, sizeof(dhcp_nack_no_endmarker));
+
+  // NAK should put us in another state for a while, no other way detecting it
+  fail_unless(net_test.dhcp->state != DHCP_REQUESTING);
+
+  netif_remove(&net_test);
+}
+END_TEST
+
+
 /** Create the suite including all tests for this module */
 Suite *
 dhcp_suite(void)
@@ -803,7 +907,8 @@ dhcp_suite(void)
   TFun tests[] = {
     test_dhcp,
     test_dhcp_nak,
-    test_dhcp_relayed
+    test_dhcp_relayed,
+    test_dhcp_nak_no_endmarker
   };
   return create_suite("DHCP", tests, sizeof(tests)/sizeof(TFun), dhcp_setup, dhcp_teardown);
 }