From aa90491792ca508795a71ffe9bccefdf4af0bf69 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 19 Jul 2026 14:32:05 +0200 Subject: [PATCH] fix(cli): stage Windows install transactions under a per-user root The activation transaction and the release-descriptor probe validate the full ancestor chain of everything they stage: every path component must have a trusted owner and grant mutation rights only to trusted identities. That walk is deliberate TOCTOU defense and stays strict. The staging directory, however, came from the process temp root, which routinely lives in a tree that legitimately fails the walk through no fault of ours: under msys2 TMP resolves inside the msys install tree (C:\msys64\tmp), and on GitHub runners inside the runner work directory, both of which carry broad inherited grants such as Authenticated Users modify on an upper component. The staged pair then failed validation ("launcher descriptor candidate is unsafe" / the smoke Phase 8 --version probe refusal) even though the staged files themselves were private. Add cli_windows_staging_root(): stage install and update transactions under %LOCALAPPDATA%\codebase-memory-mcp, whose ancestor chain is user-owned end to end, so the strict walk passes by construction instead of by environment luck. Falls back to USERPROFILE\AppData\Local and finally the process temp root when the environment is degraded. Also make the staging observable where it previously failed blind: cli_windows_stage_private_file reports which transaction step failed with the OS error code, and cbm_mkdtemp warns once if the private security descriptor cannot be applied and it falls back to default directory security. A blind boolean here cost several blind CI round-trips. win.sh: scp targets need native C:/ paths on Windows OpenSSH. Signed-off-by: Martin Vogel --- src/cli/cli.c | 71 ++++++++++++++++++++++++++++++----- src/foundation/compat.c | 18 ++++++++- test-infrastructure/vm/win.sh | 4 +- 3 files changed, 81 insertions(+), 12 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index 5723b529a..ba6f0c241 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -9477,11 +9477,31 @@ static bool cli_windows_stage_private_file( cbm_activation_transaction_status_t status = cbm_activation_transaction_stage_file(target, source, &transaction); cli_binary_validator_t validator = {{0}}; - bool ready = status == CBM_ACTIVATION_TRANSACTION_OK && transaction && - cli_activation_transaction_expected_build(transaction, &validator) && - cli_activation_transaction_commit_validated(transaction, &validator, - CLI_OCTAL_PERM) == CLI_OK && - cli_activation_transaction_finalize_close(&transaction) == CLI_OK; + /* Each transaction step reports separately: a directory-validation + * refusal, a build-identity mismatch, and a commit failure are three + * different bugs, and the caller's single boolean previously collapsed + * them into one blind "staging failed" with no OS error code. */ + bool ready = status == CBM_ACTIVATION_TRANSACTION_OK && transaction; + if (!ready) { + (void)fprintf(stderr, "error: staging transaction open failed (status %d, os %lu)\n", + (int)status, (unsigned long)GetLastError()); + } + if (ready && !cli_activation_transaction_expected_build(transaction, &validator)) { + (void)fprintf(stderr, "error: staged copy build-identity validation failed (os %lu)\n", + (unsigned long)GetLastError()); + ready = false; + } + if (ready && cli_activation_transaction_commit_validated(transaction, &validator, + CLI_OCTAL_PERM) != CLI_OK) { + (void)fprintf(stderr, "error: staging transaction commit failed (os %lu)\n", + (unsigned long)GetLastError()); + ready = false; + } + if (ready && cli_activation_transaction_finalize_close(&transaction) != CLI_OK) { + (void)fprintf(stderr, "error: staging transaction finalize failed (os %lu)\n", + (unsigned long)GetLastError()); + ready = false; + } (void)cli_activation_transaction_abort(&transaction); if (ready && fingerprint_out) { (void)snprintf(fingerprint_out, CBM_DAEMON_BUILD_FINGERPRINT_SIZE, "%s", @@ -9513,6 +9533,37 @@ static bool cli_windows_stage_private_bytes( return ready; } +/* Per-user staging root for Windows install/update transactions. The + * activation transaction and the release-descriptor probe validate the FULL + * ancestor chain of everything they stage (trusted owner and no foreign + * mutation grant on any path component). The process temp root routinely + * fails that walk through no fault of ours: under msys2, TMP resolves + * inside the msys install tree (C:\msys64\tmp), and on GitHub runners + * inside the runner work directory, both of which carry broad inherited + * grants such as Authenticated Users modify on an upper component. The + * profile's AppData\Local chain is user-owned end to end, so staging there + * satisfies the strict walk by construction instead of by environment + * luck. */ +static const char *cli_windows_staging_root(char root_out[CLI_BUF_1K]) { + char base[CLI_BUF_1K]; + if (!cbm_safe_getenv("LOCALAPPDATA", base, sizeof(base), NULL) || !base[0]) { + char profile[CLI_BUF_1K]; + if (!cbm_safe_getenv("USERPROFILE", profile, sizeof(profile), NULL) || !profile[0]) { + return cbm_tmpdir(); + } + int base_length = snprintf(base, sizeof(base), "%s/AppData/Local", profile); + if (base_length <= 0 || (size_t)base_length >= sizeof(base)) { + return cbm_tmpdir(); + } + } + int written = snprintf(root_out, CLI_BUF_1K, "%s/codebase-memory-mcp", base); + if (written <= 0 || written >= CLI_BUF_1K) { + return cbm_tmpdir(); + } + (void)_mkdir(root_out); + return root_out; +} + static bool cli_windows_prepare_install_pair(const wchar_t *launcher_source, const wchar_t *payload_source, const char expected_payload_sha256[65], @@ -9521,8 +9572,9 @@ static bool cli_windows_prepare_install_pair(const wchar_t *launcher_source, char directory_out[CLI_BUF_1K]) { char *launcher_source_utf8 = cbm_wide_to_utf8(launcher_source); char *payload_source_utf8 = cbm_wide_to_utf8(payload_source); - int directory_length = - snprintf(directory_out, CLI_BUF_1K, "%s/cbm-win-install-XXXXXX", cbm_tmpdir()); + char staging_root[CLI_BUF_1K]; + int directory_length = snprintf(directory_out, CLI_BUF_1K, "%s/cbm-win-install-XXXXXX", + cli_windows_staging_root(staging_root)); bool ready = launcher_source_utf8 && payload_source_utf8 && directory_length > 0 && directory_length < CLI_BUF_1K && cbm_mkdtemp(directory_out); char launcher_target[CLI_BUF_1K]; @@ -9622,8 +9674,9 @@ static bool cli_windows_prepare_update_pair(const cbm_windows_release_pair_t *pa pair->payload_len <= 0 || !expected_payload_sha256) { return false; } - int directory_length = - snprintf(directory_out, CLI_BUF_1K, "%s/cbm-win-update-XXXXXX", cbm_tmpdir()); + char staging_root[CLI_BUF_1K]; + int directory_length = snprintf(directory_out, CLI_BUF_1K, "%s/cbm-win-update-XXXXXX", + cli_windows_staging_root(staging_root)); bool ready = directory_length > 0 && directory_length < CLI_BUF_1K && cbm_mkdtemp(directory_out); char launcher_target[CLI_BUF_1K]; diff --git a/src/foundation/compat.c b/src/foundation/compat.c index 32f40ef24..27862fe2d 100644 --- a/src/foundation/compat.c +++ b/src/foundation/compat.c @@ -129,8 +129,22 @@ char *cbm_mkdtemp(char *tmpl) { } if (!_mktemp(buf)) return NULL; - if (!win_mkdtemp_private_create(buf) && _mkdir(buf) != 0) - return NULL; + if (!win_mkdtemp_private_create(buf)) { + /* One-time note: every private-namespace validation downstream + * depends on the explicit descriptor, so a silent fallback turns + * into unexplained owner/DACL refusals far from this call site. */ + static bool fallback_reported; + DWORD create_error = GetLastError(); + if (_mkdir(buf) != 0) + return NULL; + if (!fallback_reported) { + fallback_reported = true; + (void)fprintf(stderr, + "warning: private temp-directory descriptor unavailable " + "(os %lu); using default directory security\n", + (unsigned long)create_error); + } + } /* Normalize to forward slashes. Callers embed this path in JSON repo_path * (where "\t"/"\a" are invalid escapes → index fails) and pass it to git -C. * Windows file APIs accept forward slashes, so the created dir is unaffected. */ diff --git a/test-infrastructure/vm/win.sh b/test-infrastructure/vm/win.sh index 2e523ec3c..b9dd027a4 100644 --- a/test-infrastructure/vm/win.sh +++ b/test-infrastructure/vm/win.sh @@ -61,7 +61,9 @@ sh) ;; push-file) [ $# -eq 2 ] || { echo "usage: win.sh push-file " >&2; exit 2; } - scp -i "$KEY" -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null "$1" "${USER_}@${HOST}:$2" + # Windows OpenSSH resolves scp targets natively: use C:/... not /c/... + dest="${2/#\/c\//C:\/}" + scp -i "$KEY" -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null "$1" "${USER_}@${HOST}:${dest}" ;; asan-build) # x86_64 (CI's exact arch) with ASan, runs under Windows-on-ARM emulation.