From 7e9f000d0bcc307f4696a34b958759787a130778 Mon Sep 17 00:00:00 2001 From: Henrik Persson Date: Thu, 30 Aug 2012 13:57:33 +0200 Subject: [PATCH] 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. --- src/core/dhcp.c | 10 +++- test/unit/dhcp/test_dhcp.c | 107 ++++++++++++++++++++++++++++++++++++- 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/src/core/dhcp.c b/src/core/dhcp.c index f0f594f1..8b2139da 100644 --- 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 b42c3daf..39bd1575 100644 --- 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); }