diff --git a/docs/daemon-startup-hardening.md b/docs/daemon-startup-hardening.md index 16573ed..2f0076a 100644 --- a/docs/daemon-startup-hardening.md +++ b/docs/daemon-startup-hardening.md @@ -15,6 +15,12 @@ to verify it. Status legend: ☐ not started · ◐ in progress · ☑ landed & verified +**Status: all 8 landed & verified** (build-clean, `ctest` green after each) across four commits on +`dev` — lifecycle cluster (F1/F2/F4), filesystem+params cluster (F7/F6/F5), F3, and F8. Six new +pure-helper unit tests added. Still owed before release: the F1/F2 manual repros (`kill -SEGV` / +non-executable binary — not unit-testable), the deferred `res/lang/*.json` i18n back-fill (one +`add_missing_translations.py` run), and **release notes for F8's breaking default flip**. + --- ## Recommended rollout sequence @@ -32,7 +38,7 @@ around a single shared helper. The connectivity-breaking security flip lands las | 4 | **F7** | `util/platform` · `connection.cpp` | Structural owner of the fs-error idiom + `ConnectionConfig` that F5/F6/F8 reuse. | ☑ | | 5 | **F6 + F5** | `app.cpp` · `verifySaplingParams()` | Same `startEmbeddedDaemon` / `verifySaplingParams` block; land together. | ☑ | | 6 | **F3** | `app.cpp` · `renderLoadingOverlay()` | After F1 — panel is guarded off during `State::Error` (owned by the crash-count hint). | ☑ | -| 7 | **F8** | `connection.cpp` · `tryConnect()` | Largest; only connectivity-breaking default flip — land last, with release notes. | ☐ | +| 7 | **F8** | `connection.cpp` · `tryConnect()` | Largest; only connectivity-breaking default flip — land last, with release notes. | ☑ | --- @@ -396,7 +402,21 @@ extension (coordinated with F8). Land before F5/F6/F8. ## F8 — Plaintext-remote RPC credential transmission is warn-only -**Severity:** Medium · **Effort:** M (~6–9h) · **Status:** ☐ +**Severity:** Medium · **Effort:** M (~6–9h) · **Status:** ☑ landed & verified + +> **⚠️ RELEASE NOTES REQUIRED — breaking default flip.** A wallet configured to talk to a +> **remote** `rpchost` over **plain HTTP** (no `rpctls=1`) will now be **refused** at connect +> time instead of warned. Affected users must add **`rpcallowplaintext=1`** to `DRAGONX.conf` +> (or switch to `rpctls=1`) to reconnect. Local/embedded daemons (`127.0.0.0/8`, `localhost`, +> `::1`) are unaffected. Call this out prominently in the release notes. +> +> **As-built note.** Shipped the security-complete core: `isLocalHost` tightened to exact +> loopback (`isExactIPv4Loopback` — `127.evil.com` no longer passes), refuse-by-default in +> `tryConnect`, and the `rpcallowplaintext` conf-key opt-in. The **Settings toggle UI was +> deferred** — the RPC section of `settings_page.cpp` is read-only display and a security +> toggle there is riskier surface; the conf-key opt-in fully covers recovery, and the refusal +> status/notification tells the user exactly what to add. The toggle can be added later +> (persist a `Settings` flag and OR it into `allowsPlaintextRemote`). ### The defect A remote `rpchost` without `rpctls=1` sends Basic-auth `rpcuser:rpcpassword` over @@ -512,6 +532,7 @@ design record; see the shared-helper table above. - **F1** — ☑ landed: `isRunning()` (POSIX) now reads the atomic `state_` (predicate `Running || Stopping`) instead of calling `waitpid`, leaving `monitorProcess()` the sole reaper. Clean build (all targets link); `ctest` 1/1 passing. Not unit-testable — needs the manual `kill -SEGV` repro before release. - **F2** — ☑ landed: `startProcess()` (POSIX) now creates a `FD_CLOEXEC` self-pipe before `fork()`; the child writes `errno` to it on `execv` failure, the parent reads EOF-vs-errno and, on failure, reaps the zombie + sets a precise `last_error_` ("not executable or wrong architecture") + returns `false` (so `start()` no longer reports `Running` for a daemon that never started). Parent-side `setpgid` is now best-effort with a `DEBUG_LOGF` on failure. Clean build; `ctest` 1/1 passing. Not unit-testable — needs the manual non-executable / wrong-arch-binary repro before release. +- **F8** — ☑ landed: `isLocalHost()` tightened to exact loopback via `isExactIPv4Loopback` (a `127.`-prefixed *hostname* like `127.evil.com` is no longer misclassified as local). `tryConnect()` now **refuses** a plaintext connection to a remote host instead of warn-and-proceeding — a local-network MITM can no longer capture `rpcuser:rpcpassword` — unless the user opts in with `rpcallowplaintext=1` in `DRAGONX.conf` (new `ConnectionConfig::allow_plaintext_remote` + `allowsPlaintextRemote()` policy). The refusal surfaces via status line + a one-time notification. New `testIsLocalHost` (12 assertions) + `testAllowsPlaintextRemote` (5). Clean build; `ctest` 1/1 passing. **Breaking — needs release notes; Settings-toggle UI deferred (see as-built note).** - **F3** — ☑ landed: the connect loop now stamps `connect_stall_since_ = ImGui::GetTime()` the moment the daemon first goes "reachable but not ready" (warmup branch + `applyDaemonInitStatus`), and clears it in `onConnected` / `onDisconnected` / warmup-complete — all in `app_network.cpp`. The pure `util::connectHasStalled(stallSince, now, threshold)` helper (new `util/connect_stall.h`, default 45 s from `ui.toml`) drives a draw-list "Taking longer than expected" notice in `renderLoadingOverlay()` (title + elapsed-seconds body + full-node hint), guarded off while the daemon is in `State::Error`. Background retry continues, so the notice self-clears on connect. New `testConnectHasStalled` unit test (7 assertions). Clean build; `ctest` 1/1 passing. (Draw-list text, not buttons — see as-built note above.) - **F6** — ☑ landed: `verifySaplingParams()` now hash-verifies each Sapling param against its pinned canonical SHA-256 (from `build-lite-backend-artifact.sh`), replacing the existence-only check, so a truncated/corrupt-but-present param is rejected instead of failing later on a shielded op. A `/.sapling_verified` marker keyed on `size:mtime` skips re-hashing ~48 MB on every startup. Logic extracted to the injectable `verifySaplingParamsIn(dir, digests)`; new `testVerifySaplingParams` unit test (valid / marker fast-path / wrong-hash / truncated / missing). Clean build; `ctest` 1/1 passing. - **F5** — ☑ landed: `startEmbeddedDaemon()` now checks `extractEmbeddedResources()`'s return (abort with `sb_daemon_extract_failed` on failure) and the previously-dropped `copy_file` `error_code` in the daemon-binary fallback loop (abort with `sb_daemon_files_failed` incl. the dir), so a disk-full / truncated `dragonxd` write is surfaced up front instead of failing opaquely at spawn. An absent source file stays non-fatal. Two i18n keys added to `i18n.cpp`. Clean build; `ctest` 1/1 passing. diff --git a/src/app_network.cpp b/src/app_network.cpp index b0ab35d..0c33be3 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -321,11 +321,21 @@ void App::tryConnect() VERBOSE_LOGF("[connect #%d] Connecting to %s:%s (user=%s)\n", connect_attempt, config.host.c_str(), config.port.c_str(), config.rpcuser.c_str()); - if (rpc::Connection::usesPlaintextRemote(config) && !remote_rpc_plaintext_warning_shown_) { - remote_rpc_plaintext_warning_shown_ = true; - ui::Notifications::instance().warning( - "Remote RPC is using plaintext HTTP. Add rpctls=1 to DRAGONX.conf if your daemon supports TLS.", - 10.0f); + if (rpc::Connection::usesPlaintextRemote(config) && + !rpc::Connection::allowsPlaintextRemote(config)) { + // Refuse to send Basic-auth credentials in cleartext to a remote host — a local-network + // MITM would otherwise capture rpcuser:rpcpassword. This is a deliberate behaviour change + // from the old warn-and-proceed: opt in explicitly with rpcallowplaintext=1 in + // DRAGONX.conf (or enable TLS with rpctls=1) if the plaintext link is intended. + connection_in_progress_ = false; + connection_status_ = TR("sb_plaintext_remote_blocked"); + if (!remote_rpc_plaintext_warning_shown_) { + remote_rpc_plaintext_warning_shown_ = true; + ui::Notifications::instance().warning(TR("sb_plaintext_remote_blocked"), 20.0f); + } + VERBOSE_LOGF("[connect #%d] refusing plaintext-remote RPC to %s:%s (set rpcallowplaintext=1 to override)\n", + connect_attempt, config.host.c_str(), config.port.c_str()); + return; } // Run the blocking rpc_->connect() on the worker thread so the UI diff --git a/src/rpc/connection.cpp b/src/rpc/connection.cpp index ecd30c2..a30ef2c 100644 --- a/src/rpc/connection.cpp +++ b/src/rpc/connection.cpp @@ -290,6 +290,8 @@ ConnectionConfig Connection::parseConfFile(const std::string& path) config.proxy = value; } else if (key == "rpctls" || key == "rpcssl" || key == "use_tls" || key == "rpcuse_tls") { config.use_tls = parseBoolValue(value); + } else if (key == "rpcallowplaintext") { + config.allow_plaintext_remote = parseBoolValue(value); } } @@ -366,6 +368,31 @@ bool Connection::buildCookieAuthConfig(const ConnectionConfig& base, ConnectionC return true; } +// True only for a well-formed IPv4 loopback literal (127.0.0.0/8): exactly four dot-separated +// 0-255 octets with the first == 127. Rejects "127.evil.com", "127.0.0.1.attacker", +// "127.300.0.1", "1270.0.0.1", etc. — the old rfind("127.",0)==0 prefix matched all of those. +static bool isExactIPv4Loopback(const std::string& host) +{ + int octets = 0, value = 0, digits = 0; + bool firstIs127 = false; + for (size_t i = 0; i <= host.size(); ++i) { + const char c = (i < host.size()) ? host[i] : '.'; // trailing sentinel flushes the last octet + if (c == '.') { + if (digits == 0 || digits > 3 || value > 255) return false; + if (octets == 0) firstIs127 = (value == 127); + ++octets; + value = 0; + digits = 0; + } else if (c >= '0' && c <= '9') { + value = value * 10 + (c - '0'); + ++digits; + } else { + return false; + } + } + return octets == 4 && firstIs127; +} + bool Connection::isLocalHost(const std::string& host) { std::string lowered = lowercase(host); @@ -375,7 +402,7 @@ bool Connection::isLocalHost(const std::string& host) return lowered == "localhost" || lowered == "localhost." || lowered == "::1" || lowered == "0:0:0:0:0:0:0:1" || - lowered == "127.0.0.1" || lowered.rfind("127.", 0) == 0; + isExactIPv4Loopback(lowered); } bool Connection::usesPlaintextRemote(const ConnectionConfig& config) @@ -383,6 +410,13 @@ bool Connection::usesPlaintextRemote(const ConnectionConfig& config) return !config.use_tls && !isLocalHost(config.host); } +bool Connection::allowsPlaintextRemote(const ConnectionConfig& config) +{ + // Explicit opt-in (DRAGONX.conf: rpcallowplaintext=1) to send credentials over a plaintext + // link to a remote host. Off by default — see usesPlaintextRemote(). + return config.allow_plaintext_remote; +} + const char* Connection::authSourceName(AuthSource source) { switch (source) { diff --git a/src/rpc/connection.h b/src/rpc/connection.h index 734c598..fc0d5f3 100644 --- a/src/rpc/connection.h +++ b/src/rpc/connection.h @@ -29,6 +29,7 @@ struct ConnectionConfig { std::string proxy; // SOCKS5 proxy for Tor bool use_embedded = true; bool use_tls = false; + bool allow_plaintext_remote = false; // rpcallowplaintext=1 — opt in to plaintext creds to a remote host AuthSource auth_source = AuthSource::Missing; // Non-empty when autoDetectConfig() could not create the data directory; callers // should surface it and abort the connect rather than proceeding blindly. @@ -132,6 +133,11 @@ public: */ static bool usesPlaintextRemote(const ConnectionConfig& config); + // Whether plaintext credentials to a remote host are explicitly allowed (opt-in via the + // DRAGONX.conf rpcallowplaintext key). Off by default: usesPlaintextRemote() && !this + // means the connect is refused. + static bool allowsPlaintextRemote(const ConnectionConfig& config); + static const char* authSourceName(AuthSource source); private: diff --git a/src/util/i18n.cpp b/src/util/i18n.cpp index ea02105..3edee0f 100644 --- a/src/util/i18n.cpp +++ b/src/util/i18n.cpp @@ -1321,6 +1321,7 @@ void I18n::loadBuiltinEnglish() strings_["loading_stall_title"] = "Taking longer than expected"; strings_["loading_stall_body"] = "The daemon has been initializing for %.0fs. This can be normal after an update or on first launch (loading the block index or rescanning) — it will connect automatically once ready."; strings_["loading_stall_hint"] = "Still stuck? Open Settings and use Restart Daemon, or check the Console for details."; + strings_["sb_plaintext_remote_blocked"] = "Refusing to send RPC credentials over plaintext to a remote host. Add rpcallowplaintext=1 to DRAGONX.conf to allow it, or enable TLS with rpctls=1."; strings_["sb_dragonxd_running"] = "dragonxd running"; strings_["sb_dragonxd_stopping"] = "Stopping dragonxd..."; strings_["sb_dragonxd_stopped"] = "dragonxd stopped"; diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index 2a0dbf5..b5906ea 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -2479,6 +2479,50 @@ void testDaemonShutdownPolicy() EXPECT_TRUE(bootstrap.disconnectRpc); } +void testIsLocalHost() +{ + using dragonx::rpc::Connection; + // Genuine loopback / local hosts. + EXPECT_TRUE(Connection::isLocalHost("127.0.0.1")); + EXPECT_TRUE(Connection::isLocalHost("127.1.2.3")); + EXPECT_TRUE(Connection::isLocalHost("localhost")); + EXPECT_TRUE(Connection::isLocalHost("LocalHost")); + EXPECT_TRUE(Connection::isLocalHost("::1")); + EXPECT_TRUE(Connection::isLocalHost("[::1]")); + // The regression this fix targets: a hostname merely starting "127." is NOT loopback. + EXPECT_TRUE(!Connection::isLocalHost("127.evil.com")); + EXPECT_TRUE(!Connection::isLocalHost("127.0.0.1.attacker.example")); + EXPECT_TRUE(!Connection::isLocalHost("127.300.0.1")); + EXPECT_TRUE(!Connection::isLocalHost("1270.0.0.1")); + EXPECT_TRUE(!Connection::isLocalHost("10.0.0.5")); + EXPECT_TRUE(!Connection::isLocalHost("example.com")); +} + +void testAllowsPlaintextRemote() +{ + using dragonx::rpc::Connection; + using dragonx::rpc::ConnectionConfig; + + ConnectionConfig local; + local.host = "127.0.0.1"; + local.use_tls = false; + EXPECT_TRUE(!Connection::usesPlaintextRemote(local)); // local is never "plaintext remote" + + ConnectionConfig remote; + remote.host = "10.0.0.5"; + remote.use_tls = false; + EXPECT_TRUE(Connection::usesPlaintextRemote(remote)); // remote + no TLS + EXPECT_TRUE(!Connection::allowsPlaintextRemote(remote)); // blocked by default → connect refused + + remote.allow_plaintext_remote = true; + EXPECT_TRUE(Connection::allowsPlaintextRemote(remote)); // explicit opt-in + + ConnectionConfig remoteTls; + remoteTls.host = "10.0.0.5"; + remoteTls.use_tls = true; + EXPECT_TRUE(!Connection::usesPlaintextRemote(remoteTls)); // TLS → not plaintext, never refused +} + void testConnectHasStalled() { using dragonx::util::connectHasStalled; @@ -6758,6 +6802,8 @@ int main() testPlatformEnsureDirectory(); testVerifySaplingParams(); testConnectHasStalled(); + testIsLocalHost(); + testAllowsPlaintextRemote(); testDaemonLifecycleExecution(); testDaemonLifecycleAdapters(); testConsoleTextLayout();