From ecd95c62568092f482e93c0ad731f4d94a097834 Mon Sep 17 00:00:00 2001 From: Bryan Date: Fri, 28 Aug 2026 20:12:09 +0000 Subject: [PATCH] fix: harden os-bind-rp installer transactions --- .../ci/ci-tests/test_install_os_bind_rp.py | 211 ++++++++++++++++-- README.md | 2 +- docs/package-repository.md | 41 +++- scripts/install-os-bind-rp.sh | 94 +++++--- 4 files changed, 288 insertions(+), 60 deletions(-) diff --git a/.github/ci/ci-tests/test_install_os_bind_rp.py b/.github/ci/ci-tests/test_install_os_bind_rp.py index 765200afe4..221f716b54 100644 --- a/.github/ci/ci-tests/test_install_os_bind_rp.py +++ b/.github/ci/ci-tests/test_install_os_bind_rp.py @@ -40,7 +40,6 @@ def installer_environment( os_bind_rp_candidate_version: str | None = None, confirmation: str | None = None, key_sha256: str = PUBLIC_KEY_SHA256, - fetch_failure: bool = False, archive_checksum: str = "2$" + "a" * 64, install_failure: bool = False, plugin_install_failure: bool = False, @@ -53,6 +52,7 @@ def installer_environment( dry_run_plan: str = "valid", break_dry_run_output: bool = False, query_fault: str = "", + temporary_directory: Path | str | None = None, ) -> tuple[dict[str, str], Path, Path]: log = tmp_path / "commands.log" tty = tmp_path / "tty" @@ -67,12 +67,16 @@ def installer_environment( if pkg_locked: lock_marker.touch() - environment = os.environ.copy() + environment = {name: value for name, value in os.environ.items() if not name.startswith("RP_")} environment.update( { "RP_PKG_REPOSITORY_DIR": str(tmp_path / "repos"), "RP_PKG_KEYS_DIR": str(tmp_path / "keys"), - "RP_TEMPORARY_DIRECTORY": str(tmp_path / "temporary"), + "RP_TEMPORARY_DIRECTORY": str( + temporary_directory + if temporary_directory is not None + else tmp_path / "temporary" + ), "RP_TTY_PATH": str(tty), "RP_TEST_BIND920": bind920, "RP_TEST_BIND_TOOLS": bind_tools, @@ -82,7 +86,6 @@ def installer_environment( os_bind_rp_candidate_version or plugin_candidate_for_opnsense_version(opnsense_version) ), - "RP_TEST_FETCH_FAILURE": "yes" if fetch_failure else "no", "RP_TEST_INSTALL_FAILURE": "yes" if install_failure else "no", "RP_TEST_PLUGIN_INSTALL_FAILURE": "yes" if plugin_install_failure else "no", "RP_TEST_FETCH_LAYOUT": fetch_layout, @@ -119,7 +122,7 @@ def write_command_fixtures(directory: Path) -> None: directory / "fetch", "#!/bin/sh\n" "{ printf 'fetch'; for argument in \"$@\"; do printf ' %s' \"$argument\"; done; printf '\\n'; } >> \"$RP_TEST_LOG\"\n" - "[ \"${RP_TEST_FETCH_FAILURE:-no}\" = yes ] && exit 1\n" + "[ \"${RP_TEMPORARY_DIRECTORY:-}\" = / ] && exit 97\n" "[ \"$1\" = -o ] || exit 64\n" "printf 'test public key\\n' > \"$2\"\n", ) @@ -134,7 +137,6 @@ def write_command_fixtures(directory: Path) -> None: write_executable( directory / "pkg", r'''#!/usr/bin/env python3 -import hashlib import os from pathlib import Path import re @@ -201,8 +203,6 @@ def installed(name): if f"%n = {name}" in expression: print(f"{name}|{version}|{origin}") elif command == "fetch": - if os.environ.get("RP_TEST_FETCH_FAILURE") == "yes": - raise SystemExit(1) destination = Path(args[args.index("-o") + 1]) if os.environ.get("RP_TEST_FETCH_LAYOUT", "all") == "all": destination /= "All" @@ -287,8 +287,11 @@ def installed(name): ) elif command == "install": if "-n" in args: - plan = os.environ.get("RP_TEST_DRY_RUN_PLAN", "valid") - if plan == "all_current": + repository = args[args.index("-r") + 1] + verified_plan = repository == "resolver-verified" + plan = os.environ.get("RP_TEST_DRY_RUN_PLAN", "valid") if verified_plan else "valid" + requested = {argument for argument in args if re.search(r"-[0-9]", argument)} + if plan in {"all_current", "missing"}: print("The most recent versions of packages are already installed") elif plan == "repository_warning_noop": print( @@ -298,16 +301,10 @@ def installed(name): print("The most recent versions of packages are already installed") elif plan == "outside_identity": print(f"notice: requested archive {args[-1]} was not selected") - print("The following package(s) will be affected:") + print("The most recent versions of packages are already installed") else: print("The following package(s) will be affected:") - if plan == "outside_identity": - print("New packages to be INSTALLED:") - print("\tbind920-9.20.26_2") - elif plan == "missing": - print("New packages to be INSTALLED:") - print("\tbind920-9.20.26_2") - elif plan == "wrong_result_version": + if plan == "wrong_result_version": print("Installed packages to be UPGRADED:") print("\tos-bind-rp: 1.36_9 -> 1.36_100") elif plan == "requested_only_on_old_side": @@ -316,7 +313,12 @@ def installed(name): elif plan == "requested_removal": print("Installed packages to be REMOVED:") print(f"\tos-bind-rp-{os.environ['RP_TEST_OS_BIND_RP_CANDIDATE_VERSION']}") - elif plan not in {"missing", "all_current", "repository_warning_noop"}: + elif plan not in { + "missing", + "all_current", + "outside_identity", + "repository_warning_noop", + }: if plan == "unknown_section": print("Packages selected for CHANGE:") else: @@ -324,6 +326,13 @@ def installed(name): for argument in args: if re.search(r"-[0-9]", argument): print(f"\t{argument}") + if ( + verified_plan + and any(identity.startswith("os-bind-rp-") for identity in requested) + and os.environ.get("RP_TEST_OS_BIND") + ): + print("Installed packages to be REMOVED:") + print("\tos-bind-1.34_3") if plan == "pkg_colon": print("Installed packages to be UPGRADED:") print("\tpkg: 2.3.1_1 -> 2.4.0") @@ -345,6 +354,35 @@ def installed(name): elif plan == "unrelated_removal": print("Installed packages to be REMOVED:") print("\tpython311-3.11.13") + elif verified_plan and plan == "unrequested_bind_install": + print("New packages to be INSTALLED:") + print("\tbind920-9.20.26_2") + elif verified_plan and plan == "unrequested_bind_upgrade": + print("Installed packages to be UPGRADED:") + print("\tbind920: 9.20.25 -> 9.20.26_2") + elif verified_plan and plan == "unrequested_bind_downgrade": + print("Installed packages to be DOWNGRADED:") + print("\tbind920: 9.20.27 -> 9.20.26_2") + elif verified_plan and plan == "unrequested_bind_reinstall": + print("Installed packages to be REINSTALLED:") + print("\tbind920-9.20.26_2") + elif verified_plan and plan == "unrequested_bind_removal": + print("Installed packages to be REMOVED:") + print("\tbind920-9.20.26_2") + elif verified_plan and plan == "unrequested_os_bind_install": + print("New packages to be INSTALLED:") + print("\tos-bind-1.34_3") + elif verified_plan and plan == "unobserved_os_bind_removal": + print("Installed packages to be REMOVED:") + print("\tos-bind-1.34_3") + elif ( + verified_plan + and plan == "plugin_unrequested_bind_after_fallback" + and any(identity.startswith("os-bind-rp-") for identity in requested) + and marker("RP_TEST_FALLBACK_MARKER").exists() + ): + print("Installed packages to be REINSTALLED:") + print("\tbind920-9.20.26_2") elif plan == "malformed_entry": print("\t???") elif plan == "blank_continuation": @@ -399,6 +437,15 @@ def installed(name): directory / "dry-run-awk", "#!/bin/sh\nprintf '%s\\n' 'simulated dry-run parser failure' >&2\nexit 2\n", ) + for command in ("configctl", "service"): + write_executable( + directory / command, + "#!/bin/sh\n" + f"printf '{command}' >> \"$RP_TEST_LOG\"\n" + "for argument in \"$@\"; do printf ' %s' \"$argument\" >> \"$RP_TEST_LOG\"; done\n" + "printf '\\n' >> \"$RP_TEST_LOG\"\n" + "exit 97\n", + ) def run_installer(tmp_path: Path, **kwargs: object) -> tuple[subprocess.CompletedProcess[str], Path, Path]: @@ -505,6 +552,44 @@ def test_preserves_a_trusted_key_when_the_replacement_fails_verification(tmp_pat assert "pkg install" not in calls +def test_stages_the_repository_key_in_a_dedicated_private_child(tmp_path: Path) -> None: + result, log, _ = run_installer(tmp_path) + + assert result.returncode == 0, result.stderr + calls = log.read_text(encoding="utf-8") + assert f"fetch -o {tmp_path}/temporary/key-stage/resolver-plugins.pub " in calls + + +@pytest.mark.parametrize("scratch_kind", ["directory", "symlink"]) +def test_rejects_an_existing_scratch_path_without_changing_its_target( + tmp_path: Path, scratch_kind: str +) -> None: + target = tmp_path / "existing-target" + target.mkdir(mode=0o755) + sentinel = target / "sentinel" + sentinel.write_text("preserve me\n", encoding="utf-8") + scratch = target + if scratch_kind == "symlink": + scratch = tmp_path / "temporary-link" + scratch.symlink_to(target, target_is_directory=True) + + result, _, _ = run_installer(tmp_path, temporary_directory=scratch) + + assert result.returncode != 0 + assert "could not exclusively create temporary directory" in result.stderr + assert stat.S_IMODE(target.stat().st_mode) == 0o755 + assert sentinel.read_text(encoding="utf-8") == "preserve me\n" + assert [path.name for path in target.iterdir()] == ["sentinel"] + + +def test_rejects_the_root_directory_as_scratch_before_fetching(tmp_path: Path) -> None: + result, log, _ = run_installer(tmp_path, temporary_directory="/") + + assert result.returncode != 0 + assert "refusing unsafe temporary directory: /" in result.stderr + assert not log.exists() or "fetch " not in log.read_text(encoding="utf-8") + + def test_prompts_for_and_installs_the_fallback_when_bind_is_ineligible(tmp_path: Path) -> None: result, log, repositories = run_installer( tmp_path, @@ -530,6 +615,63 @@ def test_prompts_for_and_installs_the_fallback_when_bind_is_ineligible(tmp_path: assert "os-bind-rp-26.1_1" in live_installs[1] +def test_dry_runs_each_fallback_transaction_immediately_before_applying_it( + tmp_path: Path, +) -> None: + result, log, _ = run_installer( + tmp_path, + bind920="bind920|9.20.25|dns/bind920", + bind_tools="bind-tools|9.20.25|dns/bind-tools", + confirmation="y", + ) + + assert result.returncode == 0, result.stderr + calls = log.read_text(encoding="utf-8").splitlines() + verified_dry_runs = [ + line + for line in calls + if " install -n " in line and "-r resolver-verified" in line + ] + recovery_dry_run = next( + line + for line in calls + if " install -n -f " in line and "-r resolver-recovery" in line + ) + live_installs = [line for line in calls if " install -y " in line] + assert len(verified_dry_runs) == 2 + assert "bind920-9.20.26_2 bind-tools-9.20.26_2" in verified_dry_runs[0] + assert "os-bind-rp-26.1_1" not in verified_dry_runs[0] + assert "os-bind-rp-26.1_1" in verified_dry_runs[1] + assert "bind920-9.20.26_2" not in verified_dry_runs[1] + assert calls.index(recovery_dry_run) < calls.index(verified_dry_runs[0]) + assert calls.index(verified_dry_runs[0]) < calls.index(live_installs[0]) + assert calls.index(live_installs[0]) < calls.index(verified_dry_runs[1]) + assert calls.index(verified_dry_runs[1]) < calls.index(live_installs[1]) + + +def test_revalidates_the_plugin_plan_after_the_approved_bind_transaction( + tmp_path: Path, +) -> None: + result, log, _ = run_installer( + tmp_path, + bind920="bind920|9.20.25|dns/bind920", + bind_tools="bind-tools|9.20.25|dns/bind-tools", + confirmation="y", + dry_run_plan="plugin_unrequested_bind_after_fallback", + ) + + assert result.returncode != 0 + assert "unexpected package change: bind920" in result.stderr + live_installs = [ + line + for line in log.read_text(encoding="utf-8").splitlines() + if " install -y " in line + ] + assert len(live_installs) == 1 + assert "bind920-9.20.26_2 bind-tools-9.20.26_2" in live_installs[0] + assert "os-bind-rp-26.1_1" not in live_installs[0] + + def test_prompts_when_bind_tools_are_missing_or_from_the_wrong_origin(tmp_path: Path) -> None: result, _, _ = run_installer( tmp_path, @@ -747,6 +889,37 @@ def test_rejects_unrelated_pkg_dry_run_mutations(tmp_path: Path, plan: str) -> N assert " install -y " not in log.read_text(encoding="utf-8") +@pytest.mark.parametrize( + "plan", + [ + "unrequested_bind_install", + "unrequested_bind_upgrade", + "unrequested_bind_downgrade", + "unrequested_bind_reinstall", + "unrequested_bind_removal", + ], +) +def test_rejects_unapproved_bind_mutations_when_only_the_plugin_was_requested( + tmp_path: Path, plan: str +) -> None: + result, log, _ = run_installer(tmp_path, dry_run_plan=plan) + + assert result.returncode != 0 + assert "unexpected package change: bind920" in result.stderr + assert " install -y " not in log.read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + "plan", ["unrequested_os_bind_install", "unobserved_os_bind_removal"] +) +def test_rejects_unapproved_official_plugin_mutations(tmp_path: Path, plan: str) -> None: + result, log, _ = run_installer(tmp_path, dry_run_plan=plan) + + assert result.returncode != 0 + assert "unexpected package change: os-bind" in result.stderr + assert " install -y " not in log.read_text(encoding="utf-8") + + @pytest.mark.parametrize( "plan", ["unknown_section", "malformed_entry", "blank_continuation"], diff --git a/README.md b/README.md index b8f97b2af1..d185fdf9ad 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ Or install the signed repository and package end-to-end with the interactive installer: ```sh -fetch -o - https://raw.githubusercontent.com/resolver-plugins/plugins/master/scripts/install-os-bind-rp.sh | sh +curl -fsSL https://raw.githubusercontent.com/resolver-plugins/plugins/master/scripts/install-os-bind-rp.sh | sh ``` The installer does not enable the BIND plugin or service or change its user diff --git a/docs/package-repository.md b/docs/package-repository.md index b3570cf032..9805bfabcc 100644 --- a/docs/package-repository.md +++ b/docs/package-repository.md @@ -73,16 +73,31 @@ Configure the ABI-aware current plugin channel: series="$(opnsense-version -a)" repo_url="https://resolver-plugins.github.io/repository/pkg/\${ABI}/$series/latest" fetch_url="https://resolver-plugins.github.io/repository/pkg/$(pkg config ABI)/$series/latest" -install -d -m 0755 /usr/local/etc/pkg/keys /usr/local/etc/pkg/repos -fetch -o /usr/local/etc/pkg/keys/resolver-plugins.pub "$fetch_url/resolver-plugins.pub" -test "$(sha256 -q /usr/local/etc/pkg/keys/resolver-plugins.pub)" = \ - bd89d6f91807c71f8a744532c9ce2f97e9590f8858ac779bfb2f23c10804e07e || exit 1 +key=/usr/local/etc/pkg/keys/resolver-plugins.pub +install -d -m 0755 "${key%/*}" /usr/local/etc/pkg/repos +key_sha256=bd89d6f91807c71f8a744532c9ce2f97e9590f8858ac779bfb2f23c10804e07e +umask 077 +key_stage="$(mktemp "${key%/*}/.resolver-plugins.pub.XXXXXX")" || exit 1 +cleanup_key_stage() { + [ -z "${key_stage:-}" ] || rm -f "$key_stage" +} +trap cleanup_key_stage EXIT HUP INT TERM +chmod 0600 "$key_stage" || exit 1 +fetch -o "$key_stage" "$fetch_url/resolver-plugins.pub" || exit 1 +test "$(sha256 -q "$key_stage")" = "$key_sha256" || { + echo 'resolver-plugins public-key fingerprint verification failed' >&2 + exit 1 +} +chmod 0644 "$key_stage" || exit 1 +mv -f "$key_stage" "$key" || exit 1 +key_stage= +trap - EXIT HUP INT TERM cat > /usr/local/etc/pkg/repos/resolver-plugins.conf < "$state_directory/packages.before.txt" "$pkg_command" query '%n|%v|%o' | sort > "$state_directory/package-identities.before.txt" - cp "$state_directory/package-identities.before.txt" "$state_directory/installed-packages.txt" set +e "$pkg_command" lock -l > "$state_directory/package-locks.before.txt" lock_status=$? @@ -412,15 +416,25 @@ create_state_directory() { } prepare_temporary_directory() { + umask 077 if [ -n "${RP_TEMPORARY_DIRECTORY:-}" ] then - temporary_directory=$RP_TEMPORARY_DIRECTORY - mkdir -p "$temporary_directory" + requested_temporary_directory=$RP_TEMPORARY_DIRECTORY + [ "$requested_temporary_directory" != / ] || \ + fail 'refusing unsafe temporary directory: /' + mkdir -m 0700 "$requested_temporary_directory" 2>/dev/null || \ + fail "could not exclusively create temporary directory: $requested_temporary_directory" + temporary_directory=$requested_temporary_directory else - temporary_directory=$(mktemp -d /tmp/os-bind-rp-install.XXXXXX) + temporary_directory=$(mktemp -d /tmp/os-bind-rp-install.XXXXXX) || \ + fail 'could not create a temporary directory' temporary_directory_owned=yes fi - chmod 0700 "$temporary_directory" + if [ ! -d "$temporary_directory" ] || [ -L "$temporary_directory" ] || \ + [ "$temporary_directory" = / ] + then + fail "unsafe temporary directory: $temporary_directory" + fi archive_root="$temporary_directory/verified-repository" archive_all="$archive_root/All" isolated_repository_directory="$temporary_directory/isolated-repos" @@ -547,7 +561,7 @@ capture_recovery_packages() { write_repository resolver-recovery "file://$recovery_repository" yes \ "$recovery_repository_directory/resolver-recovery.conf" package_dry_run "$state_directory/pkg-recovery.dry-run.txt" \ - "$recovery_repository_directory" resolver-recovery yes "$@" + "$recovery_repository_directory" resolver-recovery yes '' "$@" } installed_record() { @@ -622,6 +636,7 @@ dry_run_awk_command=${RP_DRY_RUN_AWK_COMMAND:-awk} repository_directory=${RP_PKG_REPOSITORY_DIR:-/usr/local/etc/pkg/repos} key_directory=${RP_PKG_KEYS_DIR:-/usr/local/etc/pkg/keys} mkdir -p "$repository_directory" "$key_directory" +prepare_temporary_directory series=$(series_from_opnsense_version) repository_abi=$("$pkg_command" config ABI) || fail 'could not determine the pkg ABI' @@ -631,14 +646,9 @@ esac repository_url="$repository_base_url/\${ABI}/$series/latest" repository_fetch_url="$repository_base_url/$repository_abi/$series/latest" public_key="$key_directory/resolver-plugins.pub" -if [ -n "${RP_TEMPORARY_DIRECTORY:-}" ] -then - key_stage_directory=$RP_TEMPORARY_DIRECTORY -else - key_stage_directory=$(mktemp -d /tmp/os-bind-rp-key.XXXXXX) - key_stage_owned=yes -fi -mkdir -p "$key_stage_directory" +key_stage_directory="$temporary_directory/key-stage" +mkdir -m 0700 "$key_stage_directory" || fail 'could not create the public-key staging directory' +key_stage_owned=yes public_key_candidate="$key_stage_directory/resolver-plugins.pub" fetch -o "$public_key_candidate" "$repository_fetch_url/resolver-plugins.pub" @@ -693,15 +703,16 @@ fi official_plugin=$(installed_record os-bind) installed_plugin=$(installed_record os-bind-rp) +plugin_allowed_removals= if [ -n "$official_plugin" ] then + plugin_allowed_removals=os-bind printf '%s\n' "Replacing official os-bind ($(package_description "$official_plugin")) with os-bind-rp." >&2 elif [ -n "$installed_plugin" ] then printf '%s\n' "Upgrading installed os-bind-rp ($(package_description "$installed_plugin"))." >&2 fi -prepare_temporary_directory fetch_and_verify_archive bind920 "$candidate_bind920_version" dns/bind920 fetch_and_verify_archive bind-tools "$candidate_bind_tools_version" dns/bind-tools fetch_and_verify_archive os-bind-rp "$candidate_plugin_version" opnsense/os-bind-rp @@ -718,7 +729,6 @@ verify_archive_hashes create_state_directory cp "$temporary_directory/package-file-checksums.txt" "$state_directory/package-file-checksums.txt" cp "$temporary_directory/package-archives.txt" "$state_directory/package-archives.txt" -cp "$temporary_directory/package-archives.txt" "$state_directory/candidate-sha256.txt" printf '%s\n' "$candidate_bind920" "$candidate_bind_tools" "$candidate_plugin" \ > "$state_directory/candidates.txt" @@ -736,24 +746,45 @@ fi printf 'pkg_was_locked=%s\npkg_identity=%s\n' "$pkg_was_locked" "$pkg_identity_before" \ > "$state_directory/transaction.txt" -set -- "os-bind-rp-$candidate_plugin_version" -if [ "$bind_update_required" = yes ] -then - set -- "bind920-$candidate_bind920_version" "bind-tools-$candidate_bind_tools_version" "$@" -fi -package_dry_run "$state_directory/pkg-install.dry-run.txt" \ - "$isolated_repository_directory" resolver-verified no "$@" verify_archive_hashes capture_recovery_packages [ "$(installed_record pkg)" = "$pkg_identity_before" ] || \ fail 'pkg identity changed before the verified package transaction' -transaction_started=yes if [ "$bind_update_required" = yes ] then + package_dry_run "$state_directory/pkg-bind-install.dry-run.txt" \ + "$isolated_repository_directory" resolver-verified no '' \ + "bind920-$candidate_bind920_version" "bind-tools-$candidate_bind_tools_version" +else + package_dry_run "$state_directory/pkg-plugin-install.dry-run.txt" \ + "$isolated_repository_directory" resolver-verified no "$plugin_allowed_removals" \ + "os-bind-rp-$candidate_plugin_version" +fi +verify_archive_hashes + +if [ "$bind_update_required" = yes ] +then + [ "$(installed_record pkg)" = "$pkg_identity_before" ] || \ + fail 'pkg identity changed before the verified BIND transaction' + transaction_started=yes verified_pkg install -y -r resolver-verified \ "bind920-$candidate_bind920_version" "bind-tools-$candidate_bind_tools_version" + verify_installed_record "$candidate_bind920" + verify_installed_record "$candidate_bind_tools" + [ "$(installed_record os-bind)" = "$official_plugin" ] || \ + fail 'official os-bind changed during the BIND transaction' + [ "$(installed_record os-bind-rp)" = "$installed_plugin" ] || \ + fail 'os-bind-rp changed during the BIND transaction' + package_dry_run "$state_directory/pkg-plugin-install.dry-run.txt" \ + "$isolated_repository_directory" resolver-verified no "$plugin_allowed_removals" \ + "os-bind-rp-$candidate_plugin_version" fi + +verify_archive_hashes +[ "$(installed_record pkg)" = "$pkg_identity_before" ] || \ + fail 'pkg identity changed before the verified plugin transaction' +transaction_started=yes verified_pkg install -y -r resolver-verified "os-bind-rp-$candidate_plugin_version" [ -z "$(installed_record os-bind)" ] || fail 'official os-bind remains installed after replacement' @@ -762,6 +793,9 @@ if [ "$bind_update_required" = yes ] then verify_installed_record "$candidate_bind920" verify_installed_record "$candidate_bind_tools" +else + verify_installed_record "$bind920" + verify_installed_record "$bind_tools" fi verify_archive_ownership for installed_package in bind-tools bind920 os-bind-rp