Skip to content

Commit 51d5dde

Browse files
htiboschHein TiboschAniruddhaKanhere
authored
IPv4: Add length and type checks in ARP packet processing (#330)
* IPv4: Add length and type checks in ARP packet processing * Update unit-tests to include newly added branches * Uncrustified Co-authored-by: Hein Tibosch <hein@htibosch.net> Co-authored-by: Aniruddha Kanhere <kanherea@amazon.com>
1 parent fe97a2e commit 51d5dde

File tree

3 files changed

+214
-84
lines changed

3 files changed

+214
-84
lines changed

‎FreeRTOS_ARP.c‎

Lines changed: 98 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -110,111 +110,125 @@ eFrameProcessingResult_t eARPProcessPacket( ARPPacket_t * const pxARPFrame )
110110

111111
pxARPHeader = &( pxARPFrame->xARPHeader );
112112

113-
/* The field ulSenderProtocolAddress is badly aligned, copy byte-by-byte. */
114-
115-
/*
116-
* Use helper variables for memcpy() to remain
117-
* compliant with MISRA Rule 21.15. These should be
118-
* optimized away.
119-
*/
120-
pvCopySource = pxARPHeader->ucSenderProtocolAddress;
121-
pvCopyDest = &ulSenderProtocolAddress;
122-
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( ulSenderProtocolAddress ) );
123-
/* The field ulTargetProtocolAddress is well-aligned, a 32-bits copy. */
124-
ulTargetProtocolAddress = pxARPHeader->ulTargetProtocolAddress;
125-
126-
traceARP_PACKET_RECEIVED();
127-
128-
/* Don't do anything if the local IP address is zero because
129-
* that means a DHCP request has not completed. */
130-
if( *ipLOCAL_IP_ADDRESS_POINTER != 0UL )
113+
/* Only Ethernet hardware type is supported.
114+
* Only IPv4 address can be present in the ARP packet.
115+
* The hardware length (the MAC address) must be 6 bytes. And,
116+
* The Protocol address length must be 4 bytes as it is IPv4. */
117+
if( ( pxARPHeader->usHardwareType == ipARP_HARDWARE_TYPE_ETHERNET ) &&
118+
( pxARPHeader->usProtocolType == ipARP_PROTOCOL_TYPE ) &&
119+
( pxARPHeader->ucHardwareAddressLength == ipMAC_ADDRESS_LENGTH_BYTES ) &&
120+
( pxARPHeader->ucProtocolAddressLength == ipIP_ADDRESS_LENGTH_BYTES ) )
131121
{
132-
switch( pxARPHeader->usOperation )
122+
/* The field ulSenderProtocolAddress is badly aligned, copy byte-by-byte. */
123+
124+
/*
125+
* Use helper variables for memcpy() to remain
126+
* compliant with MISRA Rule 21.15. These should be
127+
* optimized away.
128+
*/
129+
pvCopySource = pxARPHeader->ucSenderProtocolAddress;
130+
pvCopyDest = &ulSenderProtocolAddress;
131+
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( ulSenderProtocolAddress ) );
132+
/* The field ulTargetProtocolAddress is well-aligned, a 32-bits copy. */
133+
ulTargetProtocolAddress = pxARPHeader->ulTargetProtocolAddress;
134+
135+
traceARP_PACKET_RECEIVED();
136+
137+
/* Don't do anything if the local IP address is zero because
138+
* that means a DHCP request has not completed. */
139+
if( *ipLOCAL_IP_ADDRESS_POINTER != 0UL )
133140
{
134-
case ipARP_REQUEST:
135-
136-
/* The packet contained an ARP request. Was it for the IP
137-
* address of the node running this code? */
138-
if( ulTargetProtocolAddress == *ipLOCAL_IP_ADDRESS_POINTER )
139-
{
140-
iptraceSENDING_ARP_REPLY( ulSenderProtocolAddress );
141+
switch( pxARPHeader->usOperation )
142+
{
143+
case ipARP_REQUEST:
141144

142-
/* The request is for the address of this node. Add the
143-
* entry into the ARP cache, or refresh the entry if it
144-
* already exists. */
145-
vARPRefreshCacheEntry( &( pxARPHeader->xSenderHardwareAddress ), ulSenderProtocolAddress );
145+
/* The packet contained an ARP request. Was it for the IP
146+
* address of the node running this code? */
147+
if( ulTargetProtocolAddress == *ipLOCAL_IP_ADDRESS_POINTER )
148+
{
149+
iptraceSENDING_ARP_REPLY( ulSenderProtocolAddress );
146150

147-
/* Generate a reply payload in the same buffer. */
148-
pxARPHeader->usOperation = ( uint16_t ) ipARP_REPLY;
151+
/* The request is for the address of this node. Add the
152+
* entry into the ARP cache, or refresh the entry if it
153+
* already exists. */
154+
vARPRefreshCacheEntry( &( pxARPHeader->xSenderHardwareAddress ), ulSenderProtocolAddress );
149155

150-
if( ulTargetProtocolAddress == ulSenderProtocolAddress )
151-
{
152-
/* A double IP address is detected! */
153-
/* Give the sources MAC address the value of the broadcast address, will be swapped later */
156+
/* Generate a reply payload in the same buffer. */
157+
pxARPHeader->usOperation = ( uint16_t ) ipARP_REPLY;
154158

155-
/*
156-
* Use helper variables for memcpy() to remain
157-
* compliant with MISRA Rule 21.15. These should be
158-
* optimized away.
159-
*/
160-
pvCopySource = xBroadcastMACAddress.ucBytes;
161-
pvCopyDest = pxARPFrame->xEthernetHeader.xSourceAddress.ucBytes;
162-
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( xBroadcastMACAddress ) );
159+
if( ulTargetProtocolAddress == ulSenderProtocolAddress )
160+
{
161+
/* A double IP address is detected! */
162+
/* Give the sources MAC address the value of the broadcast address, will be swapped later */
163+
164+
/*
165+
* Use helper variables for memcpy() to remain
166+
* compliant with MISRA Rule 21.15. These should be
167+
* optimized away.
168+
*/
169+
pvCopySource = xBroadcastMACAddress.ucBytes;
170+
pvCopyDest = pxARPFrame->xEthernetHeader.xSourceAddress.ucBytes;
171+
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( xBroadcastMACAddress ) );
172+
173+
( void ) memset( pxARPHeader->xTargetHardwareAddress.ucBytes, 0, sizeof( MACAddress_t ) );
174+
pxARPHeader->ulTargetProtocolAddress = 0UL;
175+
}
176+
else
177+
{
178+
/*
179+
* Use helper variables for memcpy() to remain
180+
* compliant with MISRA Rule 21.15. These should be
181+
* optimized away.
182+
*/
183+
pvCopySource = pxARPHeader->xSenderHardwareAddress.ucBytes;
184+
pvCopyDest = pxARPHeader->xTargetHardwareAddress.ucBytes;
185+
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( MACAddress_t ) );
186+
pxARPHeader->ulTargetProtocolAddress = ulSenderProtocolAddress;
187+
}
163188

164-
( void ) memset( pxARPHeader->xTargetHardwareAddress.ucBytes, 0, sizeof( MACAddress_t ) );
165-
pxARPHeader->ulTargetProtocolAddress = 0UL;
166-
}
167-
else
168-
{
169189
/*
170190
* Use helper variables for memcpy() to remain
171191
* compliant with MISRA Rule 21.15. These should be
172192
* optimized away.
173193
*/
174-
pvCopySource = pxARPHeader->xSenderHardwareAddress.ucBytes;
175-
pvCopyDest = pxARPHeader->xTargetHardwareAddress.ucBytes;
194+
pvCopySource = ipLOCAL_MAC_ADDRESS;
195+
pvCopyDest = pxARPHeader->xSenderHardwareAddress.ucBytes;
176196
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( MACAddress_t ) );
177-
pxARPHeader->ulTargetProtocolAddress = ulSenderProtocolAddress;
178-
}
197+
pvCopySource = ipLOCAL_IP_ADDRESS_POINTER;
198+
pvCopyDest = pxARPHeader->ucSenderProtocolAddress;
199+
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( pxARPHeader->ucSenderProtocolAddress ) );
179200

180-
/*
181-
* Use helper variables for memcpy() to remain
182-
* compliant with MISRA Rule 21.15. These should be
183-
* optimized away.
184-
*/
185-
pvCopySource = ipLOCAL_MAC_ADDRESS;
186-
pvCopyDest = pxARPHeader->xSenderHardwareAddress.ucBytes;
187-
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( MACAddress_t ) );
188-
pvCopySource = ipLOCAL_IP_ADDRESS_POINTER;
189-
pvCopyDest = pxARPHeader->ucSenderProtocolAddress;
190-
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( pxARPHeader->ucSenderProtocolAddress ) );
191-
192-
eReturn = eReturnEthernetFrame;
193-
}
201+
eReturn = eReturnEthernetFrame;
202+
}
194203

195-
break;
204+
break;
196205

197-
case ipARP_REPLY:
198-
iptracePROCESSING_RECEIVED_ARP_REPLY( ulTargetProtocolAddress );
199-
vARPRefreshCacheEntry( &( pxARPHeader->xSenderHardwareAddress ), ulSenderProtocolAddress );
200-
/* Process received ARP frame to see if there is a clash. */
201-
#if ( ipconfigARP_USE_CLASH_DETECTION != 0 )
202-
{
203-
if( ulSenderProtocolAddress == *ipLOCAL_IP_ADDRESS_POINTER )
206+
case ipARP_REPLY:
207+
iptracePROCESSING_RECEIVED_ARP_REPLY( ulTargetProtocolAddress );
208+
vARPRefreshCacheEntry( &( pxARPHeader->xSenderHardwareAddress ), ulSenderProtocolAddress );
209+
/* Process received ARP frame to see if there is a clash. */
210+
#if ( ipconfigARP_USE_CLASH_DETECTION != 0 )
204211
{
205-
xARPHadIPClash = pdTRUE;
206-
/* Remember the MAC-address of the other device which has the same IP-address. */
207-
( void ) memcpy( xARPClashMacAddress.ucBytes, pxARPHeader->xSenderHardwareAddress.ucBytes, sizeof( xARPClashMacAddress.ucBytes ) );
212+
if( ulSenderProtocolAddress == *ipLOCAL_IP_ADDRESS_POINTER )
213+
{
214+
xARPHadIPClash = pdTRUE;
215+
/* Remember the MAC-address of the other device which has the same IP-address. */
216+
( void ) memcpy( xARPClashMacAddress.ucBytes, pxARPHeader->xSenderHardwareAddress.ucBytes, sizeof( xARPClashMacAddress.ucBytes ) );
217+
}
208218
}
209-
}
210-
#endif /* ipconfigARP_USE_CLASH_DETECTION */
211-
break;
219+
#endif /* ipconfigARP_USE_CLASH_DETECTION */
220+
break;
212221

213-
default:
214-
/* Invalid. */
215-
break;
222+
default:
223+
/* Invalid. */
224+
break;
225+
}
216226
}
217227
}
228+
else
229+
{
230+
iptraceDROPPED_INVALID_ARP_PACKET( pxARPHeader );
231+
}
218232

219233
return eReturn;
220234
}

‎include/IPTraceMacroDefaults.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@
6464
#define iptraceFAILED_TO_OBTAIN_NETWORK_BUFFER_FROM_ISR()
6565
#endif
6666

67+
#ifndef iptraceDROPPED_INVALID_ARP_PACKET
68+
#define iptraceDROPPED_INVALID_ARP_PACKET( pxARPHeader )
69+
#endif
70+
6771
#ifndef iptraceCREATING_ARP_REQUEST
6872
#define iptraceCREATING_ARP_REQUEST( ulIPAddress )
6973
#endif

‎test/unit-test/FreeRTOS_ARP/FreeRTOS_ARP_utest.c‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,13 +141,89 @@ void test_xCheckLoopback_SendEventToIPTaskFails( void )
141141
/* =================================================== */
142142
}
143143

144+
void test_eARPProcessPacket_DifferentHardwareAddress( void )
145+
{
146+
ARPPacket_t xARPFrame;
147+
eFrameProcessingResult_t eResult;
148+
149+
/* =================================================== */
150+
/* Different Hardware address. */
151+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET + 1;
152+
153+
/* When the local IP address is 0, we should not process any ARP Packets. */
154+
*ipLOCAL_IP_ADDRESS_POINTER = 0UL;
155+
eResult = eARPProcessPacket( &xARPFrame );
156+
TEST_ASSERT_EQUAL( eReleaseBuffer, eResult );
157+
/* =================================================== */
158+
}
159+
160+
void test_eARPProcessPacket_DifferentProtocolType( void )
161+
{
162+
ARPPacket_t xARPFrame;
163+
eFrameProcessingResult_t eResult;
164+
165+
/* =================================================== */
166+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
167+
/* Different protocol type */
168+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE + 1;
169+
170+
/* When the local IP address is 0, we should not process any ARP Packets. */
171+
*ipLOCAL_IP_ADDRESS_POINTER = 0UL;
172+
eResult = eARPProcessPacket( &xARPFrame );
173+
TEST_ASSERT_EQUAL( eReleaseBuffer, eResult );
174+
/* =================================================== */
175+
}
176+
177+
void test_eARPProcessPacket_DifferentHardwareLength( void )
178+
{
179+
ARPPacket_t xARPFrame;
180+
eFrameProcessingResult_t eResult;
181+
182+
/* =================================================== */
183+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
184+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
185+
/* Different MAC address length. */
186+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES + 1;
187+
188+
/* When the local IP address is 0, we should not process any ARP Packets. */
189+
*ipLOCAL_IP_ADDRESS_POINTER = 0UL;
190+
eResult = eARPProcessPacket( &xARPFrame );
191+
TEST_ASSERT_EQUAL( eReleaseBuffer, eResult );
192+
/* =================================================== */
193+
}
194+
195+
void test_eARPProcessPacket_DifferentProtocolLength( void )
196+
{
197+
ARPPacket_t xARPFrame;
198+
eFrameProcessingResult_t eResult;
199+
200+
/* =================================================== */
201+
/* Add settings required for ARP header to pass checks */
202+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
203+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
204+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
205+
/* Different protocol length. */
206+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES + 1;
207+
208+
/* When the local IP address is 0, we should not process any ARP Packets. */
209+
*ipLOCAL_IP_ADDRESS_POINTER = 0UL;
210+
eResult = eARPProcessPacket( &xARPFrame );
211+
TEST_ASSERT_EQUAL( eReleaseBuffer, eResult );
212+
/* =================================================== */
213+
}
144214

145215
void test_eARPProcessPacket_LocalIPisZero( void )
146216
{
147217
ARPPacket_t xARPFrame;
148218
eFrameProcessingResult_t eResult;
149219

150220
/* =================================================== */
221+
/* Add settings required for ARP header to pass checks */
222+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
223+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
224+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
225+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
226+
151227
/* When the local IP address is 0, we should not process any ARP Packets. */
152228
*ipLOCAL_IP_ADDRESS_POINTER = 0UL;
153229
eResult = eARPProcessPacket( &xARPFrame );
@@ -161,6 +237,12 @@ void test_eARPProcessPacket_InvalidOperation( void )
161237
eFrameProcessingResult_t eResult;
162238

163239
/* =================================================== */
240+
/* Add settings required for ARP header to pass checks */
241+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
242+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
243+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
244+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
245+
164246
/* What is some invalid option is sent in the ARP Packet? */
165247
*ipLOCAL_IP_ADDRESS_POINTER = 0xAABBCCDD;
166248
/* Add invalid operation */
@@ -175,6 +257,12 @@ void test_eARPProcessPacket_Request_DifferentIP( void )
175257
eFrameProcessingResult_t eResult;
176258

177259
/* Process an ARP request, but not meant for this node. */
260+
/* Add settings required for ARP header to pass checks */
261+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
262+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
263+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
264+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
265+
178266
*ipLOCAL_IP_ADDRESS_POINTER = 0xAABBCCDD;
179267
/* Fill in the request option. */
180268
xARPFrame.xARPHeader.usOperation = ipARP_REQUEST;
@@ -190,6 +278,12 @@ void test_eARPProcessPacket_Request_SenderAndTargetDifferent( void )
190278
eFrameProcessingResult_t eResult;
191279

192280
/* =================================================== */
281+
/* Add settings required for ARP header to pass checks */
282+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
283+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
284+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
285+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
286+
193287
/* Process an ARP request - meant for this node with target and source different. */
194288
*ipLOCAL_IP_ADDRESS_POINTER = 0xAABBCCDD;
195289
/* Fill in the request option. */
@@ -209,6 +303,12 @@ void test_eARPProcessPacket_Request_SenderAndTargetSame( void )
209303
eFrameProcessingResult_t eResult;
210304

211305
/* =================================================== */
306+
/* Add settings required for ARP header to pass checks */
307+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
308+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
309+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
310+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
311+
212312
/* Process an ARP request - meant for this node with target and source same. */
213313
*ipLOCAL_IP_ADDRESS_POINTER = 0xAABBCCDD;
214314
/* Fill in the request option. */
@@ -226,6 +326,12 @@ void test_eARPProcessPacket_Reply_SenderAndTargetSame( void )
226326
eFrameProcessingResult_t eResult;
227327

228328
/* =================================================== */
329+
/* Add settings required for ARP header to pass checks */
330+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
331+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
332+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
333+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
334+
229335
/* Process an ARP reply - meant for this node with target and source same. */
230336
*ipLOCAL_IP_ADDRESS_POINTER = 0xAABBCCDD;
231337
/* Fill in the request option. */
@@ -243,6 +349,12 @@ void test_eARPProcessPacket_Reply_DifferentIP( void )
243349
eFrameProcessingResult_t eResult;
244350

245351
/* =================================================== */
352+
/* Add settings required for ARP header to pass checks */
353+
xARPFrame.xARPHeader.usHardwareType = ipARP_HARDWARE_TYPE_ETHERNET;
354+
xARPFrame.xARPHeader.usProtocolType = ipARP_PROTOCOL_TYPE;
355+
xARPFrame.xARPHeader.ucHardwareAddressLength = ipMAC_ADDRESS_LENGTH_BYTES;
356+
xARPFrame.xARPHeader.ucProtocolAddressLength = ipIP_ADDRESS_LENGTH_BYTES;
357+
246358
/* Process an ARP reply - not meant for this node. */
247359
*ipLOCAL_IP_ADDRESS_POINTER = 0xAABBCCDD;
248360
/* Fill in the request option. */

0 commit comments

Comments
 (0)