Skip to content

SSID wrong lenght validator #4911

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.

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

Closed
mictlan666 opened this issue Jul 9, 2018 · 5 comments
Closed

SSID wrong lenght validator #4911

mictlan666 opened this issue Jul 9, 2018 · 5 comments
Assignees
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Milestone

Comments

@mictlan666
Copy link

mictlan666 commented Jul 9, 2018

Hello,
In file: https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp
Method: wl_status_t ESP8266WiFiSTAClass::begin(const char* ssid, const char *passphrase, int32_t channel, const uint8_t* bssid, bool connect)
Line: 107
You have that condition:

  if(!ssid || *ssid == 0x00 || strlen(ssid) > 31) {
        // fail SSID too long or missing!
        return WL_CONNECT_FAILED;
}

This is not agree to IEEE 802.11 standard (https://tools.ietf.org/html/rfc3770#section-4)
SSID could have 32 octets, you checking by strlen so 31 octets with NULL in the end will the longest SSID in your case.
I sure about this because of my local wifi network is md5 hash so has 32 octets.
Dont work when i rewrite to 32 const array: char ssid[32];
You should change condition to strlen(ssid) > 32 OR rewrite first 32 octets (or to NULL) to char ssid[32];

Best regards.

@devyte
Copy link
Collaborator

devyte commented Jul 9, 2018

You are correct, there current core implementation limits SSID to 31 chars due to the NULL term. This is a known issue, and there has been one attempt to fix it in PR #4691 , but it was not enough, and the author has not replied.
The main reason behind the 31-char limit is that it was unclear how the SDK handles it internally. There was a similar issue on the password length limit of 63/64 chars reported in #2247 which was fixed in #4076 , although it wasn't really tested for the same reason: it is unclear how the SDK handles the full-length strings, which don't have a null term.
A similar fix could be attempted here. I invite you to study the above PRs and implement this fix.

@devyte devyte added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Jul 9, 2018
@mictlan666
Copy link
Author

Thank you for reply.
For my purposes i change condition to strlen(ssid) > 32 and works for now.

@devyte
Copy link
Collaborator

devyte commented Jul 10, 2018

@mictlan666 please don't do that, the copy of the ssid into the SDK is currently done with strcpy, and the suspicion is that the SDK array is of size 32. Your change will copy the null terminator, which will corrupt 1 byte after the end of the array inside the SDK, which in turn can potentially cause who knows what instability in your app.
At the very least, the copy into the SDK must be changed to the way the password is copied, which ensures that corruption doesn't occur. Testing must then be done to assure that it actually works, i.e.: to make sure that the SDK doesn't rely on the ssid string being null-termed.

@tablatronix
Copy link
Contributor

This should probably use explicit length to prevent that
char * strncpy ( char * destination, const char * source, size_t num );

@C0rn3j
Copy link

C0rn3j commented Sep 14, 2018

Just ran into this, was wondering why my code doesn't work but turns out my SSID is just the max length by the standard(32)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

No branches or pull requests

4 participants