From 0e07ed4b13fb8a3f23158b11dc298b4c9486bbfb Mon Sep 17 00:00:00 2001
From: goldsimon <goldsimon@gmx.de>
Date: Fri, 25 Nov 2016 10:03:43 +0100
Subject: [PATCH] fixed bug #49676 (Possible endless loop when parsing dhcp
 options) & added unit test for that

---
 CHANGELOG                  |   3 +
 src/core/ipv4/dhcp.c       |   2 +
 test/unit/dhcp/test_dhcp.c | 111 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 114 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index b5a06e71..2859a03c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -14,6 +14,9 @@ HISTORY
 
   ++ Bugfixes:
 
+  2016-11-25: Simon Goldschmidt
+  * dhcp.c: fixed bug #49676 (Possible endless loop when parsing dhcp options)
+
   2016-11-23: Dirk Ziegelmeier
   * udp.c: fixed bug #49662: multicast traffic is now only received on a UDP PCB
    (and therefore on a UDP socket/netconn) when the PCB is bound to IP_ADDR_ANY
diff --git a/src/core/ipv4/dhcp.c b/src/core/ipv4/dhcp.c
index de5fc73b..abff34dd 100644
--- a/src/core/ipv4/dhcp.c
+++ b/src/core/ipv4/dhcp.c
@@ -1541,6 +1541,8 @@ again:
 #endif /* LWIP_DHCP_GET_NTP_SRV*/
       case(DHCP_OPTION_OVERLOAD):
         LWIP_ERROR("len == 1", len == 1, return ERR_VAL;);
+        /* decode overload only in options, not in file/sname: invalid packet */
+        LWIP_ERROR("overload in file/sname", options_idx == DHCP_OPTIONS_OFS, return ERR_VAL;);
         decode_idx = DHCP_OPTION_IDX_OVERLOAD;
         break;
       case(DHCP_OPTION_MESSAGE_TYPE):
diff --git a/test/unit/dhcp/test_dhcp.c b/test/unit/dhcp/test_dhcp.c
index 68f28819..3ff42cc4 100644
--- a/test/unit/dhcp/test_dhcp.c
+++ b/test/unit/dhcp/test_dhcp.c
@@ -120,7 +120,8 @@ static enum tcase {
   TEST_LWIP_DHCP,
   TEST_LWIP_DHCP_NAK,
   TEST_LWIP_DHCP_RELAY,
-  TEST_LWIP_DHCP_NAK_NO_ENDMARKER
+  TEST_LWIP_DHCP_NAK_NO_ENDMARKER,
+  TEST_LWIP_DHCP_INVALID_OVERLOAD
 } tcase;
 
 static int debug = 0;
@@ -904,6 +905,111 @@ START_TEST(test_dhcp_nak_no_endmarker)
 }
 END_TEST
 
+START_TEST(test_dhcp_invalid_overload)
+{
+  u8_t dhcp_offer_invalid_overload[] = {
+      0x00, 0x23, 0xc1, 0xde, 0xd0, 0x0d, /* To unit */
+      0x00, 0x0F, 0xEE, 0x30, 0xAB, 0x22, /* From Remote host */
+      0x08, 0x00, /* Protocol: IP */
+      0x45, 0x10, 0x01, 0x48, 0x00, 0x00, 0x00, 0x00, 0x80, 0x11, 0x36, 0xcc, 0xc3, 0xaa, 0xbd, 0xab, 0xc3, 0xaa, 0xbd, 0xc8, /* IP header */
+      0x00, 0x43, 0x00, 0x44, 0x01, 0x34, 0x00, 0x00, /* UDP header */
+
+      0x02, /* Type == Boot reply */
+      0x01, 0x06, /* Hw Ethernet, 6 bytes addrlen */
+      0x00, /* 0 hops */
+      0xAA, 0xAA, 0xAA, 0xAA, /* Transaction id, will be overwritten */
+      0x00, 0x00, /* 0 seconds elapsed */
+      0x00, 0x00, /* Flags (unicast) */
+      0x00, 0x00, 0x00, 0x00, /* Client ip */
+      0xc3, 0xaa, 0xbd, 0xc8, /* Your IP */
+      0xc3, 0xaa, 0xbd, 0xab, /* DHCP server ip */
+      0x00, 0x00, 0x00, 0x00, /* relay agent */
+      0x00, 0x23, 0xc1, 0xde, 0xd0, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* MAC addr + padding */
+
+      /* Empty server name */
+      0x34, 0x01, 0x02, 0xff, /* Overload: SNAME + END */
+      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,
+      /* Empty boot file name */
+      0x34, 0x01, 0x01, 0xff, /* Overload FILE + END */
+      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, /* Magic cookie */
+      0x35, 0x01, 0x02, /* Message type: Offer */
+      0x36, 0x04, 0xc3, 0xaa, 0xbd, 0xab, /* Server identifier (IP) */
+      0x33, 0x04, 0x00, 0x00, 0x00, 0x78, /* Lease time 2 minutes */
+      0x03, 0x04, 0xc3, 0xaa, 0xbd, 0xab, /* Router IP */
+      0x01, 0x04, 0xff, 0xff, 0xff, 0x00, /* Subnet mask */
+      0x34, 0x01, 0x03, /* Overload: FILE + SNAME */
+      0xff, /* End option */
+      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, /* Padding */
+  };
+  ip4_addr_t addr;
+  ip4_addr_t netmask;
+  ip4_addr_t gw;
+  u32_t xid;
+  LWIP_UNUSED_ARG(_i);
+
+  tcase = TEST_LWIP_DHCP_INVALID_OVERLOAD;
+  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);
+  netif_set_up(&net_test);
+
+  dhcp_start(&net_test);
+
+  fail_unless(txpacket == 1); /* DHCP discover sent */
+  xid = htonl(netif_dhcp_data(&net_test)->xid);
+  memcpy(&dhcp_offer_invalid_overload[46], &xid, 4); /* insert correct transaction id */
+  dhcp_offer_invalid_overload[311] = 3;
+  send_pkt(&net_test, dhcp_offer_invalid_overload, sizeof(dhcp_offer_invalid_overload));
+  /* IP addresses should be zero */
+  fail_if(memcmp(&addr, &net_test.ip_addr, sizeof(ip4_addr_t)));
+  fail_if(memcmp(&netmask, &net_test.netmask, sizeof(ip4_addr_t)));
+  fail_if(memcmp(&gw, &net_test.gw, sizeof(ip4_addr_t)));
+  fail_unless(txpacket == 1); /* Nothing more sent */
+
+  dhcp_offer_invalid_overload[311] = 2;
+  send_pkt(&net_test, dhcp_offer_invalid_overload, sizeof(dhcp_offer_invalid_overload));
+  /* IP addresses should be zero */
+  fail_if(memcmp(&addr, &net_test.ip_addr, sizeof(ip4_addr_t)));
+  fail_if(memcmp(&netmask, &net_test.netmask, sizeof(ip4_addr_t)));
+  fail_if(memcmp(&gw, &net_test.gw, sizeof(ip4_addr_t)));
+  fail_unless(txpacket == 1); /* Nothing more sent */
+
+  dhcp_offer_invalid_overload[311] = 1;
+  send_pkt(&net_test, dhcp_offer_invalid_overload, sizeof(dhcp_offer_invalid_overload));
+  /* IP addresses should be zero */
+  fail_if(memcmp(&addr, &net_test.ip_addr, sizeof(ip4_addr_t)));
+  fail_if(memcmp(&netmask, &net_test.netmask, sizeof(ip4_addr_t)));
+  fail_if(memcmp(&gw, &net_test.gw, sizeof(ip4_addr_t)));
+  fail_unless(txpacket == 1); /* Nothing more sent */
+
+  dhcp_offer_invalid_overload[311] = 0;
+  send_pkt(&net_test, dhcp_offer_invalid_overload, sizeof(dhcp_offer));
+
+  fail_unless(netif_dhcp_data(&net_test)->state == DHCP_STATE_REQUESTING);
+
+  fail_unless(txpacket == 2); /* No more sent */
+  xid = htonl(netif_dhcp_data(&net_test)->xid); /* xid updated */
+
+  netif_remove(&net_test);
+}
+END_TEST
 
 /** Create the suite including all tests for this module */
 Suite *
@@ -913,7 +1019,8 @@ dhcp_suite(void)
     TESTFUNC(test_dhcp),
     TESTFUNC(test_dhcp_nak),
     TESTFUNC(test_dhcp_relayed),
-    TESTFUNC(test_dhcp_nak_no_endmarker)
+    TESTFUNC(test_dhcp_nak_no_endmarker),
+    TESTFUNC(test_dhcp_invalid_overload)
   };
   return create_suite("DHCP", tests, sizeof(tests)/sizeof(testfunc), dhcp_setup, dhcp_teardown);
 }