Possible buffer overflow in WiFi scan results

[adrian-dybwad]

I noticed a strange output when scanning networks and outputting the list. It happens when there is a network present with an SSID of 32 characters long (the maximum SSID length).

The output of the included MCVE sketch is below - note item 6 with three strange characters before the comma:

SDK:3.0.0-dev(c0f7b44)/Core:2.5.0=20500000/lwIP:STABLE-2_1_2_RELEASE/glue:1.1/BearSSL:6778687
mode : sta + softAP
add if0
scandone
1: , Ch:1 (-53dBm)  hidden
2: NetworkProvidersInc, Ch:1 (-52dBm)  
3: CBCI-0D1E-2.4, Ch:1 (-71dBm)  
4: TEST-STA, Ch:1 (-45dBm)  
5: PA, Ch:1 (-46dBm)  
6: 15167f140275ad599442f0385758d2fc �⸮, Ch:1 (-50dBm)  
7: DIRECT-0A-HP OfficeJet Pro 8740, Ch:1 (-76dBm)  
8: PoodleBall, Ch:1 (-80dBm)  
9: DIRECT-D3F52C6E, Ch:6 (-71dBm)  
10: NETGEAR57, Ch:6 (-45dBm)  
11: CBCI-8F08, Ch:6 (-35dBm)  

Sketch:

#include <ESP8266WiFi.h>
void setup() {
  Serial.begin(115200);
  // put your setup code here, to run once:
int n = WiFi.scanNetworks(false, true);

String ssid;
uint8_t encryptionType;
int32_t RSSI;
uint8_t* BSSID;
int32_t channel;
bool isHidden;

for (int i = 0; i < n; i++)
{
  WiFi.getNetworkInfo(i, ssid, encryptionType, RSSI, BSSID, channel, isHidden);
  Serial.printf("%d: %s, Ch:%d (%ddBm) %s %s\n", i + 1, ssid.c_str(), channel, RSSI, encryptionType == ENC_TYPE_NONE ? "open" : "", isHidden ? "hidden" : "");
}
}

void loop() {
  // put your main code here, to run repeatedly:

}

Since this looks like a buffer overflow, I thought it was important to point it out!

[adrian-dybwad]

PS, the sketch was created from the example code at https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/scan-class.html

[liborp]

I bumped in this issue several days ago. May be the problem occurs during the reading from the ssid or WiFi.SSID(i) because several times happened to me, that the first or first two rows returned correct value but the next one not:

Serial.println(WiFi.SSID(i));
Serial.println(WiFi.SSID(i));
Serial.println(WiFi.SSID(i));

I hope it could help to find the error.

[earlephilhower]

From the behavior this isn't a buffer overflow, it's an unterminated C string.

Seems like the ROM is doing a strncpy(dest, src, 32) and because the AP ID is 32 chars long there is no space for a trailing 0.

Arduino/libraries/ESP8266WiFi/src/ESP8266WiFiScan.cpp

Line 168 in 192aaa4

ssid = (const char*) it->ssid;

Needs to be updated to copy the 32-chars in the bss struct and 0-terminate it locally before assigning to the returned String.

[TD-er]

I just noticed a similar commit/bugfix mentioned while browsing through this list: #5873

So let's hope not too much time will be lost here if it is also fixed in the SDK 2.2.2
Even though it may be wise to copy the bytes first like suggested.

[kapyaar]

Is n't this relevant?

https://serverfault.com/questions/45439/what-is-the-maximum-length-of-a-wifi-access-points-ssid

[adrian-dybwad]

Is n't this relevant?

https://serverfault.com/questions/45439/what-is-the-maximum-length-of-a-wifi-access-points-ssid

Yes, it is. But the max length of an SSID is 32 characters and this one is that. Hence the reason to see it as a (potential) problem. In any way, seeing strange characters like this should never be ignored since it means there is a problem and could involve memory corruption or buffer overflows.

[earlephilhower]

I've just pushed a PR that should 0-terminate any 32 byte long SSIDs. I can't make one that long on my router, though, so can't test it myself. Logically, the patch is straightforward, though (famous last words).

@adrionics can you give PR #5889 a test since you seem to have a 32byte long SSID?

[adrian-dybwad]

earlephilhower, that does indeed fix it. Would there be any other situations that use the it->ssid property expecting it to be null terminated?

Also, would it be better to do the following (using the sizeof(it->ssid) to set the null caracter):

memcpy(ssid_copy, it->ssid, sizeof(it->ssid));
ssid_copy[sizeof(it->ssid)] = 0; // Potentially add 0-termination if none present earlier

instead of

memcpy(ssid_copy, it->ssid, sizeof(it->ssid));
ssid_copy[32] = 0; // Potentially add 0-termination if none present earlier