From ff7517e10f14beb3fa574227417a59ea04518ff8 Mon Sep 17 00:00:00 2001 From: cjkas Date: Tue, 27 Oct 2026 18:44:39 +0100 Subject: [PATCH] fix: review findings on the login throttle - The throttle runs on a 64-bit clock in ms (esp_timer_get_time()), so a slot left alone for 49.7 days isn't brought back by millis() wrapping round to its old failure. - Web.cpp turns the client into the throttle's key in one place, and says that an IPv6 client would share key 0; names are clearer (retryMs, vacantSlot). - The shared slot's test now succeeds from the IP counted in it, so a success that cleared the shared slot would fail it. - GLOSSARY.md names it the login throttle, with lockouts, and SECURITY_PLAN.md and ADR 0006 use that name. Co-Authored-By: Claude Opus 5.5 --- GLOSSARY.md | 4 +++ docs/SECURITY_PLAN.md | 6 ++-- ...6-two-security-modes-eleven-http-routes.md | 2 +- src/LoginThrottle.h | 30 ++++++++++--------- src/entryPoints/Web.cpp | 17 +++++++---- test/test_loginthrottle/test_main.cpp | 24 ++++++++++----- 6 files changed, 53 insertions(+), 30 deletions(-) diff --git a/GLOSSARY.md b/GLOSSARY.md index 5fa4f78..85564f0 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -148,6 +148,10 @@ _Avoid_: Secure Config Only (removed) What logging in returns. It stands for the PIN or password, on one client address, and every request under Secured carries it. _Avoid_: session, API key (the name of the field it travels in) +**Login throttle** +How the device slows down guessing: a client address with too many failed logins is refused for a while, and each refusal is a *lockout*, longer than the one before. +_Avoid_: login backoff, rate limit + ## Domain modules **Domain module** diff --git a/docs/SECURITY_PLAN.md b/docs/SECURITY_PLAN.md index 9853364..72de503 100644 --- a/docs/SECURITY_PLAN.md +++ b/docs/SECURITY_PLAN.md @@ -54,7 +54,7 @@ - **Comparison:** constant-time, used for HTTP and the websocket handshake alike. - **Migration:** none needed. The browser logs in again when its token is refused, and HA logs in again at runtime. -### Login throttling (PR C) +### Login throttle (PR C) - Per client IP: after 5 failed `/login` attempts, refuse that IP for 30 s, doubling each time up to 15 min. A success resets it. - After the first lockout, each failure starts the next one, so an IP gets one try per lockout rather than five. - An hour without a failure forgets the IP. @@ -87,7 +87,7 @@ Protocol 2 lands as one PR per step, and `main` keeps working throughout because | 3 | HA | protocol 2 with the old-firmware fallback; `/loginContext` setup; runtime login and reauth | | 4 | firmware | removals: the old routes, ports 8081 and 8080, `/discovery`, ModelSnapshot, "Secure Config Only"; static serving limited to web assets; CORS headers; Origin check | -PR B (token key) and PR C (login backoff) are separate firmware PRs that can land at any point. The GitHub update was removed ahead of the rest ([ADR 0007](adr/0007-no-github-update.md)), as protocol 2 didn't fit beside it. +PR B (token key) and PR C (login throttle) are separate firmware PRs that can land at any point. The GitHub update was removed ahead of the rest ([ADR 0007](adr/0007-no-github-update.md)), as protocol 2 didn't fit beside it. ## Tests @@ -95,7 +95,7 @@ The rules go into small pure helpers with native tests, following [src/Secrets.h - which routes are public - the token comparison - the Origin/Host match -- the backoff schedule +- the login throttle's schedule The e2e suite (`test/e2e`) checks, when given credentials, that a socket without a token is refused, that the HTTP routes answer 401, and that a token works. It never changes the device's security settings itself. diff --git a/docs/adr/0006-two-security-modes-eleven-http-routes.md b/docs/adr/0006-two-security-modes-eleven-http-routes.md index cff72c5..7ccb633 100644 --- a/docs/adr/0006-two-security-modes-eleven-http-routes.md +++ b/docs/adr/0006-two-security-modes-eleven-http-routes.md @@ -18,7 +18,7 @@ With [ADR 0005](0005-one-websocket-protocol.md) moving everything else to the we Static serving covers the web assets only, so `shades.cfg` and the other config files are never served. The script commands keep today's full field names, since people write those requests by hand. `/discovery` goes: Home Assistant's setup reads the device info from `/loginContext`, and mDNS still advertises `serverId`. -There are two security modes. **None** leaves everything open. **Secured** (PIN or password) needs the token on the socket upgrade (`?apikey=`) and on every route except the static files, `/loginContext`, `/login` and `/upnp.xml`. Devices set to "Secure Config Only" become Secured when they upgrade. The rest of the security work (the token key, the login backoff, CORS, the Origin check) is in [SECURITY_PLAN.md](../SECURITY_PLAN.md). +There are two security modes. **None** leaves everything open. **Secured** (PIN or password) needs the token on the socket upgrade (`?apikey=`) and on every route except the static files, `/loginContext`, `/login` and `/upnp.xml`. Devices set to "Secure Config Only" become Secured when they upgrade. The rest of the security work (the token key, the login throttle, CORS, the Origin check) is in [SECURITY_PLAN.md](../SECURITY_PLAN.md). ## Considered Options diff --git a/src/LoginThrottle.h b/src/LoginThrottle.h index 7224a27..b1aeb51 100644 --- a/src/LoginThrottle.h +++ b/src/LoginThrottle.h @@ -13,8 +13,10 @@ // get the free failures on each: while the table is full, an IP it doesn't // hold may have to wait even with the right credentials. // -// The time is passed in (millis()), so native tests cover it. Only -// async_tcp's task calls it: both web servers run their handlers there. +// The time is passed in, in ms on a 64-bit clock (esp_timer_get_time() / +// 1000), so native tests cover it and a slot left alone for 49.7 days isn't +// woken by millis() wrapping. Only async_tcp's task calls it: both web +// servers run their handlers there. #define LOGIN_FREE_FAILURES 5 #define LOGIN_FIRST_LOCKOUT_MS 30000UL @@ -26,20 +28,20 @@ struct login_attempts_t { bool used = false; uint32_t ip = 0; uint16_t failures = 0; - uint32_t lastFailure = 0; // millis() of the last failure + uint64_t lastFailure = 0; // the time of the last failure uint32_t lockoutMs = 0; // the lockout that failure began, 0 for none }; class LoginThrottle { public: // How long ip must wait before it may try again, in ms; 0 when it may now. - uint32_t retryAfter(uint32_t ip, uint32_t now) const { + uint32_t retryAfter(uint32_t ip, uint64_t now) const { const login_attempts_t *a = this->find(ip, now); return a ? remaining(*a, now) : 0; } // A wrong PIN or password from ip. Returns the lockout it begins, in ms, // or 0 while ip still has free failures. - uint32_t failed(uint32_t ip, uint32_t now) { + uint32_t failed(uint32_t ip, uint64_t now) { login_attempts_t *a = this->claim(ip, now); if(a->failures < UINT16_MAX) a->failures++; a->lastFailure = now; @@ -63,18 +65,18 @@ class LoginThrottle { login_attempts_t slots[LOGIN_THROTTLE_SLOTS]; login_attempts_t shared; - static uint32_t remaining(const login_attempts_t &a, uint32_t now) { + static uint32_t remaining(const login_attempts_t &a, uint64_t now) { if(!a.used || a.lockoutMs == 0) return 0; - uint32_t elapsed = now - a.lastFailure; - return elapsed < a.lockoutMs ? a.lockoutMs - elapsed : 0; + uint64_t elapsed = now - a.lastFailure; + return elapsed < a.lockoutMs ? (uint32_t)(a.lockoutMs - elapsed) : 0; } // Unused, or its last failure is old enough to forget. - static bool vacant(const login_attempts_t &a, uint32_t now) { + static bool vacant(const login_attempts_t &a, uint64_t now) { return !a.used || now - a.lastFailure >= LOGIN_FORGET_MS; } // ip's record: its own slot, nothing while a slot is vacant, else the // shared one. - const login_attempts_t *find(uint32_t ip, uint32_t now) const { + const login_attempts_t *find(uint32_t ip, uint64_t now) const { bool anyVacant = false; for(const auto &a : this->slots) { if(vacant(a, now)) anyVacant = true; @@ -84,13 +86,13 @@ class LoginThrottle { return &this->shared; } // ip's record to count a failure in, taking a vacant slot when it has none. - login_attempts_t *claim(uint32_t ip, uint32_t now) { - login_attempts_t *free = nullptr; + login_attempts_t *claim(uint32_t ip, uint64_t now) { + login_attempts_t *vacantSlot = nullptr; for(auto &a : this->slots) { - if(vacant(a, now)) { if(!free) free = &a; } + if(vacant(a, now)) { if(!vacantSlot) vacantSlot = &a; } else if(a.ip == ip) return &a; } - login_attempts_t *a = free ? free : &this->shared; + login_attempts_t *a = vacantSlot ? vacantSlot : &this->shared; if(vacant(*a, now)) { *a = login_attempts_t(); a->used = true; diff --git a/src/entryPoints/Web.cpp b/src/entryPoints/Web.cpp index 8475f7b..f75adb8 100644 --- a/src/entryPoints/Web.cpp +++ b/src/entryPoints/Web.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include "esp_log.h" #include "mbedtls/md.h" #include "ConfigSettings.h" @@ -52,6 +53,11 @@ static const char *TAG = "Web"; // Failed logins per client IP, on both web servers. static LoginThrottle loginThrottle; +// The throttle's key for a client: its IPv4 address. The firmware doesn't +// enable IPv6; an IPv6 client would share key 0 with every other one. +static uint32_t loginKey(const IPAddress &ip) { return (uint32_t)ip; } +// The throttle's clock, in ms, which doesn't wrap as millis() does. +static uint64_t loginNow() { return (uint64_t)esp_timer_get_time() / 1000; } // Hands an item to loop(), which owns the shades. When it can't be the // request is answered with the reason and false is returned. @@ -341,9 +347,10 @@ void Web::handleLogin(AsyncWebServerRequest *request, JsonVariant &json) { } // A refused IP isn't told whether these credentials were right. IPAddress ip = request->client()->remoteIP(); - uint32_t wait = loginThrottle.retryAfter((uint32_t)ip, millis()); - if(wait) { - uint32_t secs = (wait + 999) / 1000; + uint32_t key = loginKey(ip); + uint32_t retryMs = loginThrottle.retryAfter(key, loginNow()); + if(retryMs) { + uint32_t secs = (retryMs + 999) / 1000; snprintf(g_async_content, sizeof(g_async_content), "{\"type\":%u,\"msg\":\"Too many failed logins. Try again in %lu s.\",\"success\":false,\"retryAfter\":%lu}", static_cast(settings.Security.type), (unsigned long)secs, (unsigned long)secs); @@ -380,14 +387,14 @@ void Web::handleLogin(AsyncWebServerRequest *request, JsonVariant &json) { if(String(ptok) == String(token)) success = true; } if(success) { - loginThrottle.succeeded((uint32_t)ip); + loginThrottle.succeeded(key); snprintf(g_async_content, sizeof(g_async_content), "{\"type\":%u,\"apiKey\":\"%s\",\"msg\":\"Success\",\"success\":true}", static_cast(settings.Security.type), token); request->send(200, _encoding_json, g_async_content); } else { - uint32_t lockout = loginThrottle.failed((uint32_t)ip, millis()); + uint32_t lockout = loginThrottle.failed(key, loginNow()); if(lockout) ESP_LOGW(TAG, "Failed login from %s: refused for %lu s", ip.toString().c_str(), (unsigned long)(lockout / 1000)); snprintf(g_async_content, sizeof(g_async_content), "{\"type\":%u,\"msg\":\"Invalid credentials\",\"success\":false}", diff --git a/test/test_loginthrottle/test_main.cpp b/test/test_loginthrottle/test_main.cpp index 5115fca..95bf725 100644 --- a/test/test_loginthrottle/test_main.cpp +++ b/test/test_loginthrottle/test_main.cpp @@ -8,7 +8,7 @@ static const uint32_t IP_A = 0x0A01A8C0; // 192.168.1.10 static const uint32_t IP_B = 0x0B01A8C0; // 192.168.1.11 // Fails ip n times at now, as fast as the throttle lets it. -static void failTimes(LoginThrottle &t, uint32_t ip, int n, uint32_t now) { +static void failTimes(LoginThrottle &t, uint32_t ip, int n, uint64_t now) { for(int i = 0; i < n; i++) t.failed(ip, now); } @@ -73,16 +73,24 @@ static void test_an_hour_without_a_failure_forgets_the_ip() { failTimes(t, IP_A, 7, 0); // Still counted just before the hour: the next failure doubles. TEST_ASSERT_EQUAL_UINT32(240000, t.failed(IP_A, LOGIN_FORGET_MS - 1)); - uint32_t later = 2 * LOGIN_FORGET_MS; + uint64_t later = 2 * LOGIN_FORGET_MS; TEST_ASSERT_EQUAL_UINT32(0, t.failed(IP_A, later)); } -static void test_millis_wrapping_keeps_the_lockout() { +static void test_a_lockout_spans_where_millis_would_wrap() { LoginThrottle t; - uint32_t now = UINT32_MAX - 10000; + uint64_t now = UINT32_MAX - 10000ULL; failTimes(t, IP_A, 5, now); TEST_ASSERT_EQUAL_UINT32(10000, t.retryAfter(IP_A, now + 20000)); TEST_ASSERT_EQUAL_UINT32(0, t.retryAfter(IP_A, now + 30000)); } +static void test_a_slot_left_alone_for_50_days_stays_forgotten() { + LoginThrottle t; + failTimes(t, IP_A, 10, 1000); + // Where a 32-bit millis() would have come round to the same value. + uint64_t wrapped = 1000 + (1ULL << 32); + TEST_ASSERT_EQUAL_UINT32(0, t.retryAfter(IP_A, wrapped)); + TEST_ASSERT_EQUAL_UINT32(0, t.failed(IP_A, wrapped)); +} /********************************************************************* * Several IPs @@ -113,7 +121,7 @@ static void test_a_forgotten_slot_is_taken_again() { LoginThrottle t; for(uint32_t i = 0; i < LOGIN_THROTTLE_SLOTS; i++) failTimes(t, 100 + i, 5, 0); // An hour later every slot is vacant: a new IP has its own free failures. - uint32_t later = LOGIN_FORGET_MS; + uint64_t later = LOGIN_FORGET_MS; TEST_ASSERT_EQUAL_UINT32(0, t.retryAfter(IP_A, later)); failTimes(t, IP_A, 4, later); TEST_ASSERT_EQUAL_UINT32(30000, t.failed(IP_A, later)); @@ -124,7 +132,8 @@ static void test_a_success_doesnt_clear_the_shared_slot() { LoginThrottle t; for(uint32_t i = 0; i < LOGIN_THROTTLE_SLOTS; i++) failTimes(t, 100 + i, 1, 0); failTimes(t, 200, 5, 0); - t.succeeded(201); + // 200 is counted in the shared slot, not one of its own. + t.succeeded(200); TEST_ASSERT_EQUAL_UINT32(30000, t.retryAfter(202, 0)); } @@ -139,7 +148,8 @@ int main(int argc, char **argv) { RUN_TEST(test_after_a_lockout_the_next_failure_locks_out_twice_as_long); RUN_TEST(test_a_success_resets_the_ip); RUN_TEST(test_an_hour_without_a_failure_forgets_the_ip); - RUN_TEST(test_millis_wrapping_keeps_the_lockout); + RUN_TEST(test_a_lockout_spans_where_millis_would_wrap); + RUN_TEST(test_a_slot_left_alone_for_50_days_stays_forgotten); RUN_TEST(test_one_ip_locked_out_leaves_the_others_alone); RUN_TEST(test_a_success_resets_only_its_own_ip); RUN_TEST(test_with_the_table_full_other_ips_share_one_slot);