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

The first ingredient 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 ingredient was letting it overshoot the stop byte (0xFF).
This can be attributed to lack of control of its placement relative to
the end of the serialized stream, let alone the end of the buffer:
The stop byte was at the end of the buffer, but 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).

It tempts me to introduce some helper functions to help with consistency
(not to mention collapsing repeated lines 8-fold).

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.
Whether to add a functioning stop byte does not matter. It is trivial
to add a check that the serialized stream ends at the end of the buffer.

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 May 26, 2024
1 parent 5c43427 commit 3f8f464
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 3f8f464

Please sign in to comment.