diff --git a/scripts/webconfig_cli_audit.py b/scripts/webconfig_cli_audit.py index 14294334..3cd24db4 100644 --- a/scripts/webconfig_cli_audit.py +++ b/scripts/webconfig_cli_audit.py @@ -105,7 +105,10 @@ ROUND_TRIPS = [ ("set mqtt.neighbors on", "get mqtt.neighbors", "on"), ("set path.hash.mode 2", "get path.hash.mode", "2"), ("set mqtt.iata den", "get mqtt.iata", "DEN"), - ("set guest.password hunter2", "get guest.password", "********"), + # Secret reads are masked back down for an HTTP caller, in CommonCLI's own + # words for a non-serial one (wcIsSecretReadCommand). + ("set guest.password hunter2", "get guest.password", "******** (serial only)"), + ("set wifi.pwd hunter2", "get wifi.pwd", "******** (serial only)"), ] diff --git a/scripts/webconfig_mock_server.py b/scripts/webconfig_mock_server.py index 3631cdc9..c38d804e 100644 --- a/scripts/webconfig_mock_server.py +++ b/scripts/webconfig_mock_server.py @@ -156,6 +156,7 @@ class State: self.session = None # cookie token when logged in (LAN mode) self.batch = {"state": "idle"} self.cli = {"state": "idle"} # deferred CLI sequence, see /api/cli + self.admin_pwd_set = False # satisfies the initial-setup invariant self.scan_started = None # ---- auth ------------------------------------------------------------- @@ -467,6 +468,17 @@ def cli_unavailable(cmd): # Failure replies CommonCLI emits that do NOT start with "Err" — the shapes that # made a naive prefix test call them success. Mirrors # WebConfigBatch::cliReplyIsFailure. +def cli_reads_secret(cmd): + """Commands that READ a secret. CommonCLI gates these on the caller being + the serial console; the portal is not, so the value is masked here the way + CommonCLI masks it for remote callers. Mirrors wcCliReadsSecret().""" + if not cmd.startswith("get "): + return False + key = cmd[4:].strip() + return key in ("prv.key", "guest.password", "alert.psk", "bridge.secret") \ + or is_secret_key(key) + + def cli_reply_is_failure(reply): if not reply: return False @@ -907,6 +919,15 @@ class Handler(BaseHTTPRequestHandler): why = cli_unavailable(c) if why: return self._json(400, {"error": why}) + # Same invariant handleConfigPost enforces (see wcCliUnavailable's + # neighbour in WebConfigServer.cpp): first onboarding is committed by the + # reboot, and must not commit the factory password onto someone's LAN. + if (ST.setup_mode and ST.initial_setup and not ST.admin_pwd_set + and not any(c.startswith("password ") for c in cmds) + and (any(c.startswith(CLI_DEFERRED_REBOOT) for c in cmds) + or any(c.startswith("set wifi.ssid ") for c in cmds))): + return self._json(400, {"error": "admin password required for initial setup — " + "run `password ` first"}) with ST.lock: self._cli_advance(ST.cli) @@ -936,6 +957,11 @@ class Handler(BaseHTTPRequestHandler): _, reply = run_cli(ST.cfg, cmd) if cmd.startswith("password "): reply = "OK" # never echo the new password back + ST.admin_pwd_set = True + elif cli_reads_secret(cmd): + val = reply[2:] if reply.startswith("> ") else reply + reply = ("> (not set)" if val in ("", "(not set)") + else "> ******** (serial only)") ok = not cli_reply_is_failure(reply) # Only writes gate the reboot, and only on the "OK" convention every # setter keeps (WebConfigBatch::cliReplyGatesReboot). diff --git a/src/helpers/WebConfigKeys.h b/src/helpers/WebConfigKeys.h index 629df352..47fd4149 100644 --- a/src/helpers/WebConfigKeys.h +++ b/src/helpers/WebConfigKeys.h @@ -82,6 +82,28 @@ static inline bool wcIsSecretKey(const char* key) { return false; } +// CommonCLI answers a secret getter in plaintext only for the serial console +// (sender_timestamp 0) and masks it for remote callers. The web CLI executes +// with sender_timestamp 0 — that is what makes `erase`, `stats-*` and `set freq` +// reachable — so it would otherwise inherit the serial console's plaintext +// answers for an HTTP request. This says which `get` commands must be masked +// back down, restoring the distinction for a caller not at the serial port. +// +// Writing these has always been possible from the portal; reading them never +// was, because handleConfigGet masks them (wcIsSecretKey). The two are different +// capabilities: replacing a WiFi password does not reveal the current one, and +// replacing an identity does not reveal the existing private key. +static inline bool wcIsSecretReadCommand(const char* cmd) { + if (strncmp(cmd, "get ", 4) != 0) return false; + const char* key = cmd + 4; + while (*key == ' ') key++; + if (strcmp(key, "prv.key") == 0) return true; // this node's identity + if (strcmp(key, "guest.password") == 0) return true; + if (strcmp(key, "alert.psk") == 0) return true; + if (strcmp(key, "bridge.secret") == 0) return true; + return wcIsSecretKey(key); // wifi.pwd, mqttN.password, mqttN.token +} + // Browser-generated request IDs are exactly eight random bytes encoded as // hexadecimal. Keeping the grammar deliberately small makes the ID safe to // echo in JSON/logs and prevents an empty or truncated ID from weakening the diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index e2a279dc..5aecb02e 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -79,6 +79,34 @@ static const char* wcCliUnavailable(const char* cmd) { static inline bool wcCliEchoesSecret(const char* cmd) { return strncmp(cmd, "password ", 9) == 0; } + +// CommonCLI splits its surface by CALLER, not by command: a serial caller +// (sender_timestamp 0, physical access) reads secrets in plaintext, while a +// remote one gets "******** (serial only)". Its own comments say so — "Serial +// only (WiFi creds grant LAN access); remote sees set/unset". +// +// execCommand passes 0, which is what makes `erase`, `stats-*` and `set freq` +// reachable from the terminal at all. Left alone, that also claims +// physical-access trust for an HTTP request: `get prv.key` would hand this +// node's identity to anyone associated with the open setup AP, and `get +// wifi.pwd` would hand over the operator's network. Reading a secret and +// writing one are not the same capability — the wizard has always been able to +// REPLACE these; nothing in the portal could ever READ them, because +// /api/config masks them (wcIsSecretKey). +// +// So the command surface stays whole and only the READ is masked, restoring the +// distinction CommonCLI intended for a caller who is not at the serial port. +// Which commands those are lives in WebConfigKeys.h (wcIsSecretReadCommand), +// beside the rest of the secret classification and host-tested with it. + +// Keep the set/unset signal, which is the useful part and what CommonCLI itself +// reports remotely; only the value goes. A getter answers "> value". +static void wcMaskSecretReply(char* reply) { + const char* val = reply; + if (val[0] == '>' && val[1] == ' ') val += 2; + const bool unset = (val[0] == 0 || strcmp(val, "(not set)") == 0); + strcpy(reply, unset ? "> (not set)" : "> ******** (serial only)"); +} // Reply classification lives in WebConfigBatch.h with the rest of the decisions // (WebConfigBatch::cliReplyIsFailure / cliReplyGatesReboot / cliWriteSucceeded), // so the shapes CommonCLI actually emits are enumerated in one host-tested place. @@ -388,7 +416,13 @@ void WebConfigServer::drainBatch(uint32_t now) { // reply, and replies are served to the client over the open setup AP. // Overwrite it: the command cannot fail, so there is nothing to report. // Config entries carry the key; a CLI entry is matched on the command. - if (wcIsAdminPasswordKey(e.key) || wcCliEchoesSecret(e.cmd)) strcpy(e.reply, "OK"); + const bool set_admin_pwd = wcIsAdminPasswordKey(e.key) || wcCliEchoesSecret(e.cmd); + if (set_admin_pwd) strcpy(e.reply, "OK"); + // Satisfies the initial-setup invariant for the rest of this session, so + // the operator can set the password and configure WiFi in separate steps + // (the form batch always sends them together and needs no such memory). + if (set_admin_pwd) _admin_pwd_set = true; + if (_batch_kind == BATCH_CLI && wcIsSecretReadCommand(e.cmd)) wcMaskSecretReply(e.reply); // What gates the reboot is narrower than "did anything fail": only a // write can leave the node in a config not worth rebooting into, and only // a write has a reply convention ("OK") solid enough to test. Diagnostics @@ -1047,7 +1081,7 @@ void WebConfigServer::handleCliPost(AsyncWebServerRequest* req) { } int count = 0; - bool defer_reboot = false; + bool defer_reboot = false, seq_sets_pwd = false, seq_sets_ssid = false; for (JsonVariant v : cmds) { const char* raw = v.as(); if (!raw) continue; @@ -1089,6 +1123,8 @@ void WebConfigServer::handleCliPost(AsyncWebServerRequest* req) { } e.key[0] = 0; // CLI entries have no config key if (wcIsDeferredReboot(e.cmd)) defer_reboot = true; + if (strncmp(e.cmd, "password ", 9) == 0) seq_sets_pwd = true; + if (strncmp(e.cmd, "set wifi.ssid ", 14) == 0) seq_sets_ssid = true; count++; } if (count == 0) { @@ -1096,6 +1132,19 @@ void WebConfigServer::handleCliPost(AsyncWebServerRequest* req) { return; } + // The same invariant handleConfigPost enforces, and for the same reason: the + // reboot is what commits first onboarding, and a node that reboots onto the + // LAN still holding the factory password is a known credential on someone + // else's network. The terminal warned about this client-side, which is a + // reminder, not a rule — a pasted script or a direct POST ignored it. + if (_mode == MODE_SETUP && _initial_setup && !seq_sets_pwd && !_admin_pwd_set && + (defer_reboot || seq_sets_ssid)) { + req->send(400, "application/json", + "{\"error\":\"admin password required for initial setup — " + "run `password ` first\"}"); + return; + } + _batch_kind = BATCH_CLI; _batch_count = count; _batch_next = 0; diff --git a/src/helpers/esp32/WebConfigServer.h b/src/helpers/esp32/WebConfigServer.h index e1191fc3..0dcebebd 100644 --- a/src/helpers/esp32/WebConfigServer.h +++ b/src/helpers/esp32/WebConfigServer.h @@ -136,6 +136,10 @@ private: bool _stopping = false; bool _was_setup_ap = false; bool _initial_setup = false; + // A `password` command has succeeded this session. Lets the CLI satisfy the + // initial-setup invariant across separate submissions; the form batch always + // sends the password with the rest, so it never needed the memory. + bool _admin_pwd_set = false; char _ap_ssid[33] = {0}; // Currently attached session, also used by the display's setup-info poll. diff --git a/test/test_webconfig_keys/test_webconfig_keys.cpp b/test/test_webconfig_keys/test_webconfig_keys.cpp index ce78884b..3903453b 100644 --- a/test/test_webconfig_keys/test_webconfig_keys.cpp +++ b/test/test_webconfig_keys/test_webconfig_keys.cpp @@ -119,6 +119,45 @@ TEST(WebConfigKeys, EverySecretKeyIsAlsoAllowed) { } } +// ---- CLI secret reads ---------------------------------------------------- +// The web CLI runs commands with sender_timestamp 0, which is how CommonCLI +// recognises the serial console and answers secret getters in plaintext. These +// are the reads that must be masked back down for an HTTP caller. + +TEST(WebConfigKeys, MasksEverySecretReadTheCliCanReach) { + const char* masked[] = { + "get prv.key", // this node's identity — the worst one to leak + "get wifi.pwd", // grants the operator's LAN, not just the node + "get guest.password", + "get alert.psk", + "get bridge.secret", + "get mqtt1.password", "get mqtt1.token", + "get mqtt6.password", "get mqtt6.token", + "get wifi.pwd", // extra space after the verb + }; + for (const char* c : masked) EXPECT_TRUE(wcIsSecretReadCommand(c)) << c; +} + +TEST(WebConfigKeys, DoesNotMaskReadsThatCarryNoSecret) { + const char* plain[] = { + "get wifi.ssid", "get mqtt1.username", "get mqtt1.server", "get tx", + "get public.key", // public half, safe to read + "get mqtt.owner", // an owner's public key, not a credential + }; + for (const char* c : plain) EXPECT_FALSE(wcIsSecretReadCommand(c)) << c; +} + +TEST(WebConfigKeys, OnlyMasksReads) { + // Writing a secret has always been the portal's job and reveals nothing; + // only the read is restricted. Nor may a prefix be mistaken for a `get`. + EXPECT_FALSE(wcIsSecretReadCommand("set wifi.pwd hunter2")); + EXPECT_FALSE(wcIsSecretReadCommand("set prv.key aabb")); + EXPECT_FALSE(wcIsSecretReadCommand("password hunter2")); + EXPECT_FALSE(wcIsSecretReadCommand("getwifi.pwd")); + EXPECT_FALSE(wcIsSecretReadCommand("get")); + EXPECT_FALSE(wcIsSecretReadCommand("")); +} + // ---- request correlation ------------------------------------------------- TEST(WebConfigKeys, AcceptsExactHexRequestIds) { diff --git a/webui/index.html b/webui/index.html index 261d67e4..ab7b43d6 100644 --- a/webui/index.html +++ b/webui/index.html @@ -1926,8 +1926,10 @@ function cliConfirm(cmds){ // rebooting is what commits the node to normal operation. if(st.mode==="setup"&&!cli.pwd&&cmds.some(function(c){return /^(reboot|clkreboot)$/.test(c)})){ var p=document.createElement("div");p.className="w"; - p.textContent="⚠ No password command has run here. Set the admin password before rebooting, "+ - "or the node keeps the factory one."; + // The node enforces this too (handleCliPost) — this is the earlier, kinder + // half of the same rule, so the operator finds out before sending. + p.textContent="⚠ Set the admin password first: run \"password \". "+ + "The node will refuse to finish setup on the factory one."; box.appendChild(p); } var btns=document.createElement("div");btns.className="btns";