fix(harden): close the four findings the verification round left open - #1546
Merged
Conversation
Each of these was found while verifying the previous change and recorded rather than fixed. That was the wrong call: they belong in the work that produced them. What decides whether a control is reported as in place was resolved through the caller's own environment. That mattered little while the command required privilege, because raising it sanitises what is searched — and the previous change made the command run without privilege by design, which put that decision in the hands of whoever runs it. It is now read from fixed locations, and something that is not there is reported as absent rather than as an answer. Raising a control to root has to open the location for a moment, because the owner cannot be changed while it is closed. Anything that arrived in that moment was then closed over — putting content at the exact location the control exists to keep empty, out of reach without privilege. The location is checked again before it is closed, and content found there ends the attempt. Nothing removed a control this command placed, which the record made a requirement of the verb and neither change delivered. `--take-back` does, and only that: what it will remove is recognised the same way a control is, so a location holding anything is not its to open. The removal is read back. Who the command acts for was established from one way of raising privilege. The others leave different traces, and under them the effective identity — root — was taken as the operator's, which is the defect the previous change fixed, one tool over. All of them are recognised now, and where privilege was raised and the invoker cannot be established it says so rather than assuming.
Ndevu12
added
pin-bump-deferred
Ndevu12
marked this pull request as ready for review
September 1, 2026 20:49
Two findings from the verification round on this branch. Raising a control to root has to take the lock off to change the owner, so the location is briefly open. When something arrived in that window the run reported it as a location that "already had something in it, so it was not changed" — while the lock was off and the owner was now root. The file that arrived could not be deleted from a directory the operator owns, and every later run repeated the same sentence. The location is now handed back: unlocked, owned by the operator, and writable, with a state of its own that says what happened and does not count as a pass. The other verb had the same window and reported it truthfully already, under a state name that said the opposite of what it had done. Both now use one state for it. Who a command acts for was decided by the presence of an environment variable, so a process that had raised no privilege at all could pick the account every location is graded against — after which a control that was in place read as absent for the rest of the host's life, and the command could not put it back. The kernel decides that now; the environment is read only to name the invoker, and markers that disagree are refused rather than resolved to whichever was read first. Also: the `lsattr` parse tests looked the tool up on the running host, so one of them passed on every machine without it without ever reaching the parse it names, and no test pinned an ordinary unlocked directory as unlocked — a mutation making every directory read as denied survived. And `harden/settings.py` shipped unable to be imported, which nothing noticed because nothing imports it yet. Refs #205
Ndevu12
marked this pull request as draft
September 1, 2026 21:30
`run()` took a `privileged` seam and never called it. Nineteen test call sites passed it, several as `lambda: False` while asserting the unprivileged path — which those tests reach through a stubbed `apply`, not through this. An injected control that nothing consults reads as coverage and is silence, the same shape as a call site that resolved to a module instead of the function it named. Privilege is asked of the path inside `apply_one`, which is where it is actually decided. Refs #205
…ot ours Four findings from the second verification round on this branch. Not sealing content in was written at one of the three exits that put the flag back. The other two — a raise to root that failed, and one whose read-back did not confirm — re-locked unconditionally and reported a reason that says nothing about content, so a run that lost the race AND failed sealed the arrival in and called it "could not be raised to root". Both now ask the same question, and the answer comes from a read-back after the window rather than from whether a call threw, which also catches a write landing between the check and the re-lock. What that exit then says about the owner was asserted rather than read. It is reached after a raise that never changed the owner, where it told the operator to fetch sudo for a directory already their own. An immutable empty directory owned by neither root nor this account answered "nothing holds this" over a directory that plainly does. The run then tried to write, got the EPERM the flag guarantees, and called it "could not be written" — a lock it can neither verify nor clear, described as an absent control and an unexplained failure. It is its own grade now: applying leaves it alone and names it, taking back refuses it rather than unlocking and deleting another account's lock, and the audit still declines to credit it as this tool's own work. It is also what a wrongly resolved account produces, so a real control stays visible instead of reading as gone. `str.isdigit` is true for characters `int` refuses, and the guard caught only `KeyError`, so an environment holding one took `ValueError` out through `resolve` and `acting_uid` into every caller that asks who holds a control. Two of the new tests drove the audit probe against the real machine — one of its other probes walks `$HOME` — at eight seconds each. They ask about the fixture now. Refs #205
… changed The round on the previous commit found the same class surviving in the helper that commit extracted to enforce it. `_lock_back_over_nothing` asked whether the location was empty and then set the flag: check-then-act, written directly underneath a docstring about not doing that, and the gap between those two calls is the whole problem. Setting the flag is one function now and the answer comes from a read-back AFTER the write, so a flag that closed over an arrival comes straight off again. `remove_one` had the same shape at its removal failure, where ANY failure re-locked, then the next run told the operator the lock was not this command's doing. Everything after the `chmod` changes the location, so everything after it now undoes what it changed. `0o555` denies the write to the OWNER as well: bailing out after it and reporting the location as untouched left the operator unable to remove the file they were being told to go and look at, and a run that failed later left a read-only directory that `--take-back` could not see. Both go through the same hand-back, which reports the state it reads rather than the one it expected — including the flag, whose half of that sentence the previous commit asserted while fixing the owner's half. Refusing a lock held by another account while running as root asserted a limit that was never read: `chflags(2)` says the flag may be cleared by the owner OR the super-user. So the refusal was false at root, it made raising a control to root unreachable for any lock not owned by the resolved account, and it gave no way forward where every sibling refusal names one. With privilege it is taken over; without, it says to re-run with sudo. Taking back still leaves it alone — that command removes only what it placed, which is true regardless of privilege. One of the new tests planted its arrival where the run returns at an earlier branch, so it never reached the exit it was named for and passed against the defect. It plants in the gap that exit exists for. Refs #205
Ndevu12
marked this pull request as ready for review
September 2, 2026 07:23
Measured against `origin/main` for the same files: comment-plus-docstring to code had gone from 0.25 to 0.37 in denial.py, with 23 inline comments where main had 1, and the test file's docstring density had doubled. What is left carries a measurement, a rejected alternative or a trap. The narrative each one was retelling is in the commit that made the change, which is where it belongs. Refs #205
Four comments sat above an assignment, which reads as documenting the value. None of them were about the value — each described the branch or the block underneath — so they moved into the docstring of the function that applies the rule, and what did not survive the move was not worth keeping. Refs #205
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft until a round comes back clean against this code.
Closes Ndevu12/saw#267, Ndevu12/saw#268, Ndevu12/saw#269, Ndevu12/saw#270 — all four were found while verifying
#1545and filed instead of fixed, which was the wrong call. They belong in the work that produced them.What decides whether a control is in place was influenced by the caller. It mattered little while the command required privilege, since raising it sanitises what is searched;
#1545made the command run without privilege by design, which put that decision in the hands of whoever runs it. It is now read from fixed locations, and something absent is reported as absent rather than as an answer.Raising a control has to open the location for a moment — the owner cannot be changed while it is closed. Anything arriving in that moment was then closed over, putting content at the exact location the control exists to keep empty and out of reach without privilege. The location is re-checked before it is closed again.
Nothing removed a control this command placed, which the record makes a requirement of the verb and neither previous change delivered.
--take-backdoes, and only that: what it removes is recognised the same way a control is, so a location holding anything is not its to open, and the removal is read back.Who the command acts for was established from one way of raising privilege. The others leave different traces, and under them the effective identity was taken as the operator’s — the same defect
#1545fixed, one tool over. Where privilege was raised and the invoker cannot be established, it now says so rather than assuming.Seven mutations, all killed. One survived twice first, and both times the fault was in the test: it asserted a return value that is identical on a host without the tool, then named a path that does not exist, so the code it meant to reach bailed before it got there.