diff --git a/GLOSSARY.md b/GLOSSARY.md index 67f3f7c..710ab4b 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -137,7 +137,7 @@ Whether the device asks for login: *None* (everything open) or *Secured* (a PIN _Avoid_: Secure Config Only (removed) **Public route** -What anyone may reach under Secured without a token: the web UI's files, `/loginContext`, `/login`, `/upnp.xml` and preflight requests. Static serving answers the web UI's files alone, so `shades.cfg` isn't one. +What anyone may reach under Secured without a token: the web UI's files, `/loginContext`, `/login`, `/upnp.xml` and preflight requests. `/ws` counts as one at the HTTP level, because the socket checks the token itself in the handshake. Static serving answers the web UI's files alone, so `shades.cfg` isn't one. **Token** What logging in returns. It stands for the PIN or password, on one client address, and every request under Secured carries it. diff --git a/README.md b/README.md index 8004628..b19ddfe 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,8 @@ Since 4.0.0 everything is on port 80. Ports 8081 (HTTP API) and 8080 (events soc A script that called port 8081 only changes the port, if it used one of the three commands. Reads such as `/controller`, `/shades` and `/discovery` are gone: use the socket, or MQTT. The Home Assistant integration needs version 3.0.0 or newer. When the device is Secured (a PIN or password), every route above except the web UI and the login routes needs the token, in an `apikey` header; the socket takes it as `?apikey=`. Without it the reply is 401. "Secure Config Only" is gone: a device set to it is Secured from 4.0.0, so scripts that sent commands without logging in now need the token. + +In either mode the socket refuses an upgrade whose `Origin` header doesn't name the host it was sent to, so another site's page can't open it. Behind a reverse proxy, pass the `Host` header through unchanged. ## Sources for this Project I spent some time reading about a myriad of topics but in the end the primary source for this project comes from https://pushstack.wordpress.com/somfy-rts-protocol/. The work done on pushstack regarding the protocol timing made this feasible without burning a bunch of time measuring pulses. diff --git a/docs/SECURITY_PLAN.md b/docs/SECURITY_PLAN.md index b3139f1..53a6d67 100644 --- a/docs/SECURITY_PLAN.md +++ b/docs/SECURITY_PLAN.md @@ -43,7 +43,7 @@ ### CORS - The wildcard `Access-Control-Allow-*` default headers go. The UI is served from the same origin, and HA and scripts aren't browsers, so CORS doesn't affect them. -- A build flag (`WEB_CORS` in `platformio.ini`) turns the headers back on for UI development from `file://`, which `baseUrl` in `index.js` supports. It also skips the Origin check, since such a page sends `Origin: null`. +- A build flag (`WEB_CORS` in `platformio.ini`) turns the headers back on for UI development from `file://`, which `baseUrl` in `index.js` supports. It also lets the socket take `Origin: null`, which such a page sends. ### UI - The token is kept in `sessionStorage`: it survives a reload, is dropped when the tab closes, and is cleared when the socket upgrade is refused. 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 7ccb633..b482ff5 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 throttle, CORS, the Origin check) is in [SECURITY_PLAN.md](../SECURITY_PLAN.md). +There are two security modes. **None** leaves everything open, except that a socket upgrade from another site's page is refused in either mode (the Origin check). **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/platformio.ini b/platformio.ini index 31c1523..e7e5b60 100644 --- a/platformio.ini +++ b/platformio.ini @@ -43,8 +43,8 @@ build_flags = ; interleave. The core already wraps log_printf for ESP Insights. -Wl,--wrap=log_printfv ; For work on the web UI from a file:// page (baseUrl in index.js): sends - ; the CORS headers and lets the socket skip its Origin check. Never in a - ; release (docs/SECURITY_PLAN.md, CORS). + ; the CORS headers and lets the socket take Origin: null, which such a page + ; sends. Never in a release (docs/SECURITY_PLAN.md, CORS). ; -DWEB_CORS monitor_speed = 115200 monitor_filters = diff --git a/src/HttpAccess.h b/src/HttpAccess.h index f2f0a92..0394413 100644 --- a/src/HttpAccess.h +++ b/src/HttpAccess.h @@ -7,10 +7,11 @@ // (docs/adr/0006-two-security-modes-eleven-http-routes.md). Paths are the // request's path, without the query. -// The files the web UI loads. Static serving answers these alone, so the -// rolling codes in shades.cfg leave the device only through /backup. +// The files the web UI loads, which all sit in the root. Static serving +// answers these alone, so the rolling codes in shades.cfg leave the device +// only through /backup. A path below a route, such as /backup/a.js, isn't one. inline bool isWebAsset(const char *path) { - if(!path || path[0] != '/' || strstr(path, "..")) return false; + if(!path || path[0] != '/' || strchr(path + 1, '/') || strstr(path, "..")) return false; if(strcmp(path, "/") == 0 || strcmp(path, "/appversion") == 0) return true; static const char *const types[] = {".html", ".js", ".css", ".png", ".svg", ".ico"}; const size_t len = strlen(path); diff --git a/src/entryPoints/HttpRoutes.cpp b/src/entryPoints/HttpRoutes.cpp index fbec1ee..440bc6e 100644 --- a/src/entryPoints/HttpRoutes.cpp +++ b/src/entryPoints/HttpRoutes.cpp @@ -52,8 +52,10 @@ static bool uploadAllowed(AsyncWebServerRequest *request, size_t index) { if(index == 0) request->setAttribute("uploadAllowed", webServer.isAuthenticated(request)); return request->getAttribute("uploadAllowed", false); } +// Every route is an exact match. The library's default also takes any path +// below the route, and /backup/a.js then passed the token check as a web asset. void registerHttpRoutes(AsyncWebServer &server) { - server.on("/loginContext", HTTP_GET, [](AsyncWebServerRequest *request) { + server.on(AsyncURIMatcher::exact("/loginContext"), HTTP_GET, [](AsyncWebServerRequest *request) { AsyncJsonResponse *response = new AsyncJsonResponse(); JsonObject root = response->getRoot().to(); root["type"] = static_cast(settings.Security.type); @@ -65,31 +67,31 @@ void registerHttpRoutes(AsyncWebServer &server) { request->send(response); }); // Commands for scripts, as a JSON body or query parameters. - server.addHandler(new AsyncCallbackJsonWebHandler("/shadeCommand", + server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/shadeCommand"), [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleShadeCommand(request, json); })); - server.on("/shadeCommand", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleShadeCommand(request, v); }); + server.on(AsyncURIMatcher::exact("/shadeCommand"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleShadeCommand(request, v); }); - server.addHandler(new AsyncCallbackJsonWebHandler("/groupCommand", + server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/groupCommand"), [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleGroupCommand(request, json); })); - server.on("/groupCommand", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleGroupCommand(request, v); }); + server.on(AsyncURIMatcher::exact("/groupCommand"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleGroupCommand(request, v); }); - server.addHandler(new AsyncCallbackJsonWebHandler("/tiltCommand", + server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/tiltCommand"), [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleTiltCommand(request, json); })); - server.on("/tiltCommand", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleTiltCommand(request, v); }); + server.on(AsyncURIMatcher::exact("/tiltCommand"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleTiltCommand(request, v); }); - server.on("/upnp.xml", HTTP_GET, [](AsyncWebServerRequest *request) { + server.on(AsyncURIMatcher::exact("/upnp.xml"), HTTP_GET, [](AsyncWebServerRequest *request) { AsyncResponseStream *response = request->beginResponseStream("text/xml"); SSDP.schema(*response); request->send(response); }); - server.addHandler(new AsyncCallbackJsonWebHandler("/login", + server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/login"), [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleLogin(request, json); })); - server.on("/login", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleLogin(request, v); }); + server.on(AsyncURIMatcher::exact("/login"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleLogin(request, v); }); - server.on("/backup", HTTP_GET, [](AsyncWebServerRequest *request) { webServer.handleBackup(request); }); + server.on(AsyncURIMatcher::exact("/backup"), HTTP_GET, [](AsyncWebServerRequest *request) { webServer.handleBackup(request); }); - server.on("/restore", HTTP_POST, + server.on(AsyncURIMatcher::exact("/restore"), HTTP_POST, [](AsyncWebServerRequest *request) { if(webServer.uploadSuccess) { restore_options_t opts; @@ -148,7 +150,7 @@ void registerHttpRoutes(AsyncWebServer &server) { }); // updateFirmware - file upload - server.on("/updateFirmware", HTTP_POST, + server.on(AsyncURIMatcher::exact("/updateFirmware"), HTTP_POST, [](AsyncWebServerRequest *request) { if(Update.hasError()) request->send(500, _encoding_json, "{\"status\":\"ERROR\",\"desc\":\"Error updating firmware: \"}"); @@ -190,7 +192,7 @@ void registerHttpRoutes(AsyncWebServer &server) { }); // updateApplication - file upload - server.on("/updateApplication", HTTP_POST, + server.on(AsyncURIMatcher::exact("/updateApplication"), HTTP_POST, [](AsyncWebServerRequest *request) { if(Update.hasError()) request->send(500, _encoding_json, "{\"status\":\"ERROR\",\"desc\":\"Error updating application: \"}"); diff --git a/src/outputs/ProtoSocket.cpp b/src/outputs/ProtoSocket.cpp index ff3a7b2..81c5533 100644 --- a/src/outputs/ProtoSocket.cpp +++ b/src/outputs/ProtoSocket.cpp @@ -263,12 +263,14 @@ static void queueDeviceOp(AsyncWebSocketClient *client, const proto_message_t &m } // A browser can't set headers on a socket, so the token comes in the query. static bool authorized(AsyncWebServerRequest *request) { -#ifndef WEB_CORS - // A UI opened from a file:// page sends Origin: null, so WEB_CORS skips it. const AsyncWebHeader *origin = request->getHeader("Origin"); const AsyncWebHeader *host = request->getHeader("Host"); - if(!originMatchesHost(origin ? origin->value().c_str() : nullptr, host ? host->value().c_str() : nullptr)) return false; + const char *from = origin ? origin->value().c_str() : nullptr; +#ifdef WEB_CORS + // The UI opened from a file:// page sends Origin: null. + if(from && strcmp(from, "null") == 0) from = nullptr; #endif + if(!originMatchesHost(from, host ? host->value().c_str() : nullptr)) return false; const AsyncWebParameter *key = request->getParam("apikey"); return webServer.tokenAccepted(request, key ? key->value().c_str() : nullptr); } diff --git a/test/e2e/test_routes.py b/test/e2e/test_routes.py index bdaee7c..9072568 100644 --- a/test/e2e/test_routes.py +++ b/test/e2e/test_routes.py @@ -4,13 +4,14 @@ mode (docs/adr/0006-two-security-modes-eleven-http-routes.md). Like test_auth, it tests the mode it finds: pass --password or --pin for a Secured device. """ +import json import time import urllib.error import urllib.request import pytest -from proto_client import ProtoClient, heap +from proto_client import ProtoClient # Every route that needs the token under Secured, sent without a body or # parameters, so a route that let one through would still change nothing. @@ -28,23 +29,25 @@ SHADE_FILES = ["/shades.cfg", "/shades.tmp"] def http(host, path, method="GET", body=None, headers=None): - """The status and headers of a request, refusals included.""" + """The status, headers and body of a request, refusals included.""" req = urllib.request.Request(f"http://{host}{path}", data=body, method=method, headers=headers or {}) try: with urllib.request.urlopen(req, timeout=20) as r: - r.read() - return r.status, r.headers + return r.status, r.headers, r.read() except urllib.error.HTTPError as e: - e.read() - return e.code, e.headers + return e.code, e.headers, e.read() -def upload(host, path, headers=None): - """Posts a small file that isn't a backup or an image.""" +def upload(host, path, headers=None, with_file=True): + """Posts a small file that isn't a backup or an image, or a form without a file.""" boundary = "e2e-upload" - body = ( - f"--{boundary}\r\nContent-Disposition: form-data; name=\"file\"; filename=\"e2e.bin\"\r\n" - "Content-Type: application/octet-stream\r\n\r\n").encode() + b"\0" * 256 + f"\r\n--{boundary}--\r\n".encode() + if with_file: + part = ( + f"--{boundary}\r\nContent-Disposition: form-data; name=\"file\"; filename=\"e2e.bin\"\r\n" + "Content-Type: application/octet-stream\r\n\r\n").encode() + b"\0" * 256 + else: + part = f"--{boundary}\r\nContent-Disposition: form-data; name=\"e2e\"\r\n\r\n1".encode() + body = part + f"\r\n--{boundary}--\r\n".encode() return http(host, path, "POST", body, {"Content-Type": f"multipart/form-data; boundary={boundary}", **(headers or {})}) @@ -53,7 +56,7 @@ def test_secured_refuses_a_route_without_a_token(host, secured, method, path): if not secured: pytest.skip("the device's security is None") body = b"{}" if method == "PUT" else None - status, _ = http(host, path, method, body, {"Content-Type": "application/json"} if body else None) + status, _, _ = http(host, path, method, body, {"Content-Type": "application/json"} if body else None) assert status == 401 @@ -65,49 +68,80 @@ def test_secured_refuses_a_route_with_a_wrong_token(host, secured, method, path) headers = {"apikey": "0" * 64} if body: headers["Content-Type"] = "application/json" - status, _ = http(host, path, method, body, headers) + status, _, _ = http(host, path, method, body, headers) assert status == 401 +@pytest.mark.parametrize("method,path", TOKEN_ROUTES + [ + ("POST", "/restore"), ("POST", "/updateFirmware"), ("POST", "/updateApplication")]) +def test_secured_refuses_a_web_asset_name_below_a_route(host, secured, method, path): + """/backup/a.js once reached /backup and passed the token check as a web asset. + + The uploads are sent without a body: one that got through would reboot + the device. + """ + if not secured: + pytest.skip("the device's security is None") + body = b"{}" if method == "PUT" else None + status, _, _ = http(host, path + "/a.js", method, body, {"Content-Type": "application/json"} if body else None) + assert status in (401, 404) + + def test_secured_accepts_the_token_on_a_route(host, secured, token): if not secured: pytest.skip("the device's security is None") - status, _ = http(host, "/backup", headers={"apikey": token}) + status, _, _ = http(host, "/backup", headers={"apikey": token}) assert status == 200 @pytest.mark.parametrize("path", ["/restore", "/updateFirmware"]) -def test_secured_drops_an_upload_without_a_token(host, secured, token, path): - """The token check runs once the body is in, so the uploads check it on their first chunk. +def test_secured_refuses_an_upload_without_a_token(host, secured, path): + """/updateApplication isn't sent: one that got through would write over the file system.""" + if not secured: + pytest.skip("the device's security is None") + status, _, _ = upload(host, path) + assert status == 401 - An /updateFirmware that got through would stop the radio and reboot the - device once the connection closed. /updateApplication isn't sent: one - that got through would write over the file system. + +def restore_without_a_file(host, token): + status, _, body = upload(host, "/restore", {"apikey": token}, with_file=False) + return status, json.loads(body or b"{}").get("desc", "") + + +def test_secured_drops_the_chunks_of_an_upload_without_a_token(host, secured, token): + """The token check runs once the body is in, so /restore's chunks check the token themselves. + + A chunk that got through would mark the upload complete. /restore with + the token and no file would then find the refused file and call it + invalid, instead of saying no upload completed. Neither restores anything. """ if not secured: pytest.skip("the device's security is None") - before = heap(host, token) - status, _ = upload(host, path) + status, desc = restore_without_a_file(host, token) + if status != 400 or "didn't complete" not in desc: + pytest.skip("an earlier upload is still marked complete: reboot the device") + status, _, _ = upload(host, "/restore") assert status == 401 - after = heap(host, token) - assert after["uptime"] >= before["uptime"], "the device rebooted" + status, desc = restore_without_a_file(host, token) + assert status == 400 + assert "didn't complete" in desc, desc @pytest.mark.parametrize("path", PUBLIC_ROUTES) def test_public_routes_need_no_token(host, path): - status, _ = http(host, path) + status, _, _ = http(host, path) assert status == 200 @pytest.mark.parametrize("path", SHADE_FILES) def test_the_shade_files_are_not_served(host, path): """Their rolling codes leave the device only through /backup.""" - status, _ = http(host, path) + status, _, _ = http(host, path) assert status in (401, 404) def test_replies_carry_no_cors_headers(host): - _, headers = http(host, "/loginContext", headers={"Origin": "http://example.com"}) + _, headers, _ = http(host, "/loginContext", headers={"Origin": "http://example.com"}) assert headers.get("Access-Control-Allow-Origin") is None diff --git a/test/test_httpaccess/test_main.cpp b/test/test_httpaccess/test_main.cpp index 4383b0f..db2c7c3 100644 --- a/test/test_httpaccess/test_main.cpp +++ b/test/test_httpaccess/test_main.cpp @@ -34,6 +34,11 @@ static void test_a_path_leaving_the_root_is_not_a_web_asset() { TEST_ASSERT_FALSE(isWebAsset("/../shades.cfg.js")); TEST_ASSERT_FALSE(isWebAsset("/x/../index.js")); } +static void test_a_file_below_the_root_is_not_a_web_asset() { + TEST_ASSERT_FALSE(isWebAsset("/backup/a.js")); + TEST_ASSERT_FALSE(isWebAsset("/js/index.js")); + TEST_ASSERT_FALSE(isWebAsset("//index.js")); +} static void test_a_missing_or_relative_path_is_not_a_web_asset() { TEST_ASSERT_FALSE(isWebAsset(nullptr)); TEST_ASSERT_FALSE(isWebAsset("")); @@ -71,6 +76,12 @@ static void test_a_route_differing_in_case_or_length_needs_the_token() { TEST_ASSERT_FALSE(isPublicRoute("/logins", false)); TEST_ASSERT_FALSE(isPublicRoute("/ws/", false)); } +static void test_a_web_asset_name_below_a_route_needs_the_token() { + TEST_ASSERT_FALSE(isPublicRoute("/backup/a.js", false)); + TEST_ASSERT_FALSE(isPublicRoute("/shadeCommand/a.png", false)); + TEST_ASSERT_FALSE(isPublicRoute("/updateFirmware/x.js", false)); + TEST_ASSERT_FALSE(isPublicRoute("/login/a.js", false)); +} static void test_a_preflight_is_public() { TEST_ASSERT_TRUE(isPublicRoute("/backup", true)); } @@ -118,6 +129,7 @@ int main(int, char **) { RUN_TEST(test_a_compressed_file_is_not_asked_for_by_name); RUN_TEST(test_a_type_alone_is_not_a_web_asset); RUN_TEST(test_a_path_leaving_the_root_is_not_a_web_asset); + RUN_TEST(test_a_file_below_the_root_is_not_a_web_asset); RUN_TEST(test_a_missing_or_relative_path_is_not_a_web_asset); RUN_TEST(test_logging_in_and_ssdp_are_public); RUN_TEST(test_the_socket_checks_its_own_token); @@ -125,6 +137,7 @@ int main(int, char **) { RUN_TEST(test_the_other_routes_need_the_token); RUN_TEST(test_an_unknown_path_needs_the_token); RUN_TEST(test_a_route_differing_in_case_or_length_needs_the_token); + RUN_TEST(test_a_web_asset_name_below_a_route_needs_the_token); RUN_TEST(test_a_preflight_is_public); RUN_TEST(test_the_page_the_device_served_matches); RUN_TEST(test_https_behind_a_proxy_matches);