Fix lwip_mac_address buffer overflow and set_ip_bytes out of bound access #3191
Conversation
tung7970
changed the title
tung7970
force-pushed
the
fix-mbedos
branch
2 times, most recently
from
cee2b1a to
1cad373
Compare
|
@geky @kjbracey-arm |
| @@ -104,7 +104,7 @@ static void mbed_lwip_socket_callback(struct netconn *nc, enum netconn_evt eh, u | |||
| /* TCP/IP and Network Interface Initialisation */ | |||
| static struct netif lwip_netif; | |||
| static bool lwip_dhcp = false; | |||
| static char lwip_mac_address[NSAPI_MAC_SIZE] = "\0"; | |||
| static char lwip_mac_address[NSAPI_MAC_SIZE + 1] = "\0"; | |||
There was a problem hiding this comment.
+1 is not required - NSAPI_MAC_SIZE appears to already be sized to include the terminator (18). snprintf itself also can just be passed NSAPI_MAC_SIZE, which means it will write a maximum of 17 characters plus a terminator.
So this patch overcompensates - it was an 18-byte buffer with a 19-byte snprintf - this version has it as a 19-byte buffer with an 18-byte printf.
It actually seems wrong that NSAPI_MAC_SIZE even exists at the generic layer. It's up to any individual interface how long a MAC address can be. NanostackInterface returns 23-character MAC addresses.
There was a problem hiding this comment.
Also, while touching this, that existing ="\0" is bizarre, and adds to the terminator confusion. Just ="" or no initialiser at all would do.
There was a problem hiding this comment.
Sounds like NSAPI_MAC_SIZE should just be dropped, I believe it's not actually used anywhere in the nsapi.
There was a problem hiding this comment.
Pushed a new commit to fix buffer size, but left NSAPI_MAC_SIZE untouched for now.
| @@ -204,7 +204,11 @@ void SocketAddress::set_ip_bytes(const void *bytes, nsapi_version_t version) | |||
| { | |||
| nsapi_addr_t addr; | |||
| addr.version = version; | |||
| memcpy(addr.bytes, bytes, NSAPI_IP_BYTES); | |||
| if (version == NSAPI_IPv6) { | |||
There was a problem hiding this comment.
Arguably NSAPI_UNSPEC should copy nothing.
There was a problem hiding this comment.
+1 for @kjbracey-arm's point, there should be three cases here
There was a problem hiding this comment.
Pushed a new commit to do deal with all three cases.
Sounds serious, but should be benign. Signed-off-by: Tony Wu <[email protected]>
set_ip_bytes() does a 16-byte memcpy from the input buffer to the local nsapi_addr_t despite the address version. If the address version is ipv4, the input buffer may only be 4-byte in size. This causes a out-of-bound access on the input buffer. Signed-off-by: Tony Wu <[email protected]>
|
retest uvisor |
1 similar comment
|
retest uvisor |
|
/morph test-nightly |
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 1015 Build Prep failed! |
|
There was a git issue with one of the machines which caused this to fail. Should be fixed now. Restarting! /morph test-nightly |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 1018 All builds and test passed! |
Description
The size of lwip_mac_address is NSAPI_MAC_SIZE, but in mbed_lwip_set_mac_address(), the max size in snprintf is set to 19. This might overflow the lwip_mac_address buffer.
This patch sets the size of lwip_mac_address to NSAPI_MAC_SIZE + 1, and sets snprintf limit to NSAPI_MAC_SIZE.
set_ip_bytes() does a 16-byte memcpy from the input buffer to
the local nsapi_addr_t despite the address version.
If the address version is ipv4, the input buffer may only be
4-byte in size. This causes a out-of-bound access on the input buffer.
Status
READY
Migrations
NO
Deploy notes
NONE