eswifi: Fix possible buffer overflows #63074
Conversation
Limit the number of the copied ssid to WIFI_SSID_MAX_LEN and avoid a possible one byte overflow. Signed-off-by: Flavio Ceolin <[email protected]>
| strcpy(status->ssid, sta->ssid); | ||
| status->ssid_len = strlen(sta->ssid); | ||
| status->ssid_len = strnlen(sta->ssid, WIFI_SSID_MAX_LEN); | ||
| strncpy(status->ssid, sta->ssid, sta->ssid_len); |
There was a problem hiding this comment.
This is almost certainly not what we want to do, as the result will not ever have a nul termination on it.
There was a problem hiding this comment.
Not sure, I saw several places assuming that the string may be not nul terminate and allocating WIFI_SSID_MAX_LEN + 1 to accommodate the \0 e.g:
shell_fprintf(sh, SHELL_NORMAL, "SSID: %-32s\n", status.ssid)
if (params->ssid_length > WIFI_SSID_MAX_LEN) {
other drivers too (e.g drivers/wifi/esp32/src/esp_wifi_drv.c)
strncpy(status->ssid, data->status.ssid, WIFI_SSID_MAX_LEN);
status->ssid_len = strnlen(data->status.ssid, WIFI_SSID_MAX_LEN);
There was a problem hiding this comment.
In theory SSID can have \0 in it so using strncpy() will probably not work as expected. On the other hand I doubt anybody would use \0 in the SSID as it would make connecting quite difficult. So it would make sense to allocate one extra \0 byte at the end of SSID array to make sure the value is null terminated even if the SSID is max length (32 bytes).
There was a problem hiding this comment.
the point is that upper layer is whether or not assuming that ssid is nul terminate ? I didn't dig too much, but my impression is that places manipulating struct wifi_iface_status.ssid are assuming that this is not nul terminate.
There was a problem hiding this comment.
Yes, that should be the case, we should not assume there is \0 at the end. I am just pondering here that it might make sense to add one at the end anyway (just in case).
There was a problem hiding this comment.
Without really now the context and reasons why they are not nul terminate, I'd say that generally speaking this can be a huge source of mistakes since most common APIs to deal with strings expects a nul terminator.
That's said, this is not particular to this driver, I think we should merge this fix (if it is correct)
There was a problem hiding this comment.
@loicpoulain , do you have any input about this?
There was a problem hiding this comment.
IEEE 802.11-2020: 9.4.22
When the UTF-8 SSID subfield of the Extended Capabilities element is equal to 1 in the frame that includes
the SSID element, or the Extended Capabilities of the source of the SSID information is known to include
the UTF-8 SSID capability based on a previously received Extended Capabilities element, the SSID is a
sequence of UTF-8 encoded code points. Otherwise, the character encoding of the octets in this SSID
element is unspecified.
NOTE—If the SSID is a sequence of UTF-8 encoded code points, a terminating null might or might not be present
TL;DR, Null termination of SSID isn't guaranteed, so, if the stored SSID is used as a string it should be ssid[33] to accomodate NULL character.
There was a problem hiding this comment.
The WiFi controller reports NULL terminated strings and we convert that into what the WiFi susbsystem is expecting, so... it seems ok to merge.
Limit the copied data to the buffer's size. Signed-off-by: Flavio Ceolin <[email protected]>
| strcpy(status->ssid, sta->ssid); | ||
| status->ssid_len = strlen(sta->ssid); | ||
| status->ssid_len = strnlen(sta->ssid, WIFI_SSID_MAX_LEN); | ||
| strncpy(status->ssid, sta->ssid, sta->ssid_len); |
There was a problem hiding this comment.
@loicpoulain , do you have any input about this?
ceolin
dismissed
d3zd3z’s stale review
the concern about non-null terminated strings was explained.
|
I'm seeing this CI failure in #63703 |
Fix buffer overflows when copying ssid.