fix(wifi): replace credentials completely; reject over-long SSIDs / passwords (#848)

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

components/wifi/include/wifi_credentials.hpp

@@ -0,0 +1,50 @@
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <string_view>
+
+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 <size_t N> 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

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 {
@@ -53,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{
@@ -461,8 +464,16 @@ class WifiSta : public WifiBase {
                    std::string_view(reinterpret_cast<char *>(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 +580,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 +599,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) {

components/wifi/test/wifi_credentials_host_test.cpp

@@ -0,0 +1,80 @@
+// 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 <cstdio>
+#include <cstring>
+#include <string>
+
+#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<const char *>(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);
+    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')));
+  }
+  // ---- 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;
+}

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 \

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