diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index 78ca72b2b..af07f7581 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -629,6 +629,7 @@ jobs: python3 -B test/test_wifi_ota_start.py -v python3 -B test/test_webconfig_ap_start.py -v python3 -B test/test_webconfig_recovery_auth.py -v + python3 -B test/test_webconfig_login_throttle.py -v python3 -B test/test_esp32_wifi_scan_fix.py -v python3 -B test/test_esp32_wifi_scan_recipes.py -v diff --git a/MQTT_IMPLEMENTATION.md b/MQTT_IMPLEMENTATION.md index 32fec249a..d593cc8c7 100644 --- a/MQTT_IMPLEMENTATION.md +++ b/MQTT_IMPLEMENTATION.md @@ -759,7 +759,8 @@ Optionally set a WPA2 password for the setup AP at build time with LAN the node was attached to. - **LAN**: requires the admin password (same one used for remote CLI admin). Sessions use a cookie with a sliding idle expiry (default 20 minutes); five - failed logins trigger a 30-second lockout. + failed logins from one address lock that address for 30 seconds. Failed + logins from all addresses together are capped at six, then one per 6 seconds. > **Security note:** the open setup AP transports WiFi/MQTT credentials over > plain HTTP. Provision on a trusted, non-public frequency/location, set diff --git a/src/helpers/WebConfigLoginThrottle.h b/src/helpers/WebConfigLoginThrottle.h new file mode 100644 index 000000000..a70d1ecf6 --- /dev/null +++ b/src/helpers/WebConfigLoginThrottle.h @@ -0,0 +1,108 @@ +#pragma once + +#include + +// Pure, platform-independent failed-login accounting for WebConfig's +// /api/login. Each client address is locked on its own after kClientFails +// failures, so one client that keeps failing cannot lock the owner out from +// another address. +// +// Addresses are cheap to change, so a shared budget also caps the failures of +// all clients together. It refills at the rate one client may fail +// (kClientFails per kClientLockMs, the old global rate) and holds one failure +// more than a single client can spend, so one failing client alone never +// empties it. While it is empty every client waits, as under the old global +// lockout. +namespace WebConfigLoginThrottle { + +static const uint8_t kClientFails = 5; +static const uint32_t kClientLockMs = 30000; +static const uint8_t kSharedBudget = kClientFails + 1; +static const uint32_t kSharedRefillMs = kClientLockMs / kClientFails; +// Tracked failing clients. Evicting one only resets that client's count; the +// shared budget still bounds the total. +static const uint8_t kClients = 4; + +enum Verdict { ALLOW, CLIENT_LOCKED, SHARED_LOCKED }; + +class Tracker { +public: + Tracker() : _spent(0), _refill_at(0) {} + + Verdict check(uint32_t client, uint32_t now) { + refill(now); + // A client at kClientFails is locked until kClientLockMs after its last + // failure; expire every slot here so a stale lock is not kept around. + for (uint8_t i = 0; i < kClients; i++) { + if (_slots[i].fails >= kClientFails && now - _slots[i].last >= kClientLockMs) { + _slots[i].fails = 0; + } + } + const Slot* slot = find(client); + if (slot && slot->fails >= kClientFails) return CLIENT_LOCKED; + if (_spent >= kSharedBudget) return SHARED_LOCKED; + return ALLOW; + } + + void noteFailure(uint32_t client, uint32_t now) { + refill(now); + if (_spent == 0) _refill_at = now; + if (_spent < kSharedBudget) _spent++; + Slot* slot = find(client); + if (!slot) slot = claim(client, now); + slot->last = now; + if (slot->fails < kClientFails) slot->fails++; + } + + // Clears only this client's count. The shared budget keeps draining on its + // own, so a success does not hand other clients a fresh burst. + void noteSuccess(uint32_t client) { + Slot* slot = find(client); + if (slot) slot->fails = 0; + } + +private: + struct Slot { + uint32_t client = 0; + uint32_t last = 0; + uint8_t fails = 0; // 0 = free slot + }; + + Slot _slots[kClients]; + uint8_t _spent; + uint32_t _refill_at; + + void refill(uint32_t now) { + const uint32_t steps = (now - _refill_at) / kSharedRefillMs; + if (steps >= _spent) { + _spent = 0; + return; + } + _spent -= (uint8_t)steps; + _refill_at += steps * kSharedRefillMs; + } + + Slot* find(uint32_t client) { + for (uint8_t i = 0; i < kClients; i++) { + if (_slots[i].fails > 0 && _slots[i].client == client) return &_slots[i]; + } + return nullptr; + } + + // A free slot, else the one whose last failure is oldest. + Slot* claim(uint32_t client, uint32_t now) { + Slot* victim = &_slots[0]; + for (uint8_t i = 0; i < kClients; i++) { + if (_slots[i].fails == 0) { + victim = &_slots[i]; + break; + } + if (now - _slots[i].last > now - victim->last) victim = &_slots[i]; + } + victim->client = client; + victim->fails = 0; + return victim; + } +}; + +} // namespace WebConfigLoginThrottle diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index 44d5ea52e..b8c1765ac 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -2067,10 +2067,18 @@ void WebConfigServer::handleLogin(AsyncWebServerRequest* req) { return; } uint32_t now = millis(); - if (_login_lock_until && (int32_t)(now - _login_lock_until) < 0) { + // Failures lock the failing address, not every client; a shared budget in + // WebConfigLoginThrottle still caps guesses across addresses. + const uint32_t client = req->client() ? req->client()->getRemoteAddress() : 0; + const WebConfigLoginThrottle::Verdict verdict = _login_throttle.check(client, now); + if (verdict == WebConfigLoginThrottle::CLIENT_LOCKED) { req->send(429, "application/json", "{\"error\":\"locked, retry in 30s\"}"); return; } + if (verdict == WebConfigLoginThrottle::SHARED_LOCKED) { + req->send(429, "application/json", "{\"error\":\"too many failed logins, retry shortly\"}"); + return; + } const char* body = (const char*)req->_tempObject; DynamicJsonDocument doc(256); if (!body || deserializeJson(doc, body) != DeserializationError::Ok) { @@ -2079,16 +2087,11 @@ void WebConfigServer::handleLogin(AsyncWebServerRequest* req) { } const char* pwd = doc["password"] | ""; if (!fixedTimeEquals(pwd, node.admin_password, sizeof(node.admin_password))) { - if (++_login_fails >= 5) { - _login_lock_until = now + 30000; - if (_login_lock_until == 0) _login_lock_until = 1; - _login_fails = 0; - } + _login_throttle.noteFailure(client, now); req->send(401, "application/json", "{\"error\":\"wrong password\"}"); return; } - _login_fails = 0; - _login_lock_until = 0; + _login_throttle.noteSuccess(client); for (int i = 0; i < 4; i++) sprintf(&_session_token[i * 8], "%08lx", (unsigned long)esp_random()); _session_last_seen = now; noteNetworkActivity(); diff --git a/src/helpers/esp32/WebConfigServer.h b/src/helpers/esp32/WebConfigServer.h index 4974c7439..b16e370c1 100644 --- a/src/helpers/esp32/WebConfigServer.h +++ b/src/helpers/esp32/WebConfigServer.h @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -360,8 +361,7 @@ private: // LAN-mode session (single slot; new login evicts the old session) char _session_token[33] = {0}; uint32_t _session_last_seen = 0; - uint8_t _login_fails = 0; - uint32_t _login_lock_until = 0; + WebConfigLoginThrottle::Tracker _login_throttle; // Once set (via /api/portal/exit), OS captive probes get native "success" // replies so the phone's sign-in sheet can be dismissed without dropping the diff --git a/test/fixtures/webconfig_login_handler.cpp b/test/fixtures/webconfig_login_handler.cpp new file mode 100644 index 000000000..97e1ba8e4 --- /dev/null +++ b/test/fixtures/webconfig_login_handler.cpp @@ -0,0 +1,107 @@ +#include +#include +#include +#include +#include +#include +#include +static uint32_t now_ms=100000, random_word=0x12345678; +uint32_t millis() { return now_ms; } +uint32_t esp_random() { return ++random_word; } +struct NodeSnapshot { char admin_password[16] = {}; }; +struct Callback { + NodeSnapshot node; + void getNodeSnapshot(NodeSnapshot& result) { result=node; } +}; +struct WCLock { explicit WCLock(int) {} }; +struct WiFiMock { int softAPIP() const { return 4; } } WiFi; +struct Client { + int local_ip=4; uint32_t remote=0; + int localIP() const { return local_ip; } + uint32_t getRemoteAddress() const { return remote; } +}; +struct AsyncWebServerResponse { + int status; std::map headers; + void addHeader(const char* key, const char* value) { headers[key]=value; } +}; +struct AsyncWebServerRequest { + Client socket; bool has_client=true; + std::string body, reply; void* _tempObject=nullptr; + int status=0; AsyncWebServerResponse response{}; + Client* client() { return has_client ? &socket : nullptr; } + void send(int code, const char* =nullptr, const char* content=nullptr) { status=code; reply=content ? content : ""; } + AsyncWebServerResponse* beginResponse(int code, const char*, const char*) { response.status=code; return &response; } + void send(AsyncWebServerResponse* value) { status=value->status; } +}; +struct WebConfigServer { + enum { MODE_OFF, MODE_SETUP, MODE_LAN }; + int _mode=MODE_SETUP, _mux=0; + char _wifi_ssid[33]="SavedNet", _session_token[33] = {}; + bool _initial_setup=false; + uint32_t _last_activity=0, _session_last_seen=0; + WebConfigLoginThrottle::Tracker _login_throttle; + Callback callback; Callback* _cb=&callback; + WebConfigServer() { strcpy(callback.node.admin_password, "existing-secret"); } + void noteNetworkActivity() {} + void handleLogin(AsyncWebServerRequest*); +}; +@METHODS@ +static const char* kLocked = "{\"error\":\"locked, retry in 30s\"}"; +static const char* kShared = "{\"error\":\"too many failed logins, retry shortly\"}"; +static std::string last_reply; +static int login(WebConfigServer& portal, uint32_t address, const char* password, bool has_client=true) { + AsyncWebServerRequest req; req.socket.remote=address; req.has_client=has_client; + req.body=std::string("{\"password\":\"")+password+"\"}"; req._tempObject=req.body.data(); + portal.handleLogin(&req); last_reply=req.reply; return req.status; +} +int main() { + const uint32_t attacker=0x0204A8C0, owner=0x0304A8C0; + // Recovery AP on a configured node: failures from one address lock only + // that address. The owner on another address still gets in. + { + WebConfigServer portal; + for (int i = 0; i < 5; i++) assert(login(portal, attacker, "guess")==401); + assert(login(portal, attacker, "existing-secret")==429 && last_reply==kLocked); + assert(login(portal, owner, "existing-secret")==200); + assert(strlen(portal._session_token)==32); + assert(login(portal, attacker, "existing-secret")==429 && last_reply==kLocked); + now_ms+=30000; + assert(login(portal, attacker, "existing-secret")==200); + } + // A success clears the owner's own earlier typos. + { + WebConfigServer portal; + for (int i = 0; i < 4; i++) assert(login(portal, owner, "typo")==401); + assert(login(portal, owner, "existing-secret")==200); + now_ms+=30000; + assert(login(portal, owner, "typo")==401); + assert(login(portal, owner, "existing-secret")==200); + } + // Rotating addresses does not add guesses: after the shared budget is + // spent every address waits, then one more attempt per refill period. + { + WebConfigServer portal; uint32_t address=10; + for (int i = 0; i < 6; i++) assert(login(portal, address++, "guess")==401); + assert(login(portal, owner, "existing-secret")==429 && last_reply==kShared); + now_ms+=5999; + assert(login(portal, owner, "existing-secret")==429 && last_reply==kShared); + now_ms+=1; + assert(login(portal, owner, "existing-secret")==200); + } + // An old lockout does not return when millis() is half a wrap past it. + { + WebConfigServer portal; + for (int i = 0; i < 5; i++) assert(login(portal, attacker, "guess")==401); + now_ms+=0x80000000u+60000u; + assert(login(portal, owner, "existing-secret")==200); + assert(login(portal, attacker, "existing-secret")==200); + } + // LAN requests without a client object share one key and do not crash. + { + WebConfigServer portal; portal._mode=WebConfigServer::MODE_LAN; + for (int i = 0; i < 5; i++) assert(login(portal, 0, "guess", false)==401); + assert(login(portal, 0, "existing-secret", false)==429 && last_reply==kLocked); + assert(login(portal, owner, "existing-secret")==200); + } + puts("WebConfig per-client login lockout checks passed"); +} diff --git a/test/fixtures/webconfig_login_throttle.cpp b/test/fixtures/webconfig_login_throttle.cpp new file mode 100644 index 000000000..b6d094f48 --- /dev/null +++ b/test/fixtures/webconfig_login_throttle.cpp @@ -0,0 +1,120 @@ +#include +#include +#include + +using namespace WebConfigLoginThrottle; + +static const uint32_t A = 0x0204A8C0, B = 0x0304A8C0; + +static void fail(Tracker& t, uint32_t client, uint32_t now, int times = 1) { + for (int i = 0; i < times; i++) { + assert(t.check(client, now) == ALLOW); + t.noteFailure(client, now); + } +} + +int main() { + // One address at its limit is locked on its own; others still get a try. + { + Tracker t; uint32_t now = 100000; + fail(t, A, now, kClientFails - 1); + assert(t.check(A, now) == ALLOW); + t.noteFailure(A, now); + assert(t.check(A, now) == CLIENT_LOCKED); + assert(t.check(B, now) == ALLOW); + t.noteSuccess(B); + assert(t.check(A, now) == CLIENT_LOCKED); // another client's success is not A's + assert(t.check(A, now + kClientLockMs - 1) == CLIENT_LOCKED); + assert(t.check(A, now + kClientLockMs) == ALLOW); + fail(t, A, now + kClientLockMs, kClientFails - 1); // the count restarted + assert(t.check(A, now + kClientLockMs) == ALLOW); + } + // A success clears only that client's own count. + { + Tracker t; uint32_t now = 100000; + fail(t, A, now, kClientFails - 1); + t.noteSuccess(A); + now += kClientLockMs; // let the shared budget refill + fail(t, A, now, kClientFails - 1); + assert(t.check(A, now) == ALLOW); + } + // A client failing as fast as it is allowed never empties the shared budget, + // so another address is never refused, while its own rate stays at the cap. + { + Tracker t; unsigned failures = 0; + for (uint32_t now = 100000; now < 100000 + 3600000; now += 100) { + if (t.check(A, now) == ALLOW) { t.noteFailure(A, now); failures++; } + assert(t.check(B, now) == ALLOW); + } + printf("one address, 1 h: %u failures\n", failures); + assert(failures <= kClientFails * (3600000 / kClientLockMs + 1)); + } + // Changing address on every attempt is bounded by the shared budget: a + // burst of kSharedBudget, then one failure per kSharedRefillMs. + { + Tracker t; uint32_t ip = 1; unsigned failures = 0; + const uint32_t start = 100000; + for (int i = 0; i < kSharedBudget; i++) fail(t, ip++, start); + assert(t.check(ip, start) == SHARED_LOCKED); + assert(t.check(ip, start + kSharedRefillMs - 1) == SHARED_LOCKED); + fail(t, ip++, start + kSharedRefillMs); + assert(t.check(ip, start + kSharedRefillMs) == SHARED_LOCKED); + for (uint32_t now = start + kSharedRefillMs; now < start + 3600000; now += 10) { + if (t.check(ip, now) == ALLOW) { t.noteFailure(ip++, now); failures++; } + } + failures += kSharedBudget + 1; + printf("new address per attempt, 1 h: %u failures\n", failures); + assert(failures <= kSharedBudget + 3600000 / kSharedRefillMs); + } + // The more specific answer wins when both limits apply. + { + Tracker t; uint32_t now = 100000; + fail(t, A, now, kClientFails); + fail(t, B, now, kSharedBudget - kClientFails); + assert(t.check(A, now) == CLIENT_LOCKED); + assert(t.check(B, now) == SHARED_LOCKED); + } + // Locks are measured as elapsed time, so an old lock does not come back + // when millis() is half a wrap past it. + { + Tracker t; uint32_t now = 100000; + fail(t, A, now, kClientFails); + fail(t, B, now, kSharedBudget - kClientFails); + now += 0x80000000u + 60000u; + assert(t.check(A, now) == ALLOW); + assert(t.check(B, now) == ALLOW); + } + // Failures recorded without a check saturate instead of wrapping. + { + Tracker t; uint32_t now = 100000; + for (int i = 0; i < 256; i++) t.noteFailure(A, now); + assert(t.check(A, now) == CLIENT_LOCKED); + assert(t.check(B, now + kSharedRefillMs - 1) == SHARED_LOCKED); + assert(t.check(B, now + kSharedRefillMs) == ALLOW); + } + // A failure recorded later without a check first still counts in full. + { + Tracker t; uint32_t now = 100000; + t.noteFailure(1, now); + t.noteFailure(2, now += 60000); + for (uint32_t ip = 3; ip < 3 + kSharedBudget - 1; ip++) fail(t, ip, now); + assert(t.check(A, now) == SHARED_LOCKED); + } + // A free slot is reused before an in-use one; otherwise the slot with the + // oldest failure is evicted. + { + Tracker t; uint32_t now = 100000; + fail(t, 1, now); + fail(t, 2, now += kSharedRefillMs); + fail(t, 3, now += kSharedRefillMs); + fail(t, 4, now += kSharedRefillMs); + t.noteSuccess(4); + fail(t, 5, now += kSharedRefillMs); // takes 4's freed slot; 1 keeps its count + for (int i = 0; i < kClientFails - 1; i++) fail(t, 1, now += kSharedRefillMs); + assert(t.check(1, now) == CLIENT_LOCKED); + fail(t, 6, now += kSharedRefillMs); // all in use: evicts 2, the oldest + for (int i = 0; i < kClientFails - 1; i++) fail(t, 2, now += kSharedRefillMs); + assert(t.check(2, now) == ALLOW); + } + puts("WebConfig login throttle policy checks passed"); +} diff --git a/test/fixtures/webconfig_recovery_auth.cpp b/test/fixtures/webconfig_recovery_auth.cpp index a23229e16..e62ff3f28 100644 --- a/test/fixtures/webconfig_recovery_auth.cpp +++ b/test/fixtures/webconfig_recovery_auth.cpp @@ -4,6 +4,7 @@ #include #include #include +#include struct String : std::string { using std::string::string; String(std::string value) : std::string(std::move(value)) {} @@ -21,7 +22,7 @@ struct Callback { }; struct WCLock { explicit WCLock(int) {} }; struct WiFiMock { int softAPIP() const { return 4; } } WiFi; -struct Client { int local_ip=4; int localIP() const { return local_ip; } }; +struct Client { int local_ip=4; int localIP() const { return local_ip; } uint32_t getRemoteAddress() const { return 0; } }; struct Header { String cookies; const String& value() const { return cookies; } }; struct AsyncWebServerResponse { int status; std::map headers; @@ -44,8 +45,8 @@ struct WebConfigServer { int _mode=MODE_SETUP, _mux=0; char _wifi_ssid[33]="SavedNet", _session_token[33] = {}; bool _initial_setup=false, config_accepted=false; - uint32_t _last_activity=0, _session_last_seen=0, _login_lock_until=0; - int _login_fails=0; Callback callback; Callback* _cb=&callback; + uint32_t _last_activity=0, _session_last_seen=0; + WebConfigLoginThrottle::Tracker _login_throttle; Callback callback; Callback* _cb=&callback; WebConfigServer() { strcpy(callback.node.admin_password, "existing-secret"); } unsigned network_activity = 0; void noteNetworkActivity() { ++network_activity; } diff --git a/test/test_webconfig_login_throttle.py b/test/test_webconfig_login_throttle.py new file mode 100644 index 000000000..c0483d778 --- /dev/null +++ b/test/test_webconfig_login_throttle.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +"""Execute the WebConfig login throttle and the real /api/login handler.""" +from pathlib import Path +import shutil +import subprocess +import sys +import tempfile +import unittest + +from test_replay_reset_integration import extract_braced + +ROOT = Path(__file__).resolve().parents[1] +CLIENT_KEY = "req->client() ? req->client()->getRemoteAddress() : 0" + + +class WebConfigLoginThrottleTest(unittest.TestCase): + def compile_run(self, source, includes=(), expect_rejection=False): + compiler = shutil.which("g++") or shutil.which("clang++") + self.assertIsNotNone(compiler) + with tempfile.TemporaryDirectory(prefix="meshcore-webconfig-login-") as temporary: + work = Path(temporary) + generated = work / "login.cpp" + generated.write_text(source) + binary = work / "login" + sanitizers = (["-fsanitize=address,undefined", "-fno-sanitize-recover=all", + "-fno-pie", "-no-pie"] if sys.platform.startswith("linux") else []) + compiled = subprocess.run([compiler, "-std=c++17", "-Wall", "-Wextra", *sanitizers, + "-I" + str(ROOT / "src"), + *("-I" + str(path) for path in includes), + str(generated), "-o", str(binary)], + capture_output=True, text=True, timeout=60) + self.assertEqual(compiled.returncode, 0, compiled.stderr) + result = subprocess.run([str(binary)], capture_output=True, text=True, timeout=30) + if expect_rejection: + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("Assertion", result.stderr) + else: + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def handler_fixture(self): + source = (ROOT / "src/helpers/esp32/WebConfigServer.cpp").read_text() + methods = "\n".join(extract_braced(source, signature) for signature in ( + "static bool fixedTimeEquals(", "void WebConfigServer::handleLogin(")) + self.assertIn(CLIENT_KEY, methods) + return (ROOT / "test/fixtures/webconfig_login_handler.cpp").read_text().replace("@METHODS@", methods) + + def json_include(self): + json_header = next((ROOT / ".pio/libdeps").glob("*/ArduinoJson/src/ArduinoJson.h"), None) + self.assertIsNotNone(json_header, "native dependency headers are required") + return json_header.parent + + def test_policy_isolates_clients_and_bounds_all_clients_together(self): + self.compile_run((ROOT / "test/fixtures/webconfig_login_throttle.cpp").read_text()) + + def test_login_handler_locks_only_the_failing_address(self): + self.compile_run(self.handler_fixture(), (self.json_include(),)) + + def test_old_shared_lockout_fails_executable_regression(self): + # Every client on one key is the old global lockout. + fixture = self.handler_fixture() + old = fixture.replace(CLIENT_KEY, "0", 1) + self.assertNotEqual(old, fixture) + self.compile_run(old, (self.json_include(),), expect_rejection=True) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/test_webconfig_recovery_auth.py b/test/test_webconfig_recovery_auth.py index 092de93e1..0240d1460 100644 --- a/test/test_webconfig_recovery_auth.py +++ b/test/test_webconfig_recovery_auth.py @@ -27,6 +27,7 @@ class WebConfigRecoveryAuthTest(unittest.TestCase): "-fno-pie", "-no-pie"] if sys.platform.startswith("linux") else []) compiled = subprocess.run([compiler, "-std=c++17", "-Wall", "-Wextra", *sanitizers, "-I" + str(json_header.parent), + "-I" + str(ROOT / "src"), str(generated), "-o", str(binary)], capture_output=True, text=True, timeout=60) self.assertEqual(compiled.returncode, 0, compiled.stderr)