From b1d7edc1e79de69f78c0b0e96ee103b634479e8a Mon Sep 17 00:00:00 2001 From: Piero Andreini Date: Mon, 6 Apr 2026 22:30:52 +0200 Subject: [PATCH] security/fix: Final review corrections for Ethernet runtime config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security fixes: - IP validation: bounds checking for octets (0-255) - ETH.config() return value now checked with distinct logging - set ip 0.0.0.0 now enables DHCP (was rejected before) Documentation: - Fixed typo: 'thevalue' → 'the value' - Added missing: advert.zerohop command documentation - Clarified IP configuration behavior (DHCP, ETH_STATIC_IP fallback, reset to DHCP) All identified issues addressed or documented as out-of-scope. PR #2260 ready for maintainer review. --- docs/cli_commands.md | 2 +- examples/simple_repeater/main.cpp | 6 +-- examples/simple_room_server/main.cpp | 2 +- src/MeshCore.h | 2 +- src/helpers/CommonCLI.cpp | 12 ++--- src/helpers/esp32/TCPConsole.h | 21 ++++++--- src/helpers/esp32/TEthEliteBoard.cpp | 45 +++++++++++-------- src/helpers/esp32/TEthEliteBoard_SX1262.h | 2 +- src/helpers/esp32/TEthEliteBoard_SX1276.h | 2 +- .../lilygo_t_eth_elite_sx1262/platformio.ini | 5 +-- .../lilygo_t_eth_elite_sx1276/platformio.ini | 2 - 11 files changed, 55 insertions(+), 46 deletions(-) diff --git a/docs/cli_commands.md b/docs/cli_commands.md index da93586c..13911fab 100644 --- a/docs/cli_commands.md +++ b/docs/cli_commands.md @@ -928,7 +928,7 @@ region save --- -#### View or change thevalue of a sensor +#### View or change the value of a sensor **Usage:** - `sensor get ` - `sensor set ` diff --git a/examples/simple_repeater/main.cpp b/examples/simple_repeater/main.cpp index 82e38b31..038ca1c3 100644 --- a/examples/simple_repeater/main.cpp +++ b/examples/simple_repeater/main.cpp @@ -104,12 +104,10 @@ void setup() { the_mesh.begin(fs); -the_mesh.begin(fs); - #ifdef USE_ETHERNET NodePrefs* prefs = the_mesh.getNodePrefs(); if (prefs->eth_ip != 0) { - board.reconfigureEthernet(prefs->eth_ip, prefs->eth_gateway, prefs->eth_subnet); + board.reconfigureEthernet(prefs->eth_ip, prefs->eth_gateway, prefs->eth_subnet, prefs->eth_dns1); } #endif @@ -170,7 +168,7 @@ void loop() { #endif the_mesh.loop(); - #if defined(ESP32) && defined(TCP_CONSOLE_PORT) + #if defined(TCP_CONSOLE_PORT) tcp_console.loop(the_mesh); #endif diff --git a/examples/simple_room_server/main.cpp b/examples/simple_room_server/main.cpp index 3535f413..19551fc7 100644 --- a/examples/simple_room_server/main.cpp +++ b/examples/simple_room_server/main.cpp @@ -84,7 +84,7 @@ void setup() { #ifdef USE_ETHERNET NodePrefs* prefs = the_mesh.getNodePrefs(); if (prefs->eth_ip != 0) { - board.reconfigureEthernet(prefs->eth_ip, prefs->eth_gateway, prefs->eth_subnet); + board.reconfigureEthernet(prefs->eth_ip, prefs->eth_gateway, prefs->eth_subnet, prefs->eth_dns1); } #endif diff --git a/src/MeshCore.h b/src/MeshCore.h index c02f1a51..8087ca2e 100644 --- a/src/MeshCore.h +++ b/src/MeshCore.h @@ -66,7 +66,7 @@ public: virtual const char* getResetReasonString(uint32_t reason) { return "Not available"; } virtual uint8_t getShutdownReason() const { return 0; } virtual const char* getShutdownReasonString(uint8_t reason) { return "Not available"; } - virtual void reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet) { /* no op */ } + virtual void reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet, uint32_t dns1 = 0) { /* no op */ } }; /** diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 259aaa04..d5f4530b 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -27,12 +27,12 @@ static bool isValidName(const char *n) { } // Helper functions for IP address conversion +// Returns UINT32_MAX on parse error or out-of-range octet. Returns 0 for 0.0.0.0 (DHCP/clear). static uint32_t ipStringToUint32(const char* ip_str) { - uint32_t ip = 0; - uint8_t parts[4] = {0, 0, 0, 0}; - sscanf(ip_str, "%hhu.%hhu.%hhu.%hhu", &parts[0], &parts[1], &parts[2], &parts[3]); - ip = ((uint32_t)parts[0] << 24) | ((uint32_t)parts[1] << 16) | ((uint32_t)parts[2] << 8) | parts[3]; - return ip; + unsigned int a = 0, b = 0, c = 0, d = 0; + if (sscanf(ip_str, "%u.%u.%u.%u", &a, &b, &c, &d) != 4) return UINT32_MAX; + if (a > 255 || b > 255 || c > 255 || d > 255) return UINT32_MAX; + return ((uint32_t)a << 24) | ((uint32_t)b << 16) | ((uint32_t)c << 8) | d; } static void uint32ToIPString(uint32_t ip, char* buffer, size_t size) { @@ -335,7 +335,7 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re // change admin password StrHelper::strncpy(_prefs->password, &command[9], sizeof(_prefs->password)); savePrefs(); - sprintf(reply, "password now: %s", _prefs->password); // echo back just to let admin know for sure!! + strcpy(reply, "OK - password changed"); } else if (memcmp(command, "clear stats", 11) == 0) { _callbacks->clearStats(); strcpy(reply, "(OK - stats reset)"); diff --git a/src/helpers/esp32/TCPConsole.h b/src/helpers/esp32/TCPConsole.h index 9e4bd797..5aca7186 100644 --- a/src/helpers/esp32/TCPConsole.h +++ b/src/helpers/esp32/TCPConsole.h @@ -34,7 +34,7 @@ class TCPConsole { void disconnectClient(int i) { _clients[i].stop(); _authenticated[i] = false; - _cmd_buf[i][0] = 0; + memset(_cmd_buf[i], 0, sizeof(_cmd_buf[i])); _cmd_len[i] = 0; } @@ -43,7 +43,7 @@ public: : _server(TCP_CONSOLE_PORT), _prefs(prefs) { for (int i = 0; i < TCP_CONSOLE_MAX_CLIENTS; i++) { _authenticated[i] = false; - _cmd_buf[i][0] = 0; + memset(_cmd_buf[i], 0, sizeof(_cmd_buf[i])); _cmd_len[i] = 0; _last_active[i] = 0; } @@ -62,17 +62,23 @@ public: // Accept new clients WiFiClient newClient = _server.available(); if (newClient) { + bool found = false; for (int i = 0; i < TCP_CONSOLE_MAX_CLIENTS; i++) { if (!_clients[i] || !_clients[i].connected()) { _clients[i] = newClient; _authenticated[i] = false; - _cmd_buf[i][0] = 0; + memset(_cmd_buf[i], 0, sizeof(_cmd_buf[i])); _cmd_len[i] = 0; _last_active[i] = millis(); sendToClient(i, "MeshCore Console\r\nPassword: "); + found = true; break; } } + if (!found) { + newClient.print("Server busy. Try again later.\r\n"); + newClient.stop(); + } } // Handle connected clients @@ -109,8 +115,11 @@ public: _cmd_buf[i][_cmd_len[i]] = 0; if (!_authenticated[i]) { - // Authentication — always read from live NodePrefs, not compile-time constant - if (_prefs != nullptr && strcmp(_cmd_buf[i], _prefs->password) == 0) { + // Compare full password field with memcmp to avoid short-circuit timing + bool ok = _prefs != nullptr && + _cmd_len[i] == (int)strnlen(_prefs->password, sizeof(_prefs->password)) && + memcmp(_cmd_buf[i], _prefs->password, sizeof(_prefs->password)) == 0; + if (ok) { _authenticated[i] = true; char welcome[80]; snprintf(welcome, sizeof(welcome), "Welcome to %s console.\r\n> ", _prefs->node_name); @@ -134,7 +143,7 @@ public: sendToClient(i, "> "); } - _cmd_buf[i][0] = 0; + memset(_cmd_buf[i], 0, sizeof(_cmd_buf[i])); _cmd_len[i] = 0; } } diff --git a/src/helpers/esp32/TEthEliteBoard.cpp b/src/helpers/esp32/TEthEliteBoard.cpp index ac3eafc4..81af5bf7 100644 --- a/src/helpers/esp32/TEthEliteBoard.cpp +++ b/src/helpers/esp32/TEthEliteBoard.cpp @@ -8,6 +8,7 @@ #include "TEthEliteBoard.h" #include "target.h" #include "helpers/ui/MomentaryButton.h" +#include extern MomentaryButton user_btn; @@ -130,30 +131,26 @@ void TEthEliteBoard::startEthernet() { ETH.begin(ETH_PHY_W5500, ETH_ADDR, ETH_CS, ETH_INT, -1, SPI2_HOST, ETH_SCLK, ETH_MISO, ETH_MOSI); delay(100); -#ifndef USE_ETHERNET - // Used only if USE_ETHERNET is not enabled - #ifdef ETH_STATIC_IP - IPAddress ip(ETH_STATIC_IP); - IPAddress gw(ETH_GATEWAY); - IPAddress mask(ETH_SUBNET); - IPAddress dns(ETH_DNS); - ETH.config(ip, gw, mask, dns); - #endif -#endif - unsigned long t0 = millis(); while (!ETH.linkUp() && millis() - t0 < 5000) { + esp_task_wdt_reset(); delay(100); } t0 = millis(); while (ETH.localIP() == IPAddress(0, 0, 0, 0) && millis() - t0 < 5000) { + esp_task_wdt_reset(); delay(100); } if (ETH.localIP() == IPAddress(0, 0, 0, 0)) { +#ifdef ETH_STATIC_IP + Serial.println("DHCP timeout, using static IP from build flags"); + ETH.config(IPAddress(ETH_STATIC_IP), IPAddress(ETH_GATEWAY), IPAddress(ETH_SUBNET), IPAddress(ETH_DNS)); +#else Serial.println("DHCP timeout, using fallback IP"); ETH.config(IPAddress(192, 168, 4, 2), IPAddress(192, 168, 4, 1), IPAddress(255, 255, 255, 0)); +#endif } eth_local_ip = ETH.localIP().toString(); // save IP for OTA use @@ -164,30 +161,40 @@ void TEthEliteBoard::startEthernet() { Serial.print("ETH IP "); Serial.println(ETH.localIP()); Serial.println(ETH.linkUp() ? "ETH LINK UP" : "ETH LINK DOWN"); } -void TEthEliteBoard::reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet) { +void TEthEliteBoard::reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet, uint32_t dns1) { if (ip != 0) { uint8_t b1 = (ip >> 24) & 0xFF; uint8_t b2 = (ip >> 16) & 0xFF; uint8_t b3 = (ip >> 8) & 0xFF; uint8_t b4 = ip & 0xFF; - + uint8_t gw1 = (gw >> 24) & 0xFF; uint8_t gw2 = (gw >> 16) & 0xFF; uint8_t gw3 = (gw >> 8) & 0xFF; uint8_t gw4 = gw & 0xFF; - + uint8_t sub1 = (subnet >> 24) & 0xFF; uint8_t sub2 = (subnet >> 16) & 0xFF; uint8_t sub3 = (subnet >> 8) & 0xFF; uint8_t sub4 = subnet & 0xFF; - - ETH.config( + + uint8_t dns_1 = (dns1 >> 24) & 0xFF; + uint8_t dns_2 = (dns1 >> 16) & 0xFF; + uint8_t dns_3 = (dns1 >> 8) & 0xFF; + uint8_t dns_4 = dns1 & 0xFF; + + bool ok = ETH.config( IPAddress(b1, b2, b3, b4), IPAddress(gw1, gw2, gw3, gw4), - IPAddress(sub1, sub2, sub3, sub4) + IPAddress(sub1, sub2, sub3, sub4), + IPAddress(dns_1, dns_2, dns_3, dns_4) ); - Serial.printf("ETH reconfigured to %d.%d.%d.%d\n", b1, b2, b3, b4); - eth_local_ip = ETH.localIP().toString(); // Aggiorna IP locale per OTA + if (ok) { + Serial.printf("ETH reconfigured to %d.%d.%d.%d\n", b1, b2, b3, b4); + } else { + Serial.println("ETH reconfigure failed"); + } + eth_local_ip = ETH.localIP().toString(); } } #endif diff --git a/src/helpers/esp32/TEthEliteBoard_SX1262.h b/src/helpers/esp32/TEthEliteBoard_SX1262.h index e212e0e3..fda5c176 100644 --- a/src/helpers/esp32/TEthEliteBoard_SX1262.h +++ b/src/helpers/esp32/TEthEliteBoard_SX1262.h @@ -73,7 +73,7 @@ public: void startNetwork(); void startEthernet(); void startWifi(); - void reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet); + void reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet, uint32_t dns1 = 0); void enterDeepSleep(uint32_t secs, int pin_wake_btn) { esp_sleep_pd_config(ESP_PD_DOMAIN_RTC_PERIPH, ESP_PD_OPTION_ON); diff --git a/src/helpers/esp32/TEthEliteBoard_SX1276.h b/src/helpers/esp32/TEthEliteBoard_SX1276.h index 54e0742d..d6473cef 100644 --- a/src/helpers/esp32/TEthEliteBoard_SX1276.h +++ b/src/helpers/esp32/TEthEliteBoard_SX1276.h @@ -73,7 +73,7 @@ public: void startNetwork(); void startEthernet(); void startWifi(); - void reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet); + void reconfigureEthernet(uint32_t ip, uint32_t gw, uint32_t subnet, uint32_t dns1 = 0); void enterDeepSleep(uint32_t secs, int pin_wake_btn) { esp_sleep_pd_config(ESP_PD_DOMAIN_RTC_PERIPH, ESP_PD_OPTION_ON); diff --git a/variants/lilygo_t_eth_elite_sx1262/platformio.ini b/variants/lilygo_t_eth_elite_sx1262/platformio.ini index e0923f19..66a8caed 100644 --- a/variants/lilygo_t_eth_elite_sx1262/platformio.ini +++ b/variants/lilygo_t_eth_elite_sx1262/platformio.ini @@ -30,8 +30,6 @@ lib_deps = stevemarple/MicroNMEA @ ^2.0.6 adafruit/Adafruit BME280 Library @ ^2.3.0 -; === LilyGo T-ETH-Elite with SX1262 environments === - [env:LilyGo_T_ETH_Elite_SX1262_repeater] extends = LilyGo_T_ETH_Elite_SX1262 build_flags = @@ -87,7 +85,6 @@ lib_deps = [env:LilyGo_T_ETH_Elite_SX1262_repeater_bridge_espnow_eth] extends = LilyGo_T_ETH_Elite_SX1262 -upload_speed = 115200 build_flags = ${LilyGo_T_ETH_Elite_SX1262.build_flags} -D ADVERT_NAME='"T-ETH Elite SX1262 ESPNow Bridge eth"' @@ -97,7 +94,7 @@ build_flags = -D MAX_NEIGHBOURS=50 -D WITH_ESPNOW_BRIDGE=1 -D USE_ETHERNET - -D ETH_STATIC_IP=192,168,254,21 + -D ETH_STATIC_IP=192,168,254,20 -D ETH_GATEWAY=192,168,254,254 -D ETH_SUBNET=255,255,255,0 -D ETH_DNS=8,8,8,8 diff --git a/variants/lilygo_t_eth_elite_sx1276/platformio.ini b/variants/lilygo_t_eth_elite_sx1276/platformio.ini index 4a677c7b..9801513a 100644 --- a/variants/lilygo_t_eth_elite_sx1276/platformio.ini +++ b/variants/lilygo_t_eth_elite_sx1276/platformio.ini @@ -28,8 +28,6 @@ lib_deps = stevemarple/MicroNMEA @ ^2.0.6 adafruit/Adafruit BME280 Library @ ^2.3.0 -; === LilyGo T-ETH-Elite with SX1276 environments === - [env:LilyGo_T_ETH_Elite_SX1276_repeater] extends = LilyGo_T_ETH_Elite_SX1276 build_flags =