diff --git a/doc/manual/rl-next/failed-cve-mitigation.md b/doc/manual/rl-next/failed-cve-mitigation.md new file mode 100644 index 000000000..4d848ff1c --- /dev/null +++ b/doc/manual/rl-next/failed-cve-mitigation.md @@ -0,0 +1,35 @@ +--- +synopsis: "Revert CVE-2025-52992 failed mitigation" +issues: [fj#883, fj#887] +cls: [3444, 3528] +category: "Fixes" +credits: ["raito", "horrors"] +--- + +Following the initial mitigation of **CVE-2025-52992** in `cl/3444`, we +received reports of **unexpected deletion of in-use store paths**. + +Upon investigation, we found that the patch did **not correctly cancel all +automatic deleters**, resulting in potentially critical path loss during normal +operation. + +Given the severity and time-sensitive nature of the situation ([see incident +report](https://lix.systems/blog/2025-06-27-lix-critical-bug/)), we evaluated +possible options to repair the behavior safely. However, we concluded that a +rushed fix would either + +* **Overdelete**, i.e. breaking running systems, or, +* **Underdelete**, effectively **reopening CVE-2025-52992** while leaving + orphaned paths behind. + +As **CVE-2025-52992 has no known exploit vector**, and correctness is critical +in the Lix project, we have **fully reverted the previous mitigations**. + +The affected patches (`cl/3444`) have been rolled back for the time being. + +Moving forward, the Lix team will rework this code path in a **long-term, +correctness-first fix** on the main branch. We will explore backporting it to +stable channels once its safety is assured. + +We are deeply sorry for the stability incident and the Lix team remain +available for assisting you in recovering your systems. diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index c866a3b66..6e8d9480f 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -394,13 +394,9 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck() void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() { - /* In the past, redirected outputs were manually tracked for deletion. - * Now that we have the scratch outputs cleaner which are a superset of - * redirected outputs, we just fire all uncancelled automatic deleters now. - * - * This should clean up any paths that IS NOT registered in the database. - */ - scratchOutputsCleaner.clear(); + /* Delete unused redirected outputs (when doing hash rewriting). */ + for (auto & i : redirectedOutputs) + deletePath(worker.store.Store::toRealPath(i.second)); /* Delete the chroot (if we were using one). */ autoDelChroot.reset(); /* this runs the destructor */ @@ -535,10 +531,6 @@ try { to use a temporary path */ makeFallbackPath(status.known->path); scratchOutputs.insert_or_assign(outputName, scratchPath); - /* Schedule this scratch output path for automatic deletion - * if we do not cancel it, e.g. when registering the outputs. - */ - scratchOutputsCleaner.insert_or_assign(outputName, worker.store.printStorePath(scratchPath)); /* Substitute output placeholders with the scratch output paths. We'll use during the build. */ @@ -561,6 +553,8 @@ try { std::string h2 { scratchPath.hashPart() }; inputRewrites[h1] = h2; } + + redirectedOutputs.insert_or_assign(std::move(fixedFinalPath), std::move(scratchPath)); } /* Construct the environment passed to the builder. */ @@ -2035,9 +2029,7 @@ try { } /* Don't register anything, since we already have the - previous versions which we're comparing. - NOTE: this means that the `.check` path will be automatically deleted. - */ + previous versions which we're comparing. */ continue; } @@ -2061,13 +2053,8 @@ try { /* If it's a CA path, register it right away. This is necessary if it isn't statically known so that we can safely unlock the path before the next iteration */ - if (newInfo.ca) { + if (newInfo.ca) TRY_AWAIT(localStore.registerValidPaths({{newInfo.path, newInfo}})); - /* Cancel automatic deletion of that output if it was a scratch output. */ - if (auto cleaner = scratchOutputsCleaner.extract(outputName)) { - cleaner.mapped().cancel(); - } - } infos.emplace(outputName, std::move(newInfo)); } @@ -2107,13 +2094,6 @@ try { infos2.insert_or_assign(newInfo.path, newInfo); } TRY_AWAIT(localStore.registerValidPaths(infos2)); - - /* Cancel automatic deletion of that output if it was a scratch output that we just registered. */ - for (auto & [outputName, _ ] : infos) { - if (auto cleaner = scratchOutputsCleaner.extract(outputName)) { - cleaner.mapped().cancel(); - } - } } /* In case of a fixed-output derivation hash mismatch, throw an @@ -2147,13 +2127,6 @@ try { builtOutputs.emplace(outputName, thisRealisation); } - /* NOTE: At this point, all outputs MAY NOT have been registered. - * Therefore, there may remains auto-deleters pending in the cleaner list (`scratchOutputsCleaner`). - * - * They will be finally deleted but we have no way to assert they all have been, e.g. - * `assert(scratchOutputsCleaner.size() == 0)` cannot be written. - */ - co_return builtOutputs; } catch (...) { co_return result::current_exception(); diff --git a/lix/libstore/build/local-derivation-goal.hh b/lix/libstore/build/local-derivation-goal.hh index 86a6cd80c..740357a7a 100644 --- a/lix/libstore/build/local-derivation-goal.hh +++ b/lix/libstore/build/local-derivation-goal.hh @@ -100,6 +100,8 @@ struct LocalDerivationGoal : public DerivationGoal * Hash rewriting. */ StringMap inputRewrites, outputRewrites; + typedef map RedirectedOutputs; + RedirectedOutputs redirectedOutputs; /** * The outputs paths used during the build. @@ -116,19 +118,6 @@ struct LocalDerivationGoal : public DerivationGoal * self-references. */ OutputPathMap scratchOutputs; - /** - * Output paths used during the build are scheduled for - * automatic cleanup unless they have been successfully built. - * - * `registerOutputs` take care of cancelling the cleanups - * and clearing this vector. - * - * `startBuilder` take care of filling this vector - * as `scratchOutputs` gets filled. - * - * This is a map from output names to automatic delete handles. - */ - std::map scratchOutputsCleaner; /** * Path registration info from the previous round, if we're diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 5e76244dc..9bcbc8e25 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -169,9 +169,6 @@ in symlinkResolvconf = runNixOSTestFor "x86_64-linux" ./symlink-resolvconf.nix; - # Use this test to test things that cannot easily be tested under chroot Nix stores in functional test suite. - non-chroot-misc = runNixOSTestFor "x86_64-linux" ./non-chroot-misc; - noNewPrivilegesInSandbox = runNixOSTestFor "x86_64-linux" ./no-new-privileges/sandbox.nix; noNewPrivilegesOutsideSandbox = runNixOSTestFor "x86_64-linux" ./no-new-privileges/no-sandbox.nix; diff --git a/tests/nixos/non-chroot-misc/default.nix b/tests/nixos/non-chroot-misc/default.nix deleted file mode 100644 index 93a66a595..000000000 --- a/tests/nixos/non-chroot-misc/default.nix +++ /dev/null @@ -1,34 +0,0 @@ -{ ... }: -# Misc things we want to test inside of a non redirected, non chroot Nix store. -let - nonAutoCleaningFailingDerivationCode = '' - derivation { - name = "scratch-failing"; - system = builtins.currentSystem; - builder = "/bin/sh"; - args = [ (builtins.toFile "builder.sh" "echo bonjour > $out; echo out: $out; false") ]; - } - ''; -in -{ - name = "non-chroot-sandbox-misc"; - - nodes.machine = { - }; - - testScript = { nodes }: '' - import re - start_all() - - # You might ask yourself why write such a convoluted thing? - # The condition for fooling Nix into NOT cleaning up the output path are non trivial and unclear. - # This is one of those: create a derivation, mkdir or touch the $out path, communicate it back. - # Even with a sandboxed Lix, you will observe leftovers before 2.93.0. After this version, this test passes. - result = machine.fail("""nix-build --substituters "" -E '${nonAutoCleaningFailingDerivationCode}' 2>&1""") - match = re.search(r'out: (\S+)', result) - assert match is not None, "Did not find Nix store path in the result of the failing build" - outpath = match.group(1).strip() - print(f"Found Nix store path: {outpath}") - machine.fail(f'stat {outpath}') - ''; -}