Skip to content

Commit

Permalink
unit-test of DHCP option parser: Delete the buffer overflow
Browse files Browse the repository at this point in the history 
Symptom:

    AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffc9dfa5c07
    READ of size 1 at 0x7ffc9dfa5c07 thread T0
    #0 0x459a49 in prvProcessDHCPReplies test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:1310
    FreeRTOS#1 0x4526d2 in vHandleWaitingAcknowledge test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:495
    FreeRTOS#2 0x4544ef in vDHCPProcessEndPoint test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:739
    FreeRTOS#3 0x43dbd9 in test_vDHCPProcess_eWaitingAcknowledge_DNSIncorrectLength2
        (test/unit-test/build/bin/tests/FreeRTOS_DHCP_utest+0x43dbd9)

    Address 0x7ffc9dfa5c07 is located in stack of thread T0
    SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow
        test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:1310 in prvProcessDHCPReplies
    Shadow bytes around the buggy address:
    0x7ffc9dfa5980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5a80: 00 00 00 00 00 00 00 00 ca ca ca ca 00 00 00 00
    0x7ffc9dfa5b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    =>0x7ffc9dfa5c00:[05]cb cb cb cb cb cb cb 00 00 00 00 00 00 00 00
    0x7ffc9dfa5c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x7ffc9dfa5e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    Shadow byte legend (one shadow byte represents 8 application bytes):
    Addressable:           00
    Partially addressable: 01 02 03 04 05 06 07
    Left alloca redzone:   ca
    Right alloca redzone:  cb

There were two problems that conspired to create this segfault. The first
was allowing the option parser to run off the end of the buffer at all:

    /* ulGenericLength is incremented by 100 to have uxDNSCount > ipconfigENDPOINT_DNS_ADDRESS_COUNT scenario */
    ulGenericLength = sizeof( DHCPMsg ) + 100;

The second problem was letting it overshoot the stop byte (0xFF).
Which is a problem with having manually updated indexes and length fields.
The stop byte was at the end of the buffer, but was of no help, because
the buffer length was off by -3 (missing 2 bytes for the opcode and
length field of the 6 server addresses and 1 byte to account for an
unexplained hole in the serialized stream).

The real fix for this kind of fragility is using some helper funcitons
for serializing the data while keeping indexes and lenghts consistent
(not to mention collapsing repeated lines 8-fold).
Anyway, it is trivial to add a check that the serialized stream ends
at the end of the buffer (done).
Whether to add a functioning stop byte does not matter
and should not be needed anymore with such a check.

I initially fixed it the wrong way, by keeping it within the same buffer,
which hurt line coverage. But what the test wants to test is as commented:
At least 6 server addresses, because that's the value of
ipconfigENDPOINT_DNS_ADDRESS_COUNT + 1. No "invalid" length required,
just an overabundance of DNS servers. As such, let's rename the test.

Btw, the test was using a VLA (fixed), and most of the uint32_t writes
are still unaligned (I replaced one of them with memcpy).
  • Loading branch information
@anordal
anordal committed Jun 4, 2024
1 parent 8238005 commit fba335f
Showing 1 changed file with 26 additions and 22 deletions.
48 changes: 26 additions & 22 deletions test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4363,33 +4363,29 @@ void test_vDHCPProcess_eWaitingAcknowledge_DNSIncorrectLength( void )
TEST_ASSERT_EQUAL( xIPv4Addressing->ulNetMask, ulSubnetMask );
}

void test_vDHCPProcess_eWaitingAcknowledge_DNSIncorrectLength2( void )
void test_vDHCPProcess_eWaitingAcknowledge_DNSServerOverabundance( void )
{
struct xSOCKET xTestSocket;
TickType_t xTimeValue = 1234;

/* Create a bit longer DHCP message but keep it empty. */
const BaseType_t xTotalLength = sizeof( struct xDHCPMessage_IPv4 ) + 1U /* Padding */
+ 3U /* DHCP offer */
+ 6U /* Server IP address */
+ 6U /* Subnet Mask */
+ 6U /* Gateway */
+ 6U /* Lease time */
+ 24U /* DNS server */
+ 1U /* End */;
uint8_t DHCPMsg[ xTotalLength ];
uint32_t DHCPServerAddress = 0xC0A80001; /* 192.168.0.1 */
uint32_t ulClientIPAddress = 0xC0A8000A; /* 192.168.0.10 */
uint32_t ulSubnetMask = 0xFFFFF100; /* 255.255.241.0 */
uint32_t ulGateway = 0xC0A80001; /* 192.168.0.1 */
uint32_t ulLeaseTime = 0x00000096; /* 150 seconds */
uint32_t ulDNSServer = 0xC0010101; /* 192.1.1.1 */
uint8_t DHCPMsg[
sizeof( DHCPMessage_IPv4_t )
+ 2U + sizeof( ( uint8_t ) dhcpMESSAGE_TYPE_ACK )
+ 2U + sizeof( DHCPServerAddress )
+ 2U + sizeof( ulSubnetMask )
+ 2U + sizeof( ulGateway )
+ 2U + sizeof( ulLeaseTime )
+ 2U + sizeof( ulDNSServer ) * ( ipconfigENDPOINT_DNS_ADDRESS_COUNT + 1 )
];
DHCPMessage_IPv4_t * pxDHCPMessage = ( DHCPMessage_IPv4_t * ) DHCPMsg;
NetworkEndPoint_t xEndPoint = { 0 }, * pxEndPoint = &xEndPoint;
IPV4Parameters_t * xIPv4Addressing = &( pxEndPoint->ipv4_settings );

DHCPMsg[ xTotalLength - 1U ] = 0xFF;

size_t xDNSServersToAdd = ipconfigENDPOINT_DNS_ADDRESS_COUNT + 1;

/* Set the header - or at least the start of DHCP message. */
memset( DHCPMsg, 0, sizeof( DHCPMsg ) );
Expand All @@ -4404,16 +4400,16 @@ void test_vDHCPProcess_eWaitingAcknowledge_DNSIncorrectLength2( void )
/* Set the client IP address. */
pxDHCPMessage->ulYourIPAddress_yiaddr = ulClientIPAddress;

/* Leave one byte for the padding. */
uint8_t * DHCPOption = &DHCPMsg[ sizeof( struct xDHCPMessage_IPv4 ) + 1 ];
uint8_t * DHCPOption = &DHCPMsg[ sizeof( DHCPMessage_IPv4_t ) ];

/* Add Message type code. */
DHCPOption[ 0 ] = dhcpIPv4_MESSAGE_TYPE_OPTION_CODE;
/* Add length. */
DHCPOption[ 1 ] = 1;
/* Add the offer byte. */
DHCPOption[ 2 ] = dhcpMESSAGE_TYPE_ACK;

DHCPOption += 4;
DHCPOption += 3;
/* Add Message type code. */
DHCPOption[ 0 ] = dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE;
/* Add length. */
Expand Down Expand Up @@ -4449,15 +4445,23 @@ void test_vDHCPProcess_eWaitingAcknowledge_DNSIncorrectLength2( void )
/* Add Message type code. */
DHCPOption[ 0 ] = dhcpIPv4_DNS_SERVER_OPTIONS_CODE;
/* Add length. */
DHCPOption[ 1 ] = 24;
/* Add the offer byte. */
*( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulDNSServer;
DHCPOption[ 1 ] = xDNSServersToAdd * sizeof( ulDNSServer );
DHCPOption += 2;

while( xDNSServersToAdd-- > 0U )
{
memcpy( DHCPOption, &ulDNSServer, sizeof( ulDNSServer ) );
DHCPOption += sizeof( ulDNSServer );
}

/* A stop byte shall not be necessary to prevent the DHCP option parser from running off the end of the buffer. */
/* *DHCPOption++ = 0xFF; */
TEST_ASSERT_EQUAL( DHCPOption - DHCPMsg, sizeof( DHCPMsg ) );

/* Put the information in global variables to be returned by
* the FreeRTOS_recvrom. */
ucGenericPtr = DHCPMsg;
ulGenericLength = sizeof( DHCPMsg ) + 100; /* ulGenericLength is incremented by 100 to have uxDNSCount > ipconfigENDPOINT_DNS_ADDRESS_COUNT scenario */
ulGenericLength = sizeof( DHCPMsg );

/* This should remain unchanged. */
xDHCPv4Socket = &xTestSocket;
Expand Down

0 comments on commit fba335f

Please sign in to comment.