Close gateway-secret desync, TOCTOU, and undisclosed custom-image gaps
Secret Scan / scan (push) Successful in 6s
Build App (Preview) / compute-version (pull_request) Successful in 5s
Secret Scan / scan (pull_request) Successful in 5s
Build App (Preview) / create-release (pull_request) Successful in 2s
Build App (Preview) / build-macos (pull_request) Successful in 2m41s
Build App (Preview) / build-windows (pull_request) Successful in 4m53s
Build App (Preview) / build-linux (pull_request) Successful in 7m5s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
Secret Scan / scan (push) Successful in 6s
Build App (Preview) / compute-version (pull_request) Successful in 5s
Secret Scan / scan (pull_request) Successful in 5s
Build App (Preview) / create-release (pull_request) Successful in 2s
Build App (Preview) / build-macos (pull_request) Successful in 2m41s
Build App (Preview) / build-windows (pull_request) Successful in 4m53s
Build App (Preview) / build-linux (pull_request) Successful in 7m5s
Build App (Preview) / prune-previews (pull_request) Successful in 1s
Round 4 review findings: - Disclose and warn on a custom Docker image the import would set (HIGH): it's the image every project container is created from, so an undisclosed change here was a sharper version of the redirected-base-URL problem round 3 already flagged for the model backends. - Recreate a running gateway container when an import restores a new secret with the shape unchanged (MEDIUM): reconcile_gateway's shape comparison can't see a secret-only change, so the container would otherwise keep serving old key material indefinitely. - Report keychain write failures back to the caller instead of only logging them (MEDIUM): apply_settings_import now returns SettingsImportOutcome with secret_restore_warnings so a partial restore can't read as unqualified success. - Pin a hash of the previewed file's ciphertext and refuse to apply if it changed on disk (MEDIUM): closes a TOCTOU between preview and apply. - Sanitize and cap every free-form string a preview surfaces, and move the warning boxes above the replace list in the UI (MEDIUM): an unbounded base URL or image name could otherwise push the security warnings below the scroll fold. - Validate the Docker socket path on import the same as the SSH key and CA cert paths (LOW): it was the one mounted host path validate_settings_update didn't cover. - Fix ExportedSecrets::is_empty() to treat whitespace-only as blank, like every other secret-presence check in this feature (LOW). - Authenticate the file header as AEAD associated data (LOW, defense in depth) and correct two doc comments that overstated the password not being cached.
This commit is contained in:
@@ -588,11 +588,18 @@ deliberately out of scope — this is not a project backup.
|
||||
handing Rust a host path string is the exact shape of bug that produced this app's past
|
||||
criticals. `preview_settings_import` resolves the chosen path itself and remembers it
|
||||
(`AppState::pending_settings_import`) so `apply_settings_import` re-reads the same file without
|
||||
a path ever crossing back over IPC.
|
||||
- **The password is re-entered, not cached, between preview and apply.** Nothing here holds
|
||||
decrypted plaintext — secrets included — in memory for longer than one command's execution.
|
||||
`preview_settings_import` returns counts and presence flags only (`SettingsImportPreview`),
|
||||
never a secret value, so it's safe to hand to the frontend and render directly.
|
||||
a path ever crossing back over IPC. It also pins a hash of the file's ciphertext next to that
|
||||
path, and `apply_settings_import` refuses to proceed if the file on disk no longer matches it —
|
||||
otherwise confirming a preview would not actually be binding on what gets applied, which matters
|
||||
given this feature's own threat model: a file shared between people may sit in a synced or
|
||||
otherwise shared directory that changes between the two calls.
|
||||
- **The decrypted payload is not cached between preview and apply — only the password is reused.**
|
||||
The frontend holds the password in React state and passes it to both calls; nothing in Rust
|
||||
holds decrypted plaintext — secrets included — in memory for longer than one command's
|
||||
execution, so `apply_settings_import` always re-decrypts rather than reusing anything
|
||||
`preview_settings_import` computed. `preview_settings_import` returns counts and presence flags
|
||||
only (`SettingsImportPreview`), never a secret value, so it's safe to hand to the frontend and
|
||||
render directly.
|
||||
- **Import replaces settings wholesale, but only writes secrets actually present in the file.**
|
||||
An import is "restore this environment," so the settings half is a full replace, not a
|
||||
field-by-field merge. Secrets are different on purpose: an absent secret in the export means
|
||||
@@ -601,6 +608,23 @@ deliberately out of scope — this is not a project backup.
|
||||
gateway key). Secrets are restored *before* the settings replace runs, not after — replacing
|
||||
settings is what triggers `reconcile_gateway`, and restoring the other way round leaves a real
|
||||
window where a gateway recreation happens against the destination's old keys.
|
||||
- **A restored gateway secret nudges a running gateway container to recreate itself, even when
|
||||
nothing about the gateway's *shape* changed.** `reconcile_gateway`'s `gateway_shape_changed` only
|
||||
compares port/provider/base URL/models — deliberately, since that's what's rendered into the
|
||||
container's config — so a secret-only change (same shape, new key) is invisible to it. Left
|
||||
alone, a running container would keep serving the old key material indefinitely after an import
|
||||
that restored a new one. `apply_settings_import` tracks whether either gateway secret was
|
||||
actually written and, if the gateway is enabled and its container both exists and is running,
|
||||
calls `docker::gateway::ensure_gateway_running` directly afterward — its own fingerprint already
|
||||
includes the secret rotation id (`storage::secure::get_gateway_secret_version`), so it recreates
|
||||
exactly when it should and no more.
|
||||
- **A keychain write failing during import is reported back, not only logged.** Each of the three
|
||||
`secure::store_*` calls collects its error into `SettingsImportOutcome::secret_restore_warnings`
|
||||
in addition to logging it — an import that silently restores two of three secrets but not the
|
||||
third must not read as unqualified success just because the settings half of the import (which
|
||||
runs after, and is validated before any of this) went through. `apply_settings_import` returns
|
||||
`SettingsImportOutcome { settings, secret_restore_warnings }` rather than bare `AppSettings` for
|
||||
this reason; `ImportSettingsModal` shows any warnings alongside the "Settings imported" message.
|
||||
- **The imported settings are validated *before* any secret is written, not just before the
|
||||
settings replace.** `apply_settings_import` calls
|
||||
`settings_commands::validate_settings_update(¤t, &settings)` — the same checks
|
||||
@@ -634,6 +658,24 @@ deliberately out of scope — this is not a project backup.
|
||||
terminal token that arrives with the terminal left *off*: `start_web_terminal` only mints a fresh
|
||||
token when none is already set, so a planted token would otherwise activate silently the next
|
||||
time someone turns the terminal on, with no import-time signal that it wasn't freshly generated.
|
||||
- **The preview also discloses a custom Docker image, and warns on one every time — not just on
|
||||
change.** `custom_image_name`/`image_source` weren't in scope for the base-URL disclosure above,
|
||||
but a review pointed out they're a sharper version of the same problem: this is the image *every*
|
||||
project container is created from (`models::container_config::resolve_image_name`), so a crafted
|
||||
export pointing it at an attacker-controlled image is a path to running arbitrary code with
|
||||
whatever a project's containers are allowed to reach, not merely a redirected API endpoint.
|
||||
`describeImportWarnings` fires on `image_source == Custom` unconditionally rather than only when
|
||||
it differs from the destination's current value, since re-importing the same risky configuration
|
||||
is still worth surfacing every time a user confirms an import.
|
||||
- **Every free-form string a preview surfaces is sanitized and length-capped before it's built.**
|
||||
`SettingsImportPreview::from_payload`'s `sanitize_for_preview` strips control characters and caps
|
||||
at 100 characters (`MAX_PREVIEW_STRING_LEN`) for every base URL and the custom image name — a
|
||||
review noted that, unlike the count- and boolean-derived fields the preview started with, these
|
||||
are verbatim strings from a not-yet-trusted decrypted payload rendered directly into the
|
||||
confirmation dialog. Unbounded, a single pathological value (very long, or holding embedded
|
||||
newlines) could push the security warnings above the scroll fold in the dialog that exists
|
||||
specifically to make them unmissable — the frontend's `<li>`/warning boxes also get `break-all`
|
||||
as a second layer against the same failure mode.
|
||||
|
||||
## Testing
|
||||
|
||||
|
||||
Reference in New Issue
Block a user