From 440a56afad7239b92af28d91bd1c5f9dde5354c7 Mon Sep 17 00:00:00 2001 From: Jesse_Chen Date: Mon, 20 Jul 2026 23:42:15 +0800 Subject: [PATCH] fix: secure staging backup directory creation --- .superpowers/sdd/task-3-report.md | 20 ++++++++ deploy/backup-staging-postgres.sh | 70 ++++++++++++++++++++------ frontend/tests/database-backup.test.ts | 47 +++++++++++++++-- 3 files changed, 120 insertions(+), 17 deletions(-) diff --git a/.superpowers/sdd/task-3-report.md b/.superpowers/sdd/task-3-report.md index 4cdced1a..b271311f 100644 --- a/.superpowers/sdd/task-3-report.md +++ b/.superpowers/sdd/task-3-report.md @@ -36,6 +36,26 @@ cd frontend && npm run lint exit 0 git diff --check exit 0 ``` +## Secure directory-creation follow-up (2026-07-20) + +- `umask 077` now executes at script startup, before any possible `mkdir`. The script records the first absent component, then re-walks every component after `mkdir -p`; all newly created directories must be current-user owned, non-symlink directories without group/world write permission before `cd -P` pins the target. +- Existing root-owned sticky shared ancestors, including canonical `/tmp`, are allowed; ordinary group/world-writable ancestors remain rejected. The shared-temporary-root tests canonicalize `/tmp` with `realpathSync`, so they prove validation reaches their deliberately nested symlink and `0777` parent instead of stopping at the standard sticky ancestor. + +```text +RED: +cd frontend && npm run test:db +FAIL rejects destructive backup directory aliases and symlink components before mutation +FAIL creates every absent backup path component privately despite a permissive caller umask +The prior policy rejected canonical /tmp and only set umask after mkdir. + +GREEN: +bash -n deploy/backup-staging-postgres.sh exit 0 +cd frontend && npm run test:db 18 passed, 0 failed +cd frontend && npm test 494 passed, 0 failed +cd frontend && npm run lint exit 0 +git diff --check exit 0 +``` + ## TOCTOU follow-up (2026-07-20) - Before `mkdir`/`cd`, the script walks all existing absolute-target ancestors, rejects symlinks, requires current-user-or-root ownership, and rejects group/world-writable modes. The unsafe-parent regression uses `realpathSync` for the macOS temporary root, so its nested symlink and `0777` parent are the components actually reached by validation. diff --git a/deploy/backup-staging-postgres.sh b/deploy/backup-staging-postgres.sh index 3473b6ea..438cdd51 100755 --- a/deploy/backup-staging-postgres.sh +++ b/deploy/backup-staging-postgres.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash set -euo pipefail set +x +umask 077 if [ "$#" -ne 2 ]; then echo "usage: backup-staging-postgres.sh DATABASE_ENV_FILE BACKUP_DIRECTORY" >&2 @@ -28,8 +29,8 @@ reject_unsafe_backup_directory_ancestor() { stat_owner_and_mode() { local path="$1" - if stat -f '%u %Lp' "$path" >/dev/null 2>&1; then - stat -f '%u %Lp' "$path" + if stat -f '%u %p' "$path" >/dev/null 2>&1; then + stat -f '%u %p' "$path" else stat -c '%u %a' "$path" fi @@ -45,6 +46,43 @@ directory_identity() { fi } +directory_mode_is_group_or_world_writable() { + local mode="$1" + local permissions="${mode: -3}" + + (( (10#${permissions:1:1} & 2) != 0 || (10#${permissions:2:1} & 2) != 0 )) +} + +directory_mode_is_sticky() { + local mode="$1" + + [ "${#mode}" -ge 4 ] && (( (10#${mode: -4:1} & 1) != 0 )) +} + +validate_backup_directory_component() { + local path="$1" + local require_private="$2" + local owner + local mode + + if [ -L "$path" ] || [ ! -d "$path" ]; then + reject_backup_directory + fi + read -r owner mode <<< "$(stat_owner_and_mode "$path")" + if [ "$require_private" = "1" ]; then + if [ "$owner" != "$CURRENT_UID" ] || directory_mode_is_group_or_world_writable "$mode"; then + reject_unsafe_backup_directory_ancestor + fi + return + fi + if [ "$owner" != "$CURRENT_UID" ] && [ "$owner" != "0" ]; then + reject_unsafe_backup_directory_ancestor + fi + if directory_mode_is_group_or_world_writable "$mode" && ! { [ "$owner" = "0" ] && directory_mode_is_sticky "$mode"; }; then + reject_unsafe_backup_directory_ancestor + fi +} + if [ "$BACKUP_DIRECTORY_INPUT" = "/" ] || [[ "$BACKUP_DIRECTORY_INPUT" != /* ]] || [[ "$BACKUP_DIRECTORY_INPUT" == */ ]] || [[ "$BACKUP_DIRECTORY_INPUT" == *"//"* ]]; then reject_backup_directory fi @@ -56,7 +94,9 @@ fi backup_directory_component_path="" CURRENT_UID="$(id -u)" -for backup_directory_component in "${backup_directory_components[@]}"; do +FIRST_CREATED_COMPONENT_INDEX=-1 +for ((backup_directory_component_index = 0; backup_directory_component_index < ${#backup_directory_components[@]}; backup_directory_component_index += 1)); do + backup_directory_component="${backup_directory_components[$backup_directory_component_index]}" if [ -z "$backup_directory_component" ] || [ "$backup_directory_component" = "." ] || [ "$backup_directory_component" = ".." ]; then reject_backup_directory fi @@ -65,16 +105,9 @@ for backup_directory_component in "${backup_directory_components[@]}"; do reject_backup_directory fi if [ -e "$backup_directory_component_path" ]; then - if [ ! -d "$backup_directory_component_path" ]; then - reject_backup_directory - fi - read -r backup_directory_owner backup_directory_mode <<< "$(stat_owner_and_mode "$backup_directory_component_path")" - if [ "$backup_directory_owner" != "$CURRENT_UID" ] && [ "$backup_directory_owner" != "0" ]; then - reject_unsafe_backup_directory_ancestor - fi - if (( (10#${backup_directory_mode: -2:1} & 2) != 0 || (10#${backup_directory_mode: -1} & 2) != 0 )); then - reject_unsafe_backup_directory_ancestor - fi + validate_backup_directory_component "$backup_directory_component_path" 0 + elif [ "$FIRST_CREATED_COMPONENT_INDEX" -eq -1 ]; then + FIRST_CREATED_COMPONENT_INDEX="$backup_directory_component_index" fi done @@ -98,6 +131,16 @@ STAGING_BACKUP_ENCRYPTION_KEY="$(read_environment_value STAGING_BACKUP_ENCRYPTIO export STAGING_BACKUP_ENCRYPTION_KEY mkdir -p "$BACKUP_DIRECTORY_INPUT" +backup_directory_component_path="" +for ((backup_directory_component_index = 0; backup_directory_component_index < ${#backup_directory_components[@]}; backup_directory_component_index += 1)); do + backup_directory_component="${backup_directory_components[$backup_directory_component_index]}" + backup_directory_component_path="${backup_directory_component_path}/${backup_directory_component}" + if [ "$FIRST_CREATED_COMPONENT_INDEX" -ne -1 ] && [ "$backup_directory_component_index" -ge "$FIRST_CREATED_COMPONENT_INDEX" ]; then + validate_backup_directory_component "$backup_directory_component_path" 1 + else + validate_backup_directory_component "$backup_directory_component_path" 0 + fi +done cd -P "$BACKUP_DIRECTORY_INPUT" BACKUP_DIRECTORY="$(pwd -P)" if [ "$BACKUP_DIRECTORY" != "$BACKUP_DIRECTORY_INPUT" ] || [ "$BACKUP_DIRECTORY" = "/" ]; then @@ -144,7 +187,6 @@ cleanup_partial() { } trap cleanup_partial EXIT HUP INT TERM -umask 077 if ! mkdir "$LOCK_DIRECTORY"; then echo "backup destination is already being created" >&2 exit 1 diff --git a/frontend/tests/database-backup.test.ts b/frontend/tests/database-backup.test.ts index 19afa628..2467b377 100644 --- a/frontend/tests/database-backup.test.ts +++ b/frontend/tests/database-backup.test.ts @@ -54,6 +54,10 @@ function canonicalTemporaryDirectory(prefix: string): string { return realpathSync(mkdtempSync(join(tmpdir(), prefix))); } +function canonicalSharedTemporaryDirectory(prefix: string): string { + return realpathSync(mkdtempSync(join("/tmp", prefix))); +} + function listDumpArchive(fixture: ReturnType, dump: Buffer): void { const hostRestore = spawnSync("pg_restore", ["--list"], { input: dump, @@ -119,8 +123,12 @@ function runBackup( backupDirectory: string, environment: NodeJS.ProcessEnv, cwd = repositoryRoot, + callerUmask?: string, ) { - return spawnSync("bash", [backupScript, databaseEnvFile, backupDirectory], { + const args = callerUmask + ? ["-c", 'umask "$1"; shift; exec bash "$@"', "backup-umask", callerUmask, backupScript, databaseEnvFile, backupDirectory] + : [backupScript, databaseEnvFile, backupDirectory]; + return spawnSync("bash", args, { cwd, encoding: "utf8", env: environment, @@ -228,7 +236,7 @@ test("staging backups are encrypted, atomic, private, and retain the newest thre }); test("rejects destructive backup directory aliases and symlink components before mutation", () => { - const root = canonicalTemporaryDirectory("jyotisha-backup-boundary-"); + const root = canonicalSharedTemporaryDirectory("jyotisha-backup-boundary-"); const environmentFile = createDatabaseEnvironment(); const target = join(root, "target"); const sentinel = join(target, "sentinel.txt"); @@ -273,7 +281,7 @@ test("rejects destructive backup directory aliases and symlink components before }); test("rejects unsafe writable backup parents before creating the target", () => { - const root = canonicalTemporaryDirectory("jyotisha-backup-unsafe-parent-"); + const root = canonicalSharedTemporaryDirectory("jyotisha-backup-unsafe-parent-"); const environmentFile = createDatabaseEnvironment(); const unsafeParent = join(root, "unsafe-parent"); const target = join(unsafeParent, "backup"); @@ -297,6 +305,39 @@ test("rejects unsafe writable backup parents before creating the target", () => } }); +test("creates every absent backup path component privately despite a permissive caller umask", () => { + const root = canonicalSharedTemporaryDirectory("jyotisha-backup-umask-"); + const environmentFile = createDatabaseEnvironment(); + const commandDirectory = mkdtempSync(join(tmpdir(), "jyotisha-backup-umask-command-")); + const components = ["first", "second", "backup"]; + const target = join(root, ...components); + + try { + safeDiskCommand(commandDirectory); + writeCommand(commandDirectory, "docker", "printf '%s' 'umask dump payload'"); + const result = runBackup( + environmentFile.file, + target, + backupEnvironment(commandDirectory, { BACKUP_TIMESTAMP: "20260720T050101Z" }), + repositoryRoot, + "000", + ); + + assert.equal(result.status, 0, result.stderr); + let createdPath = root; + for (const component of components) { + createdPath = join(createdPath, component); + const created = statSync(createdPath); + assert.equal(created.uid, process.getuid?.()); + assert.equal(created.mode & 0o777, 0o700); + } + } finally { + rmSync(root, { force: true, recursive: true }); + rmSync(environmentFile.directory, { force: true, recursive: true }); + rmSync(commandDirectory, { force: true, recursive: true }); + } +}); + test("same-second backups publish once without overwriting the completed archive", async () => { const environmentFile = createDatabaseEnvironment(); const backupDirectory = canonicalTemporaryDirectory("jyotisha-backup-collision-");