diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index 592edee..bb3c1cb 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -5,6 +5,7 @@ - [#77](https://github.com/cjkas/ESPSomfy-RTS/issues/77) Port 80 routes don't check login - [#78](https://github.com/cjkas/ESPSomfy-RTS/issues/78) CORS allows any origin - [#79](https://github.com/cjkas/ESPSomfy-RTS/issues/79) Websocket has no auth +- [#81](https://github.com/cjkas/ESPSomfy-RTS/issues/81) `/login` has no rate limit - [#82](https://github.com/cjkas/ESPSomfy-RTS/issues/82) A saved WiFi passphrase or MQTT password can't be cleared from the UI - [#83](https://github.com/cjkas/ESPSomfy-RTS/issues/83) Installing "Main" from the update dialog always fails - [#84](https://github.com/cjkas/ESPSomfy-RTS/issues/84) Transmitting a frame stops `loop()` for about 365 ms diff --git a/src/ApiToken.h b/src/ApiToken.h index 13cfa7f..9e9fe26 100644 --- a/src/ApiToken.h +++ b/src/ApiToken.h @@ -3,14 +3,16 @@ #include #include -// A token is HMAC-SHA256(token key, PIN or username:password + ":" + client -// IP) in hex. The token key is 32 random bytes the device makes on first boot -// and keeps in NVS: it never leaves the device. +// The token key's size: 32 random bytes the device makes on first boot and +// keeps in NVS. It never leaves the device. #define API_TOKEN_KEY_SIZE 32 +// A token's buffer: an HMAC-SHA256 in hex and its terminator. +#define API_TOKEN_SIZE 65 // True when given is the expected token. It takes the same time whatever // given holds, so the time doesn't tell how much of a guess was right. Only -// expected's length shows, and every token has the same one. +// expected's length shows, and every token has the same one. An empty +// expected token, which is what a failed HMAC leaves, matches nothing. inline bool tokenMatches(const char *expected, const char *given) { if(!expected || !given || !expected[0]) return false; uint8_t diff = 0; diff --git a/src/DeviceOps.cpp b/src/DeviceOps.cpp index 0361008..f96f195 100644 --- a/src/DeviceOps.cpp +++ b/src/DeviceOps.cpp @@ -160,7 +160,7 @@ static void runSave(const proto_reply_to_t &to, settings_save_t *save) { const settings_section_t section = save->section; const uint16_t parts = settingsParts(section, req); bool reboot = false; - char token[65] = ""; + char token[API_TOKEN_SIZE] = ""; const security_types securityBefore = settings.Security.type; switch(section) { case settings_section_t::general: diff --git a/src/entryPoints/Web.cpp b/src/entryPoints/Web.cpp index 23c578b..5cbe89c 100644 --- a/src/entryPoints/Web.cpp +++ b/src/entryPoints/Web.cpp @@ -8,7 +8,6 @@ #include #include #include -#include "ApiToken.h" #include "ConfigSettings.h" #include "ConfigFile.h" #include "Utils.h" @@ -66,11 +65,13 @@ static uint64_t loginNow() { return (uint64_t)esp_timer_get_time() / 1000; } // The tokens' HMAC key. It has its own NVS namespace, which no settings // save clears and no backup holds, so only erasing NVS replaces it. static uint8_t tokenKey[API_TOKEN_KEY_SIZE]; -// Reads the token key, or makes one on first boot. Runs before the radio is -// on, so the hardware RNG gets its entropy from the bootloader's source. +// Reads the token key, or makes one on first boot. Runs before either web +// server starts, so no token is made with an empty key, and before WiFi, so +// the hardware RNG takes its entropy from the bootloader's source: calling it +// once the radio is on would disturb WiFi. static void loadTokenKey() { Preferences pref; - pref.begin("TOKEN"); + if(!pref.begin("TOKEN")) ESP_LOGE(TAG, "Couldn't open the token key's NVS namespace: every token changes at the next boot"); if(pref.getBytes("key", tokenKey, sizeof(tokenKey)) != sizeof(tokenKey)) { bootloader_random_enable(); esp_fill_random(tokenKey, sizeof(tokenKey)); @@ -108,6 +109,7 @@ AsyncWebServer asyncServer(80); AsyncWebServer asyncApiServer(8081); void Web::startup() { ESP_LOGI(TAG, "Launching web server..."); + loadTokenKey(); asyncApiServer.begin(); ESP_LOGI(TAG, "Async API server started on port 8081"); } @@ -120,7 +122,7 @@ bool Web::isAuthenticated(AsyncWebServerRequest *request, bool cfg) { else if(!cfg && (settings.Security.permissions & static_cast(security_permissions::ConfigOnly)) == 0x01) return true; else if(request->hasHeader("apikey")) { ESP_LOGD(TAG, "Checking API Key..."); - char token[65]; + char token[API_TOKEN_SIZE]; memset(token, 0x00, sizeof(token)); this->createAPIToken(request->client()->remoteIP(), token); if(!tokenMatches(token, request->getHeader("apikey")->value().c_str())) { @@ -142,17 +144,24 @@ bool Web::createAPIPinToken(const IPAddress ipAddress, const char *pin, char *to bool Web::createAPIPasswordToken(const IPAddress ipAddress, const char *username, const char *password, char *token) { return this->createAPIToken((String(username) + ":" + String(password) + ":" + ipAddress.toString()).c_str(), token); } +// Writes HMAC-SHA256(token key, payload) in hex to token, which holds +// API_TOKEN_SIZE. The payload is "username:password:ip" under a password, +// "pin:ip" under a PIN and "ip" under None. When mbedtls fails (it allocates +// the HMAC context) token is left empty, which tokenMatches never accepts. bool Web::createAPIToken(const char *payload, char *token) { byte hmacResult[32]; mbedtls_md_context_t ctx; - mbedtls_md_type_t md_type = MBEDTLS_MD_SHA256; mbedtls_md_init(&ctx); - mbedtls_md_setup(&ctx, mbedtls_md_info_from_type(md_type), 1); - mbedtls_md_hmac_starts(&ctx, tokenKey, sizeof(tokenKey)); - mbedtls_md_hmac_update(&ctx, (const unsigned char *)payload, strlen(payload)); - mbedtls_md_hmac_finish(&ctx, hmacResult); + bool ok = mbedtls_md_setup(&ctx, mbedtls_md_info_from_type(MBEDTLS_MD_SHA256), 1) == 0 + && mbedtls_md_hmac_starts(&ctx, tokenKey, sizeof(tokenKey)) == 0 + && mbedtls_md_hmac_update(&ctx, (const unsigned char *)payload, strlen(payload)) == 0 + && mbedtls_md_hmac_finish(&ctx, hmacResult) == 0; mbedtls_md_free(&ctx); token[0] = '\0'; + if(!ok) { + ESP_LOGE(TAG, "Couldn't make a token"); + return false; + } for(int i = 0; i < sizeof(hmacResult); i++){ char str[3]; sprintf(str, "%02x", (int)hmacResult[i]); @@ -161,11 +170,9 @@ bool Web::createAPIToken(const char *payload, char *token) { return true; } bool Web::createAPIToken(const IPAddress ipAddress, char *token) { - String payload; - if(settings.Security.type == security_types::Password) createAPIPasswordToken(ipAddress, settings.Security.username, settings.Security.password, token); - else if(settings.Security.type == security_types::PinEntry) createAPIPinToken(ipAddress, settings.Security.pin, token); - else createAPIToken(ipAddress.toString().c_str(), token); - return true; + if(settings.Security.type == security_types::Password) return createAPIPasswordToken(ipAddress, settings.Security.username, settings.Security.password, token); + if(settings.Security.type == security_types::PinEntry) return createAPIPinToken(ipAddress, settings.Security.pin, token); + return createAPIToken(ipAddress.toString().c_str(), token); } // ===================================================== // Async API Handlers @@ -359,7 +366,7 @@ void Web::handleController(AsyncWebServerRequest *request) { } void Web::handleLogin(AsyncWebServerRequest *request, JsonVariant &json) { if(request->method() == HTTP_OPTIONS) { request->send(200); return; } - char token[65]; + char token[API_TOKEN_SIZE]; memset(&token, 0x00, sizeof(token)); this->createAPIToken(request->client()->remoteIP(), token); if(settings.Security.type == security_types::None) { @@ -397,19 +404,11 @@ void Web::handleLogin(AsyncWebServerRequest *request, JsonVariant &json) { if(!obj["password"].isNull()) strlcpy(password, obj["password"], sizeof(password)); if(!obj["pin"].isNull()) strlcpy(pin, obj["pin"], sizeof(pin)); } - bool success = false; - if(settings.Security.type == security_types::PinEntry) { - char ptok[65]; - memset(ptok, 0x00, sizeof(ptok)); - this->createAPIPinToken(request->client()->remoteIP(), pin, ptok); - success = tokenMatches(token, ptok); - } - else if(settings.Security.type == security_types::Password) { - char ptok[65]; - memset(ptok, 0x00, sizeof(ptok)); - this->createAPIPasswordToken(request->client()->remoteIP(), username, password, ptok); - success = tokenMatches(token, ptok); - } + // The token these credentials would get, against the stored ones'. + char ptok[API_TOKEN_SIZE] = ""; + if(settings.Security.type == security_types::PinEntry) this->createAPIPinToken(ip, pin, ptok); + else if(settings.Security.type == security_types::Password) this->createAPIPasswordToken(ip, username, password, ptok); + bool success = tokenMatches(token, ptok); if(success) { loginThrottle.succeeded(key); snprintf(g_async_content, sizeof(g_async_content), @@ -789,7 +788,6 @@ static void timeRequest(AsyncWebServerRequest *request, ArMiddlewareNext next, c void Web::begin() { ESP_LOGI(TAG, "Creating Web MicroServices..."); - loadTokenKey(); asyncApiServer.addMiddleware([](AsyncWebServerRequest *r, ArMiddlewareNext next) { timeRequest(r, next, "api"); }); asyncServer.addMiddleware([](AsyncWebServerRequest *r, ArMiddlewareNext next) { timeRequest(r, next, "web"); }); // Async API Server (port 8081) diff --git a/src/entryPoints/Web.h b/src/entryPoints/Web.h index 8ab4adf..3317f5e 100644 --- a/src/entryPoints/Web.h +++ b/src/entryPoints/Web.h @@ -1,6 +1,7 @@ #include #include #include "Somfy.h" +#include "ApiToken.h" #ifndef webserver_h #define webserver_h @@ -10,7 +11,7 @@ class Web { void startup(); void begin(); void loop(); - // Auth helpers + // Auth helpers. token holds API_TOKEN_SIZE; false leaves it empty. bool createAPIToken(const IPAddress ipAddress, char *token); bool createAPIToken(const char *payload, char *token); bool createAPIPinToken(const IPAddress ipAddress, const char *pin, char *token); diff --git a/src/entryPoints/WebUiRoutes.cpp b/src/entryPoints/WebUiRoutes.cpp index b2683b5..4159fb4 100644 --- a/src/entryPoints/WebUiRoutes.cpp +++ b/src/entryPoints/WebUiRoutes.cpp @@ -601,7 +601,7 @@ void registerWebUiRoutes(AsyncWebServer &server) { JsonObject obj = json.as(); settings.Security.fromJSON(obj); settings.Security.save(); - char token[65]; + char token[API_TOKEN_SIZE]; webServer.createAPIToken(request->client()->remoteIP(), token); JsonDocument sdoc; JsonObject sobj = sdoc.to(); diff --git a/src/outputs/ProtoSocket.cpp b/src/outputs/ProtoSocket.cpp index 2324865..2ca117f 100644 --- a/src/outputs/ProtoSocket.cpp +++ b/src/outputs/ProtoSocket.cpp @@ -23,7 +23,6 @@ #include "Utils.h" #include "SocketEmitter.h" #include "../entryPoints/Web.h" -#include "ApiToken.h" #if HAS_ETHERNET #include #endif @@ -263,7 +262,7 @@ static bool authorized(AsyncWebServerRequest *request) { if(settings.Security.type == security_types::None) return true; const AsyncWebParameter *key = request->getParam("apikey"); if(!key || !request->client()) return false; - char token[65]; + char token[API_TOKEN_SIZE]; webServer.createAPIToken(request->client()->remoteIP(), token); return tokenMatches(token, key->value().c_str()); } diff --git a/test/e2e/test_auth.py b/test/e2e/test_auth.py index 8997c83..82fa098 100644 --- a/test/e2e/test_auth.py +++ b/test/e2e/test_auth.py @@ -38,22 +38,30 @@ def test_secured_refuses_a_wrong_token(host, secured): assert c.status == 401 -def test_secured_refuses_a_token_keyed_with_the_server_id(request, host, login_context, token): - """The token key used to be the public serverId, so anyone who saw a token could check PIN guesses against it.""" - if token is None: +PASSWORD = 2 # the security type /loginContext reports for a password + + +def test_secured_refuses_a_token_keyed_with_the_server_id(request, host, secured, login_context, token): + """The token key used to be the public serverId, so anyone who saw a token could check PIN guesses against it. + + It works out the token the old way, for this PC's address as the route to + the device picks it. Through NAT or a VPN the device sees another address, + and the test passes without testing anything. + """ + if not secured: pytest.skip("the device's security is None") with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: s.connect((host, 80)) - me = s.getsockname()[0] - if login_context["type"] == 2: - payload = f"{request.config.getoption('--username')}:{request.config.getoption('--password')}:{me}" + my_ip = s.getsockname()[0] + if login_context["type"] == PASSWORD: + payload = f"{request.config.getoption('--username')}:{request.config.getoption('--password')}:{my_ip}" else: - payload = f"{request.config.getoption('--pin')}:{me}" - old = hmac.new(login_context["serverId"].encode(), payload.encode(), hashlib.sha256).hexdigest() + payload = f"{request.config.getoption('--pin')}:{my_ip}" + server_id_token = hmac.new(login_context["serverId"].encode(), payload.encode(), hashlib.sha256).hexdigest() # A bool, so a failure doesn't print the tokens. - same = old == token - assert not same, "the device's token is the one keyed with its serverId" - c = ProtoClient(host, params={"apikey": old}) + is_device_token = server_id_token == token + assert not is_device_token, "the device's token is the one keyed with its serverId" + c = ProtoClient(host, params={"apikey": server_id_token}) assert c.status == 401