From 0f8b41a8ab233c67ab273cef02ab4323a404a5e3 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Tue, 20 Oct 2026 09:37:48 -0500 Subject: [PATCH 1/3] fix(wifi): replace credentials completely; reject SSIDs / passwords longer than their fields WifiSta::connect(ssid, password) loaded the stored STA configuration and memcpy'd the new SSID and password over it without clearing the fields, so a shorter value kept the trailing bytes of the previous one ("1234567890" -> "abcdefgh" became "abcdefgh90") and authentication failed; an empty password silently kept the old one although the API documents it as "open network". None of the setters checked the lengths against the 32 / 64-byte fields. New wifi_credentials::set_field() (standard-library only, host test in components/wifi/test) clears a field and copies the value, refusing one that does not fit; WifiSta::set_config / connect and WifiAp::set_config / set_ssid_and_password use it and log + return false on an over-long value. An empty password now clears the stored one, as documented. Fixes #848 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- components/wifi/include/wifi_ap.hpp | 38 ++++++--- components/wifi/include/wifi_credentials.hpp | 50 ++++++++++++ components/wifi/include/wifi_sta.hpp | 33 ++++++-- .../wifi/test/wifi_credentials_host_test.cpp | 81 +++++++++++++++++++ doc/Doxyfile | 1 + doc/en/wifi/wifi_sta.rst | 1 + 6 files changed, 184 insertions(+), 20 deletions(-) create mode 100644 components/wifi/include/wifi_credentials.hpp create mode 100644 components/wifi/test/wifi_credentials_host_test.cpp diff --git a/components/wifi/include/wifi_ap.hpp b/components/wifi/include/wifi_ap.hpp index 129ec8fdad..edb236b5b5 100644 --- a/components/wifi/include/wifi_ap.hpp +++ b/components/wifi/include/wifi_ap.hpp @@ -9,6 +9,7 @@ #include "base_component.hpp" #include "wifi_base.hpp" +#include "wifi_credentials.hpp" #include "wifi_format_helpers.hpp" namespace espp { @@ -264,11 +265,19 @@ class WifiAp : public WifiBase { // Update configuration wifi_config_t wifi_config; memset(&wifi_config, 0, sizeof(wifi_config)); + if (!wifi_credentials::set_field(wifi_config.ap.ssid, config.ssid)) { + logger_.error("SSID is too long ({} bytes, at most {})", config.ssid.size(), + sizeof(wifi_config.ap.ssid)); + return false; + } + if (!wifi_credentials::set_field(wifi_config.ap.password, config.password)) { + logger_.error("Password is too long ({} bytes, at most {})", config.password.size(), + sizeof(wifi_config.ap.password)); + return false; + } wifi_config.ap.ssid_len = (uint8_t)config.ssid.size(); wifi_config.ap.channel = config.channel; wifi_config.ap.max_connection = config.max_number_of_stations; - memcpy(wifi_config.ap.ssid, config.ssid.data(), config.ssid.size()); - memcpy(wifi_config.ap.password, config.password.data(), config.password.size()); if (config.password.size() == 0) { wifi_config.ap.authmode = WIFI_AUTH_OPEN; } @@ -382,18 +391,21 @@ class WifiAp : public WifiBase { logger_.error("Could not get current WiFi config: {}", esp_err_to_name(err)); return false; } - memset(wifi_config.ap.ssid, 0, sizeof(wifi_config.ap.ssid)); - memset(wifi_config.ap.password, 0, sizeof(wifi_config.ap.password)); - // now update the SSID and password - wifi_config.ap.ssid_len = (uint8_t)ssid.size(); - memcpy(wifi_config.ap.ssid, ssid.data(), ssid.size()); - // update the password / authmode - if (password.empty()) { - wifi_config.ap.authmode = WIFI_AUTH_OPEN; - } else { - wifi_config.ap.authmode = WIFI_AUTH_WPA2_PSK; - memcpy(wifi_config.ap.password, password.data(), password.size()); + // now update the SSID and password (each field is cleared before the copy, + // so a shorter value never keeps the previous one's trailing bytes) + if (!wifi_credentials::set_field(wifi_config.ap.ssid, ssid)) { + logger_.error("SSID is too long ({} bytes, at most {})", ssid.size(), + sizeof(wifi_config.ap.ssid)); + return false; } + if (!wifi_credentials::set_field(wifi_config.ap.password, password)) { + logger_.error("Password is too long ({} bytes, at most {})", password.size(), + sizeof(wifi_config.ap.password)); + return false; + } + wifi_config.ap.ssid_len = (uint8_t)ssid.size(); + // update the authmode + wifi_config.ap.authmode = password.empty() ? WIFI_AUTH_OPEN : WIFI_AUTH_WPA2_PSK; // set the new configuration err = esp_wifi_set_config(WIFI_IF_AP, &wifi_config); if (err != ESP_OK) { diff --git a/components/wifi/include/wifi_credentials.hpp b/components/wifi/include/wifi_credentials.hpp new file mode 100644 index 0000000000..ae4df99120 --- /dev/null +++ b/components/wifi/include/wifi_credentials.hpp @@ -0,0 +1,50 @@ +#pragma once + +#include +#include +#include +#include + +namespace espp { +/// Helpers for the fixed-size SSID / password fields of ESP-IDF's +/// `wifi_config_t` (`wifi_sta_config_t` and `wifi_ap_config_t`), shared by +/// espp::WifiSta and espp::WifiAp. Standard-library only (host-testable, see +/// test/wifi_credentials_host_test.cpp). +namespace wifi_credentials { + +/// Size of the SSID fields (`wifi_sta_config_t::ssid`, `wifi_ap_config_t::ssid`). +static constexpr size_t kSsidFieldSize = 32; +/// Size of the password fields; a WPA passphrase is 8..63 characters and a +/// raw 64-hex-digit PSK fills the field. +static constexpr size_t kPasswordFieldSize = 64; + +/// @brief Whether `value` fits a credential field of `field_size` bytes. +constexpr bool fits(std::string_view value, size_t field_size) { + return value.size() <= field_size; +} + +/** + * @brief Store a credential in a fixed-size field, replacing it completely. + * + * The whole field is cleared before the copy, so a value shorter than the one + * the field held before never keeps the old trailing bytes (a password + * "1234567890" replaced by "abcdefgh" used to become "abcdefgh90"). A value + * that fills the field exactly is allowed (ESP-IDF treats such a field as not + * NUL-terminated); an empty value clears the field. + * + * @param field The destination array (its size is deduced). + * @param value The new content. + * @return true when the value fit and was stored; false (field untouched) when + * it is longer than the field. + */ +template bool set_field(uint8_t (&field)[N], std::string_view value) { + if (!fits(value, N)) + return false; + std::memset(field, 0, N); + if (!value.empty()) + std::memcpy(field, value.data(), value.size()); + return true; +} + +} // namespace wifi_credentials +} // namespace espp diff --git a/components/wifi/include/wifi_sta.hpp b/components/wifi/include/wifi_sta.hpp index e991f6973e..b9a583f0a1 100644 --- a/components/wifi/include/wifi_sta.hpp +++ b/components/wifi/include/wifi_sta.hpp @@ -11,6 +11,7 @@ #include "base_component.hpp" #include "wifi_base.hpp" +#include "wifi_credentials.hpp" #include "wifi_format_helpers.hpp" namespace espp { @@ -461,8 +462,16 @@ class WifiSta : public WifiBase { std::string_view(reinterpret_cast(wifi_config.sta.ssid))); } else { logger_.debug("Setting WiFi SSID to '{}'", config.ssid); - memcpy(wifi_config.sta.ssid, config.ssid.data(), config.ssid.size()); - memcpy(wifi_config.sta.password, config.password.data(), config.password.size()); + if (!wifi_credentials::set_field(wifi_config.sta.ssid, config.ssid)) { + logger_.error("SSID is too long ({} bytes, at most {})", config.ssid.size(), + sizeof(wifi_config.sta.ssid)); + return false; + } + if (!wifi_credentials::set_field(wifi_config.sta.password, config.password)) { + logger_.error("Password is too long ({} bytes, at most {})", config.password.size(), + sizeof(wifi_config.sta.password)); + return false; + } } wifi_config.sta.channel = config.channel; @@ -569,9 +578,13 @@ class WifiSta : public WifiBase { /** * @brief Connect to the access point with the given SSID and password. - * @param ssid SSID of the access point. - * @param password Password of the access point. If empty, the AP will be open / have no security. + * @param ssid SSID of the access point (at most 32 bytes). + * @param password Password of the access point (at most 64 bytes). If empty, the AP is open / + * has no security: the previously stored password is cleared, not kept. * @return true if the connection was initiated successfully, false otherwise. + * @note The SSID and password replace the stored ones completely: a shorter value never keeps + * the trailing bytes of the previous one. The rest of the stored STA configuration + * (channel, BSSID, ...) is kept. */ bool connect(std::string_view ssid, std::string_view password) { if (ssid.empty()) { @@ -584,9 +597,15 @@ class WifiSta : public WifiBase { logger_.error("Could not get saved WiFi configuration"); return false; } - memcpy(wifi_config.sta.ssid, ssid.data(), ssid.size()); - if (!password.empty()) { - memcpy(wifi_config.sta.password, password.data(), password.size()); + if (!wifi_credentials::set_field(wifi_config.sta.ssid, ssid)) { + logger_.error("SSID is too long ({} bytes, at most {})", ssid.size(), + sizeof(wifi_config.sta.ssid)); + return false; + } + if (!wifi_credentials::set_field(wifi_config.sta.password, password)) { + logger_.error("Password is too long ({} bytes, at most {})", password.size(), + sizeof(wifi_config.sta.password)); + return false; } esp_err_t err = esp_wifi_set_config(WIFI_IF_STA, &wifi_config); if (err != ESP_OK) { diff --git a/components/wifi/test/wifi_credentials_host_test.cpp b/components/wifi/test/wifi_credentials_host_test.cpp new file mode 100644 index 0000000000..f02c158f9c --- /dev/null +++ b/components/wifi/test/wifi_credentials_host_test.cpp @@ -0,0 +1,81 @@ +// Host-buildable test for the credential field helpers shared by WifiSta and +// WifiAp (include/wifi_credentials.hpp -- standard library only). Build & run: +// c++ -std=c++23 -Wall -Wextra -Werror -I components/wifi/include \ +// components/wifi/test/wifi_credentials_host_test.cpp -o test && ./test + +#include +#include +#include + +#include "wifi_credentials.hpp" + +namespace creds = espp::wifi_credentials; + +static int g_failures = 0; +#define CHECK(cond) \ + do { \ + if (!(cond)) { \ + std::printf(" FAIL: %s (line %d)\n", #cond, __LINE__); \ + ++g_failures; \ + } \ + } while (0) + +// the field as ESP-IDF reads it: NUL-terminated unless it is completely full +static std::string as_string(const uint8_t *field, size_t n) { + size_t len = 0; + while (len < n && field[len] != 0) + len++; + return std::string(reinterpret_cast(field), len); +} + +int main() { + // ---- the reported bug: a shorter password kept the old trailing bytes ---- + { + uint8_t password[creds::kPasswordFieldSize]; + CHECK(creds::set_field(password, "1234567890")); + CHECK(as_string(password, sizeof(password)) == "1234567890"); + CHECK(creds::set_field(password, "abcdefgh")); + CHECK(as_string(password, sizeof(password)) == "abcdefgh"); // not "abcdefgh90" + // every byte past the value is zero, not just the one after it + bool clean = true; + for (size_t i = 8; i < sizeof(password); i++) + clean = clean && password[i] == 0; + CHECK(clean); + } + // ---- the same for the SSID ---- + { + uint8_t ssid[creds::kSsidFieldSize]; + CHECK(creds::set_field(ssid, "a-long-network-name")); + CHECK(creds::set_field(ssid, "net")); + CHECK(as_string(ssid, sizeof(ssid)) == "net"); + } + // ---- an empty value clears the field (an open network) ---- + { + uint8_t password[creds::kPasswordFieldSize]; + CHECK(creds::set_field(password, "secret")); + CHECK(creds::set_field(password, "")); + CHECK(as_string(password, sizeof(password)).empty()); + } + // ---- a value that exactly fills the field is accepted, one byte more is refused ---- + { + uint8_t ssid[creds::kSsidFieldSize]; + const std::string full(creds::kSsidFieldSize, 'x'); + CHECK(creds::set_field(ssid, full)); + CHECK(std::memcmp(ssid, full.data(), full.size()) == 0); + const std::string previous = full; + CHECK(!creds::set_field(ssid, full + "y")); // refused ... + CHECK(std::memcmp(ssid, previous.data(), previous.size()) == 0); // ... and the field untouched + uint8_t password[creds::kPasswordFieldSize]; + CHECK(creds::set_field(password, std::string(creds::kPasswordFieldSize, 'p'))); + CHECK(!creds::set_field(password, std::string(creds::kPasswordFieldSize + 1, 'p'))); + } + // ---- fits() mirrors set_field()'s acceptance ---- + CHECK(creds::fits("", 32) && creds::fits(std::string(32, 'a'), 32) && + !creds::fits(std::string(33, 'a'), 32)); + + if (g_failures == 0) + std::printf("wifi_credentials_host_test: all checks passed\n"); + else + std::printf("wifi_credentials_host_test: %d check(s) FAILED\n", g_failures); + return g_failures == 0 ? 0 : 1; +} diff --git a/doc/Doxyfile b/doc/Doxyfile index c763f12c82..32bf8529f7 100755 --- a/doc/Doxyfile +++ b/doc/Doxyfile @@ -515,6 +515,7 @@ INPUT = \ $(PROJECT_PATH)/components/wifi/include/wifi.hpp \ $(PROJECT_PATH)/components/wifi/include/wifi_base.hpp \ $(PROJECT_PATH)/components/wifi/include/wifi_ap.hpp \ + $(PROJECT_PATH)/components/wifi/include/wifi_credentials.hpp \ $(PROJECT_PATH)/components/wifi/include/wifi_ap_menu.hpp \ $(PROJECT_PATH)/components/wifi/include/wifi_sta.hpp \ $(PROJECT_PATH)/components/wifi/include/wifi_sta_menu.hpp \ diff --git a/doc/en/wifi/wifi_sta.rst b/doc/en/wifi/wifi_sta.rst index 4b4af1840a..e581d75e53 100644 --- a/doc/en/wifi/wifi_sta.rst +++ b/doc/en/wifi/wifi_sta.rst @@ -23,3 +23,4 @@ API Reference .. include-build-file:: inc/wifi_sta.inc .. include-build-file:: inc/wifi_sta_menu.inc +.. include-build-file:: inc/wifi_credentials.inc From 8e5e9d541ad841d86f7c144e49be405d01c88e26 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Tue, 20 Oct 2026 09:45:21 -0500 Subject: [PATCH 2/3] test(wifi): drop a redundant local copy in the credentials host test (cppcheck) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- components/wifi/test/wifi_credentials_host_test.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/components/wifi/test/wifi_credentials_host_test.cpp b/components/wifi/test/wifi_credentials_host_test.cpp index f02c158f9c..ba1294188b 100644 --- a/components/wifi/test/wifi_credentials_host_test.cpp +++ b/components/wifi/test/wifi_credentials_host_test.cpp @@ -62,9 +62,8 @@ int main() { const std::string full(creds::kSsidFieldSize, 'x'); CHECK(creds::set_field(ssid, full)); CHECK(std::memcmp(ssid, full.data(), full.size()) == 0); - const std::string previous = full; - CHECK(!creds::set_field(ssid, full + "y")); // refused ... - CHECK(std::memcmp(ssid, previous.data(), previous.size()) == 0); // ... and the field untouched + CHECK(!creds::set_field(ssid, full + "y")); // refused ... + CHECK(std::memcmp(ssid, full.data(), full.size()) == 0); // ... and the field untouched uint8_t password[creds::kPasswordFieldSize]; CHECK(creds::set_field(password, std::string(creds::kPasswordFieldSize, 'p'))); CHECK(!creds::set_field(password, std::string(creds::kPasswordFieldSize + 1, 'p'))); From 1740a57e3d632c74fec61049d2200406b0ac1f70 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Tue, 20 Oct 2026 09:49:32 -0500 Subject: [PATCH 3/3] docs(wifi): an empty WifiSta::Config::ssid means the saved credentials are used Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- components/wifi/include/wifi_sta.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/wifi/include/wifi_sta.hpp b/components/wifi/include/wifi_sta.hpp index b9a583f0a1..9a3995ceae 100644 --- a/components/wifi/include/wifi_sta.hpp +++ b/components/wifi/include/wifi_sta.hpp @@ -54,7 +54,9 @@ class WifiSta : public WifiBase { * @brief Configuration structure for the WiFi Station. */ struct Config { - std::string ssid; /**< SSID for the access point. */ + std::string ssid; /**< SSID for the access point. If empty, the credentials (SSID and + password) saved by esp-wifi from the last connection are used and + `password` is ignored. */ std::string password; /**< Password for the access point. If empty, the AP will be open / have no security. */ wifi_phy_rate_t phy_rate{