From 1d9e7c832a001c4e43185f7cafd8378c004ecbfa Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 14 Jan 2020 14:57:49 -0500 Subject: [#71] Addressed review comments Updated release notes relay/tests/relay_unittests.c Cleaned up commentary and other minor changes relay/dhcrelay.c relay/tests/Atffile relay/tests/Makefile.am white space fixes --- RELNOTES | 9 ++++++ relay/dhcrelay.c | 1 - relay/tests/Atffile | 2 +- relay/tests/Makefile.am | 4 +-- relay/tests/relay_unittests.c | 71 ++++++++++++++++++++++--------------------- 5 files changed, 48 insertions(+), 39 deletions(-) diff --git a/RELNOTES b/RELNOTES index c1303d37..0db52abd 100644 --- a/RELNOTES +++ b/RELNOTES @@ -72,6 +72,13 @@ We welcome comments from DHCP users, about this or anything else we do. Email Vicky Risk, Product Manager at vicky@isc.org or discuss on dhcp-users@lists.isc.org. + Changes since 4.1-ESV-R16b1 + +- Corrected buffer pointer logic in dhcrelay functions that manipulate + agent relay options. Thanks to Thomas Imbert of MSRC Vulnerabilities & + Mitigations for reporting the issue. + [#71] + Changes since 4.1-ESV-R15-P1 - Made minor changes to eliminate warnings when compiled with GCC 9. @@ -88,6 +95,8 @@ dhcp-users@lists.isc.org. [Gitlab #2] - Corrected a number of reference counter and zero-length buffer leaks. + Thanks to Christopher Ertl of MSRC Vulnerabilities & Mitigations for + pointing them out. [Gitlab #57] - The option format for the server option omapi-key was changed to a diff --git a/relay/dhcrelay.c b/relay/dhcrelay.c index 1623e098..73fd3054 100644 --- a/relay/dhcrelay.c +++ b/relay/dhcrelay.c @@ -118,7 +118,6 @@ static void setup_streams(void); static void do_relay4(struct interface_info *, struct dhcp_packet *, unsigned int, unsigned int, struct iaddr, struct hardware *); - #endif /* UNIT_TEST */ extern int add_relay_agent_options(struct interface_info *, diff --git a/relay/tests/Atffile b/relay/tests/Atffile index 0572f318..c854582c 100644 --- a/relay/tests/Atffile +++ b/relay/tests/Atffile @@ -1,5 +1,5 @@ Content-Type: application/X-atf-atffile; version="1" -prop: test-suite = dhcrelay +prop: test-suite = dhcrelay tp-glob: *_unittests diff --git a/relay/tests/Makefile.am b/relay/tests/Makefile.am index 8eeda43e..c35f0f45 100644 --- a/relay/tests/Makefile.am +++ b/relay/tests/Makefile.am @@ -21,9 +21,9 @@ DHCPLIBS = $(top_builddir)/common/libdhcp.a \ ATF_TESTS = if HAVE_ATF -ATF_TESTS += relay_unittests +ATF_TESTS += relay_unittests -relay_unittests_SOURCES = $(DHCPSRC) +relay_unittests_SOURCES = $(DHCPSRC) relay_unittests_SOURCES += relay_unittests.c relay_unittests_LDADD = $(ATF_LDFLAGS) diff --git a/relay/tests/relay_unittests.c b/relay/tests/relay_unittests.c index 59320981..dd30bf2f 100644 --- a/relay/tests/relay_unittests.c +++ b/relay/tests/relay_unittests.c @@ -129,7 +129,9 @@ ATF_TC_BODY(strip_relay_agent_options_test, tc) { memset(&packet, 0x0, sizeof(packet)); len = 0; - /* Make sure an empty packet is harmless */ + /* Make sure an empty packet is harmless. We set add_agent_options = 1 + * to avoid early return when it's 0. */ + add_agent_options = 1; ret = strip_relay_agent_options(&ifaces, &matched, &packet, len); if (ret != 0) { atf_tc_fail("empty packet failed"); @@ -140,17 +142,15 @@ ATF_TC_BODY(strip_relay_agent_options_test, tc) { * Uses valid input option data to verify that: * - When add_agent_options is false, the output option data is the * the same as the input option data (i.e. nothing removed) - * - When add_agent_options is true, the relay agent option has - * been removed from the output option data * - When add_agent_options is true, 0 length relay agent option has * been removed from the output option data + * - When add_agent_options is true, a relay agent option has + * been removed from the output option data * */ unsigned char input_data[] = { 0x35, 0x00, /* DHCP_TYPE = DISCOVER */ - 0x52, 0x00, /* Relay Agent Option, len = 0 */ - 0x52, 0x08, 0x01, 0x06, 0x65, /* Relay Agent Option, len = 8 */ 0x6e, 0x70, 0x30, 0x73, 0x4f, @@ -189,12 +189,13 @@ ATF_TC_BODY(strip_relay_agent_options_test, tc) { atf_tc_fail("expected unchanged data, does not match"); } - /* When add_agent_options = 1, it should remove the eight byte */ - /* relay agent option. */ + /* When add_agent_options = 1, it should remove the eight byte + * relay agent option. */ add_agent_options = 1; - /* Because the option data is less is small, the packet should be */ - /* padded out to BOOTP_MIN_LEN */ + /* Beause the inbound option data is less than the BOOTP_MIN_LEN, + * the output data should get padded out to BOOTP_MIN_LEN + * padded out to BOOTP_MIN_LEN */ ret = strip_relay_agent_options(&ifaces, &matched, &packet, len); if (ret != BOOTP_MIN_LEN) { atf_tc_fail("input_data: len of %d, returned %d", @@ -206,14 +207,15 @@ ATF_TC_BODY(strip_relay_agent_options_test, tc) { atf_tc_fail("input_data: expected data does not match"); } - // Now let's repeat it with a relay agent option 0 bytes in length. + /* Now let's repeat it with a relay agent option 0 bytes in length. */ len = set_packet_options(&packet, input_data2, sizeof(input_data2), pad); if (len == 0) { atf_tc_fail("input_data2 set_packet_options failed"); } - /* Because the option data is less is small, the packet should be */ - /* padded out to BOOTP_MIN_LEN */ + /* Because the inbound option data is less than the BOOTP_MIN_LEN, + * the output data should get padded out to BOOTP_MIN_LEN + * padded out to BOOTP_MIN_LEN */ ret = strip_relay_agent_options(&ifaces, &matched, &packet, len); if (ret != BOOTP_MIN_LEN) { atf_tc_fail("input_data2: len of %d, returned %d", @@ -227,26 +229,26 @@ ATF_TC_BODY(strip_relay_agent_options_test, tc) { } { - /* Verify that oversized pack filled with long options does not */ - /* cause overrun */ + /* Verify that oversized packet filled with long options does not + * cause overrun */ /* We borrowed this union from discover.c:got_one() */ union { unsigned char packbuf [4095]; /* Packet input buffer. - Must be as large as largest - possible MTU. */ + * Must be as large as largest + * possible MTU. */ struct dhcp_packet packet; } u; unsigned char input_data[] = { - 0x35, 0x00, // DHCP_TYPE = DISCOVER - 0x52, 0x00, // Relay Agent Option, len = 0 - 0x42, 0x02, 0x00, 0x10, // Opt 0x42, len = 2 - 0x43, 0x00 // Opt 0x43, len = 0 + 0x35, 0x00, /* DHCP_TYPE = DISCOVER */ + 0x52, 0x00, /* Relay Agent Option, len = 0 */ + 0x42, 0x02, 0x00, 0x10, /* Opt 0x42, len = 2 */ + 0x43, 0x00 /* Opt 0x43, len = 0 */ }; - /* We'll pad it 0xFE, that way wherever we hit for "length" we'll */ - /* have length of 254. Increasing the odds we'll push over the end. */ + /* We'll pad it 0xFE, that way wherever we hit for "length" we'll + * have length of 254. Increasing the odds we'll push over the end. */ unsigned char pad = 0xFE; memset(u.packbuf, pad, 4095); @@ -255,10 +257,10 @@ ATF_TC_BODY(strip_relay_agent_options_test, tc) { atf_tc_fail("overrun: set_packet_options failed"); } - // With add_agent_options = 1, it should remove the two byte - // relay agent option. + /* Enable option stripping by setting add_agent_options = 1 */ add_agent_options = 1; + /* strip_relay_agent_options() should detect the overrun and return 0 */ ret = strip_relay_agent_options(&ifaces, &matched, &u.packet, 4095); if (ret != 0) { atf_tc_fail("overrun expected return len = 0, we got %d", ret); @@ -281,7 +283,7 @@ ATF_TC_BODY(add_relay_agent_options_test, tc) { int ret; struct in_addr giaddr; - giaddr.s_addr = inet_addr("192.0.1.1"); + giaddr.s_addr = inet_addr("192.0.1.1"); u_int8_t circuit_id[] = { 0x01,0x02,0x03,0x04,0x05,0x06 }; u_int8_t remote_id[] = { 0x11,0x22,0x33,0x44,0x55,0x66 }; @@ -315,11 +317,8 @@ ATF_TC_BODY(add_relay_agent_options_test, tc) { unsigned char input_data[] = { 0x35, 0x00, /* DHCP_TYPE = DISCOVER */ - 0x52, 0x00, /* Relay Agent Option, len = 0 */ - 0x52, 0x08, 0x01, 0x06, 0x65, /* Relay Agent Option, len = 8 */ 0x6e, 0x70, 0x30, 0x73, 0x4f, - 0x42, 0x02, 0x00, 0x10, /* Opt 0x42, len = 2 */ 0x43, 0x00 /* Opt 0x43, len = 0 */ }; @@ -360,12 +359,13 @@ ATF_TC_BODY(add_relay_agent_options_test, tc) { atf_tc_fail("expected unchanged data, does not match"); } - /* When add_agent_options = 1, it should remove the eight byte */ - /* relay agent option. */ + /* When add_agent_options = 1, it should remove the eight byte + * relay agent option. */ add_agent_options = 1; - /* Because the option data is less is small, the packet should be */ - /* padded out to BOOTP_MIN_LEN */ + /* Because the inbound option data is less than the BOOTP_MIN_LEN, + * the output data should get padded out to BOOTP_MIN_LEN + * padded out to BOOTP_MIN_LEN */ ret = add_relay_agent_options(&ifc, &packet, len, giaddr); if (ret != BOOTP_MIN_LEN) { atf_tc_fail("input_data: len of %d, returned %d", @@ -377,15 +377,16 @@ ATF_TC_BODY(add_relay_agent_options_test, tc) { atf_tc_fail("input_data: expected data does not match"); } - // Now let's repeat it with a relay agent option 0 bytes in length. + /* Now let's repeat it with a relay agent option 0 bytes in length. */ len = set_packet_options(&packet, input_data2, sizeof(input_data2), pad); if (len == 0) { atf_tc_fail("input_data2 set_packet_options failed"); } - /* Because the option data is less is small, the packet should be */ - /* padded out to BOOTP_MIN_LEN */ + /* Because the inbound option data is less than the BOOTP_MIN_LEN, + * the output data should get padded out to BOOTP_MIN_LEN + * padded out to BOOTP_MIN_LEN */ ret = add_relay_agent_options(&ifc, &packet, len, giaddr); if (ret != BOOTP_MIN_LEN) { atf_tc_fail("input_data2: len of %d, returned %d", -- cgit v1.2.1