fix(permission-hardener): prevent whitelist fail-open and hardlink abort#371
Open
maybebyte wants to merge 2 commits into
Open
fix(permission-hardener): prevent whitelist fail-open and hardlink abort#371maybebyte wants to merge 2 commits into
maybebyte wants to merge 2 commits into
Conversation
check_nosuid_whitelist iterated with
"${policy_match_white_list[@]:-}". Under `set -o nounset` the `:-`
default expands an empty array to a single empty-string element, so
the loop ran once with an empty entry. The body test
`[[ "${target_file}" == *""* ]]` matches every string, so the
function returned 1 (whitelisted) for every file. Both callers use
`|| continue`, so an effective config with no `matchwhitelist` line
silently skipped all SUID/SGID hardening with exit 0 -- a fail-open.
Expand with ${arr[@]+"${arr[@]}"} instead, which yields no elements
when the array is empty and does not trip nounset.
output_stat returned 1 for a non-directory SUID/SGID file with a
hardlink count > 1. The four call sites invoke output_stat as a bare
command under `set -o errexit`, so that return propagated and
terminated the whole run instead of skipping the file. The following
`if [ -z "${file_name_from_stat}" ]` guard never caught this case
because file_name_from_stat was already populated.
The surrounding comment states hardlinked files are meant to be
skipped (scanning a hardlink pool is too costly). Blank the output
vars and return 0, matching the existing nonexistent-file skip path,
so the caller's empty-name check continues past the file. Genuine
error paths (stat failure, parse corruption) still return 1 and fail
loud.
Contributor
Author
|
AI assisted. Related: #308 (that is where the shell glob line was introduced, for performance reasons). |
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.
Summary
Two independent defects in
permission-hardener's SUID/SGID hardeningpath each cause hardening to be silently skipped rather than applied —
the worst failure mode for a hardening tool, since both exit 0 with no
obvious signal. Both are one-spot fixes in
check_nosuid_whitelistandoutput_stat.Changes
matchwhitelistno longer whitelists every file. Rootcause:
check_nosuid_whitelistlooped over"${policy_match_white_list[@]:-}"; underset -o nounsetthe:-expands an empty array to one empty-string element, and
[[ "$target" == *""* ]]matches everything, so the function returned"whitelisted" for every file and both
|| continuecallers skippedall hardening. Now expands with
${arr[@]+"${arr[@]}"}(no elementswhen empty, nounset-safe).
cause:
output_statreturned 1 for a non-directory file with linkcount > 1; as a bare call under
errexitthat terminated the wholepass, and the following
[ -z "${file_name_from_stat}" ]guard nevercaught it because the var was already set. Now blanks the output vars
and returns 0 — matching the existing nonexistent-file skip path and
the "cannot handle hardlinks" comment — so the caller continues.
Testing
bash -non the modified script — no syntax errors.check_nosuid_whitelistextracted and exercised underset -u: empty matchwhitelist → hardens (return 0); matching entry →skips (return 1); non-matching entry → hardens. All three correct;
the empty case fails against the pre-fix function.
output_statrun against an actual 2-link hardlinkedfile: file is skipped, the loop continues to the next (single-link)
file which is still hardened, and the run exits 0 instead of aborting.
Notes for reviewers
genuine error paths (stat failure, parse corruption) still
return 1and fail loud under
errexit. The alternative —output_stat … || continueat all four call sites — is simpler but would also swallowthose genuine errors, so it was rejected in favor of the narrower fix.
permission-hardener.d/*.confprovide manymatchwhitelistentries,and hardlinked SUID binaries are uncommon); they surface on custom
effective configs and unusual filesystems respectively.