Skip to content

Commit e2f32d0

Browse files
kstribrnAmznkstribrnAmzn
authored
Increase DNS response validations (#1370)
Verifies the DNS reply matches against the server which was queried. We thank Causal Security (https://causalsecurity.com/) for their help identifying this issue. We thank @htibosch for the extensive help developing source code changes for this fix
1 parent ece06d3 commit e2f32d0

22 files changed

Lines changed: 2443 additions & 233 deletions

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ jobs:
114114
uses: FreeRTOS/CI-CD-Github-Actions/coverage-cop@main
115115
with:
116116
coverage-file: ./test/unit-test/build/coverage.info
117-
branch-coverage-min: 99.8
117+
branch-coverage-min: 99.7
118118
line-coverage-min: 99.9
119119

120120
spell-check:

‎source/FreeRTOS_DNS.c‎

Lines changed: 49 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -655,7 +655,7 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
655655
else
656656
#endif
657657
{
658-
FreeRTOS_printf( ( "prvPrepareLookup: found '%s' in cache: %xip\n", pcHostName, ( unsigned ) ulIPAddress ) );
658+
FreeRTOS_printf( ( "prvPrepareLookup: found '%s' in cache: %xip\n", pcHostName, ( unsigned ) FreeRTOS_ntohl( ulIPAddress ) ) );
659659
}
660660
}
661661
}
@@ -900,6 +900,7 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
900900
case xPreferenceIPv4:
901901
pxAddress->sin_address.ulIP_IPv4 = ipMDNS_IP_ADDRESS; /* Is in network byte order. */
902902
/* sin_family is default set to FREERTOS_AF_INET */
903+
pxAddress->sin_family = FREERTOS_AF_INET4;
903904
break;
904905
#endif /* ( ipconfigUSE_IPv4 != 0 ) */
905906

@@ -1064,16 +1065,18 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
10641065
* will be stored.
10651066
* @param [in] uxIdentifier matches sent and received packets
10661067
* @param [in] usPort Port from which DNS reply was read
1068+
* @param [in] pcHostName the name that was asked for
10671069
* @returns ip address or zero on error
10681070
*
10691071
*/
10701072
static uint32_t prvDNSReply( const struct xDNSBuffer * pxReceiveBuffer,
10711073
struct freertos_addrinfo ** ppxAddressInfo,
10721074
TickType_t uxIdentifier,
1073-
uint16_t usPort )
1075+
uint16_t usPort,
1076+
const char * pcHostName )
10741077
{
10751078
uint32_t ulIPAddress = 0U;
1076-
BaseType_t xExpected;
1079+
BaseType_t xExpected = pdFALSE;
10771080

10781081
/* MISRA Ref 11.3.1 [Misaligned access] */
10791082
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-113 */
@@ -1112,7 +1115,8 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
11121115
pxReceiveBuffer->uxPayloadLength,
11131116
ppxAddressInfo,
11141117
xExpected,
1115-
usPort );
1118+
usPort,
1119+
pcHostName );
11161120
}
11171121

11181122
return ulIPAddress;
@@ -1233,14 +1237,16 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
12331237
* @param[in] xFamily Either FREERTOS_AF_INET4 or FREERTOS_AF_INET6.
12341238
* @param[in] uxReadTimeOut_ticks The timeout in ticks for waiting. In case the user has supplied
12351239
* a call-back function, this value should be zero.
1240+
* @param[out] pxTargetAddress The IP-address of the DNS server that answered the request.
12361241
* @returns ip address or zero on error
12371242
*/
12381243
static uint32_t prvGetHostByNameOp( const char * pcHostName,
12391244
TickType_t uxIdentifier,
12401245
Socket_t xDNSSocket,
12411246
struct freertos_addrinfo ** ppxAddressInfo,
12421247
BaseType_t xFamily,
1243-
TickType_t uxReadTimeOut_ticks )
1248+
TickType_t uxReadTimeOut_ticks,
1249+
IPv46_Address_t * pxTargetAddress )
12441250
{
12451251
uint32_t ulIPAddress = 0;
12461252
struct freertos_sockaddr xAddress;
@@ -1252,9 +1258,27 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
12521258

12531259
/* Make sure all fields of the 'sockaddr' are cleared. */
12541260
( void ) memset( ( void * ) &xAddress, 0, sizeof( xAddress ) );
1261+
( void ) memset( ( void * ) &xRecvAddress, 0, sizeof( xRecvAddress ) );
12551262

12561263
pxEndPoint = prvFillSockAddress( &xAddress, pcHostName );
12571264

1265+
/* 'xFamily' is the record type being asked for, not the address family
1266+
* of the server, so it must not be overwritten here. */
1267+
( void ) memset( pxTargetAddress, 0, sizeof( *pxTargetAddress ) );
1268+
1269+
if( xAddress.sin_family == ( uint8_t ) FREERTOS_AF_INET6 )
1270+
{
1271+
pxTargetAddress->xIs_IPv6 = pdTRUE;
1272+
( void ) memcpy( pxTargetAddress->xIPAddress.xIP_IPv6.ucBytes,
1273+
xAddress.sin_address.xIP_IPv6.ucBytes,
1274+
ipSIZE_OF_IPv6_ADDRESS );
1275+
}
1276+
else
1277+
{
1278+
pxTargetAddress->xIs_IPv6 = pdFALSE;
1279+
pxTargetAddress->xIPAddress.ulIP_IPv4 = xAddress.sin_address.ulIP_IPv4;
1280+
}
1281+
12581282
if( pxEndPoint != NULL )
12591283
{
12601284
do
@@ -1294,7 +1318,8 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
12941318
/* receive a dns reply message */
12951319
xBytes = DNS_ReadReply( xDNSSocket,
12961320
&xRecvAddress,
1297-
&xReceiveBuffer );
1321+
&xReceiveBuffer,
1322+
pxTargetAddress );
12981323

12991324
if( ( uxReadTimeOut_ticks > 0U ) &&
13001325
( ( xBytes == -pdFREERTOS_ERRNO_EWOULDBLOCK ) ||
@@ -1331,7 +1356,7 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
13311356
/* MISRA Ref 4.14.2 [The validity of values received from external sources]. */
13321357
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#directive-414. */
13331358
/* coverity[misra_c_2012_directive_4_14_violation] */
1334-
ulIPAddress = prvDNSReply( &xReceiveBuffer, ppxAddressInfo, uxIdentifier, xRecvAddress.sin_port );
1359+
ulIPAddress = prvDNSReply( &xReceiveBuffer, ppxAddressInfo, uxIdentifier, xRecvAddress.sin_port, pcHostName );
13351360
}
13361361

13371362
/* Finished with the buffer. The zero copy interface
@@ -1362,6 +1387,7 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
13621387
* @param[in] xFamily Either FREERTOS_AF_INET4 or FREERTOS_AF_INET6.
13631388
* @param[in] uxReadTimeOut_ticks The timeout in ticks for waiting. In case the user has supplied
13641389
* a call-back function, this value should be zero.
1390+
* @param[out] pxTargetAddress The IP-address of the DNS server that answered the request.
13651391
* @returns ip address or zero on error
13661392
*
13671393
*/
@@ -1370,7 +1396,8 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
13701396
Socket_t xDNSSocket,
13711397
struct freertos_addrinfo ** ppxAddressInfo,
13721398
BaseType_t xFamily,
1373-
TickType_t uxReadTimeOut_ticks )
1399+
TickType_t uxReadTimeOut_ticks,
1400+
IPv46_Address_t * pxTargetAddress )
13741401
{
13751402
uint32_t ulIPAddress = 0;
13761403
BaseType_t xAttempt;
@@ -1382,7 +1409,8 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
13821409
xDNSSocket,
13831410
ppxAddressInfo,
13841411
xFamily,
1385-
uxReadTimeOut_ticks );
1412+
uxReadTimeOut_ticks,
1413+
pxTargetAddress );
13861414

13871415
if( ulIPAddress != 0U )
13881416
{ /* ip found, no need to retry */
@@ -1416,7 +1444,11 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
14161444
Socket_t xDNSSocket;
14171445
uint32_t ulIPAddress = 0U;
14181446

1447+
/* The IP-address of the DNS server will be stored in
1448+
* 'xTargetAddress'. */
1449+
IPv46_Address_t xTargetAddress;
14191450

1451+
( void ) memset( &( xTargetAddress ), 0, sizeof( xTargetAddress ) );
14201452
xDNSSocket = DNS_CreateSocket( uxReadTimeOut_ticks );
14211453

14221454
if( xDNSSocket != NULL )
@@ -1429,7 +1461,8 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
14291461
xDNSSocket,
14301462
ppxAddressInfo,
14311463
xFamily,
1432-
uxReadTimeOut_ticks );
1464+
uxReadTimeOut_ticks,
1465+
&xTargetAddress );
14331466
}
14341467
else
14351468
{
@@ -1438,7 +1471,8 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
14381471
xDNSSocket,
14391472
ppxAddressInfo,
14401473
xFamily,
1441-
uxReadTimeOut_ticks );
1474+
uxReadTimeOut_ticks,
1475+
&xTargetAddress );
14421476
}
14431477

14441478
/* Finished with the socket. */
@@ -1590,12 +1624,14 @@ const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB
15901624
struct freertos_addrinfo * pxAddressInfo = NULL;
15911625
pucPayLoadBuffer = &( pxNetworkBuffer->pucEthernetBuffer[ uxUDPPacketSize ] );
15921626

1593-
/* The parameter pdFALSE indicates that the reply was not expected. */
1627+
/* NULL: the IP task does not know which look-up a reply
1628+
* belongs to, so xDNSDoCallback() binds it by name instead. */
15941629
( void ) DNS_ParseDNSReply( pucPayLoadBuffer,
15951630
uxPayloadSize,
15961631
&( pxAddressInfo ),
15971632
pdFALSE,
1598-
FreeRTOS_ntohs( pxNetworkBuffer->usPort ) );
1633+
FreeRTOS_ntohs( pxNetworkBuffer->usPort ),
1634+
NULL );
15991635

16001636
if( pxAddressInfo != NULL )
16011637
{

‎source/FreeRTOS_DNS_Callback.c‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,13 @@
4949
* call the handler.
5050
*
5151
* @param[in,out] pxSet a set of variables that are shared among the helper functions.
52-
* @param[in] pxAddress Pointer to address info ( IPv4/IPv6 ) obtained from the DNS server.
53-
*
54-
* @return Returns pdTRUE if uxIdentifier was recognized.
52+
* @param[out] ppvSearchID The search ID of the matching request. Only written
53+
* when a matching request was found.
54+
* @return Returns not NULL if a matching callback function was found.
5555
*/
56-
BaseType_t xDNSDoCallback( ParseSet_t * pxSet,
57-
struct freertos_addrinfo * pxAddress )
56+
FOnDNSEvent xDNSDoCallback( ParseSet_t * pxSet,
57+
void ** ppvSearchID )
5858
{
59-
BaseType_t xResult = pdFALSE;
6059
const ListItem_t * pxIterator;
6160
const ListItem_t * pxEnd = listGET_END_MARKER( &xCallbackList );
6261
TickType_t uxIdentifier = ( TickType_t ) pxSet->pxDNSMessageHeader->usIdentifier;
@@ -87,7 +86,10 @@
8786
else
8887
#endif /* if ( ipconfigUSE_MDNS == 1 ) */
8988
{
90-
xMatching = ( listGET_LIST_ITEM_VALUE( pxIterator ) == uxIdentifier ) ? pdTRUE : pdFALSE;
89+
/* The identifier alone is not enough: every name in a
90+
* reply is supplied by whoever sent it. */
91+
xMatching = ( ( listGET_LIST_ITEM_VALUE( pxIterator ) == uxIdentifier ) &&
92+
( strcasecmp( pxCallback->pcName, pxSet->pcName ) == 0 ) ) ? pdTRUE : pdFALSE;
9193
}
9294

9395
if( xMatching == pdTRUE )
@@ -103,7 +105,6 @@
103105
vIPSetDNSTimerEnableState( pdFALSE );
104106
}
105107

106-
xResult = pdTRUE;
107108
break;
108109
}
109110
}
@@ -112,10 +113,12 @@
112113

113114
if( pCallbackFunction != NULL )
114115
{
115-
pCallbackFunction( pxSet->pcName, pvSearchID, pxAddress );
116+
/* Called once per address record, and the entry is removed on the
117+
* first match, so writing unconditionally would clear it again. */
118+
*( ppvSearchID ) = pvSearchID;
116119
}
117120

118-
return xResult;
121+
return pCallbackFunction;
119122
}
120123

121124
/**

0 commit comments

Comments
 (0)