Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eswifi: Fix possible buffer overflows #63074

Merged
merged 2 commits into from
Oct 10, 2023
+12 −4
Merged

eswifi: Fix possible buffer overflows #63074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d14a398
drivers: eswifi: Fix possible buffer overflow
Sep 25, 2023
ec9f542
drivers: eswifi: shell: Fix possible overflow
Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions drivers/wifi/eswifi/eswifi_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,8 @@ int eswifi_mgmt_iface_status(const struct device *dev,
}

status->state = WIFI_STATE_COMPLETED;
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost certainly not what we want to do, as the result will not ever have a nul termination on it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loicpoulain , do you have any input about this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WiFi controller reports NULL terminated strings and we convert that into what the WiFi susbsystem is expecting, so... it seems ok to merge.

status->band = WIFI_FREQ_BAND_2_4_GHZ;
status->channel = 0;

Expand Down
12 changes: 10 additions & 2 deletions drivers/wifi/eswifi/eswifi_shell.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ static int eswifi_shell_atcmd(const struct shell *sh, size_t argc,
char **argv)
{
int i;
size_t len = 0;

if (eswifi == NULL) {
shell_print(sh, "no eswifi device registered");
Expand All @@ -40,9 +41,16 @@ static int eswifi_shell_atcmd(const struct shell *sh, size_t argc,

memset(eswifi->buf, 0, sizeof(eswifi->buf));
for (i = 1; i < argc; i++) {
strcat(eswifi->buf, argv[i]);
size_t argv_len = strlen(argv[i]);

if ((len + argv_len) >= sizeof(eswifi->buf) - 1) {
break;
}

memcpy(eswifi->buf + len, argv[i], argv_len);
len += argv_len;
}
strcat(eswifi->buf, "\r");
eswifi->buf[len] = '\r';

shell_print(sh, "> %s", eswifi->buf);
eswifi_at_cmd(eswifi, eswifi->buf);
Expand Down