Repository navigation
fix(cli): trust root-owned per-user /home entries during activation walk - #2309
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you for #2306 — a root-owned /home/<name> symlink is a common Linux layout, and the check you added is narrow in the right way: only root can create or change that entry, so another user cannot race the lstat and realpath window, and every component of the resolved path is still walked with O_NOFOLLOW.
Three things before it merges:
- Memory-core lint. The
lintjob is red because raw allocator use grew insrc/cli/activation_transaction.c(80 to 82). Building the mapped path in a stack buffer and returningactivation_string_copy(buf)keeps the count flat, and callers stillfree()it as before. - A test. This loosens a security check, so it needs one that fails without the change. Either a test seam that swaps the
/homeprefix and the uid checks, or a Linux container test that runs as root, would do. - Consider folding it into the existing
/homealias loop (the #2175 branch), which does almost the same walk. Note one deliberate difference to keep visible: the alias branch requires a root-owned target, while this one also accepts a target owned by the current user. That is reasonable, and worth one comment saying so.
Thank you again.
a9a2535 to
48265f5
Compare
Address review on DeusData#2309: - Fold the /home/<name> handling into the existing DeusData#2175 alias loop via a shared activation_alias_map() that builds the mapped path in a stack buffer and returns activation_string_copy(); raw allocator sites in activation_transaction.c drop from 80 to 78 (baseline lowered). - Comment the deliberate difference: the per-user entry may resolve to a directory owned by the current account, the whole-alias case may not. - Add cbm_activation_transaction_set_home_root_for_testing() (test seams only) and a Linux test that fails without the per-user branch. Co-authored-by: Claude <[email protected]> Signed-off-by: Glen Ko <[email protected]>
|
|
Managed Linux hosts keep /home a real directory and point /home/<user> at another tree (e.g. /local/home/<user>). The O_NOFOLLOW walk rejected that entry and install failed with 'activation transaction I/O failed'. Resolve /home/<name> only when /home is root-owned and not group/world writable, the entry is a root-owned symlink, and it resolves to a directory owned by root or the current user. Arbitrary user-owned symlinks are still rejected. Fixes DeusData#2306 Co-authored-by: Claude <[email protected]> Signed-off-by: BumaldaOverTheWater94 <[email protected]>
Address review on DeusData#2309: - Fold the /home/<name> handling into the existing DeusData#2175 alias loop via a shared activation_alias_map() that builds the mapped path in a stack buffer and returns activation_string_copy(); raw allocator sites in activation_transaction.c drop from 80 to 78 (baseline lowered). - Comment the deliberate difference: the per-user entry may resolve to a directory owned by the current account, the whole-alias case may not. - Add cbm_activation_transaction_set_home_root_for_testing() (test seams only) and a Linux test that fails without the per-user branch. Co-authored-by: Claude <[email protected]> Signed-off-by: Glen Ko <[email protected]>
48265f5 to
2fbf3fb
Compare
|
Hi @DeusData, |
DeusData
added
bug
|
Thanks for following up, and for the CodeQL rerun request. The earlier request for the project allocator and a focused root-owned-home regression remains the review baseline. I have not rerun CI in this sweep; please point out any update that addresses those requested changes so the next review can pick it up directly. |
Signed-off-by: Martin Vogel <[email protected]>
|
Thank you for the managed-home-directory fix and for working through the earlier review feedback! I merged current main into this branch and added two focused regression tests so the link-owner rejection and the explicit current-user target allowance are each exercised independently. The tests call the shared alias helper with real filesystem targets and test-only ownership inputs, so they do not need privileged ownership changes. Both tests fail when their respective guard is deliberately broken, and the restored macOS activation suite passes all 22 tests under ASan/UBSan. The ownership policy itself is unchanged by this follow-up. GitHub CI will validate the updated branch before merge. Thanks again for the contribution! |
Signed-off-by: Martin Vogel <[email protected]>
|
Thank you again for the activation-path fix and regression coverage! I refreshed this branch with current main to pick up the shared CI repairs that blocked its previous run. The merge was conflict-free, and fresh GitHub checks will validate the combined branch before any merge decision. |
|
Thank you again for the managed-home activation fix. Our October updates record the focused ownership regressions and baseline refresh as completed. The remaining follow-up is on us; sorry for the delay, and I’ll follow up soon. |
Signed-off-by: Martin Vogel <[email protected]>
|
Thank you @BumaldaOverTheWater94 for this fix, and for your patience while it waited on us. Pointing /home/ at a relocated home directory is a common layout on managed Linux hosts, and you kept the new trust check narrow: /home must be root-owned and not writable by others, the entry itself must be root-owned, and the resolved path is still walked component by component afterwards. All three points from the September review are covered on the branch:
Main has changed scripts/memory-core-baseline.txt since the last CI run, so we merged current main into this branch (merge commit 7716153, no conflicts) to get a fresh run on the combined tree. Once the required checks are green on that commit, we will merge it. If you push again before then, please pull the branch first so the merge commit is kept. Thanks again for the careful work on this one. |
|
Merged as 635f8e3 - thank you, @BumaldaOverTheWater94! /home/ entries that point at a relocated home directory now activate correctly on managed Linux hosts, with the narrow trust rule you designed (root-owned /home and entry, the resolved path still walked component by component). Thank you for working through all three review points and for your patience. |
Fixes #2306
Linux
installfailed withactivation transaction I/O failedwhen/homeis a real directory but/home/<user>is a root-owned symlink (for example/home/alice -> /local/home/alice). The #2175 alias handling only covers/homeitself being a symlink.activation_posix_walk_path()now also resolves/home/<name>(Linux only), but only when all of these hold:/homeis a root-owned directory that isn't group- or world-writableArbitrary user-owned symlinks are still rejected by the
O_NOFOLLOWwalk.Verification (AL2023 x86_64, gcc 11.5,
/home/$USERis a root-owned symlink to/local/home/$USER). I raninstall -y --force --skip-config --dir=$HOME_SANDBOX/binwith a sandbox HOME/CBM_CACHE_DIR under/home/$USER:main@ same base:error: failed to stage install candidate: activation transaction I/O failed, rc=1Install complete, rc=0scripts/test.shsuites: 8175 passed, 0 failed, 8 skipped (143 suites)clang-format20--dry-run --Werror: clean. clang-tidy and cppcheck weren't available locally.No unit test is included: the case needs a root-owned symlink under
/home, which the existing fixtures can't create without root.Written with Claude Code (AI-assisted), on behalf of the account owner, who is accountable for the change.