Model what each parser does with an empty and a dangling directive
Review of the previous commits turned up four places where the predicates and their tests disagreed with the tools they are modelling, each checked against udevadm verify, systemd-analyze verify and visudo -cf rather than against reading of the sources. An empty ExecStop= resets the list, so a unit an administrator neutralised that way runs nothing at shutdown and is no longer ours to remove; the predicate now tracks the last state instead of returning on the first home path it sees. A file whose last line ends in a backslash still carries a live directive for systemd, so the pending logical line is emitted at EOF; udev ignores such a line and sudo rejects the file outright, so this costs those two nothing. The scanner's taint pass now reads += appends, which its own comment already promised: the value of an append is no use, but a name that reaches a user root through one has to be judged on it. Two regression guards passed against the implementations they were written for. The udev continuation fixture put the whole RUN+= below the comment, so it matched whether or not the pending half was carried across; the split now falls inside the RUN+= value. The sudoers one kept its file on the strength of a spec above the comment, so it could not fail either; the hand-written spec now sits below. Both fail against a mutant that discards the pending line. The comment above the second also claimed a continued comment stays a comment, which visudo contradicts.
This commit is contained in:
@@ -176,16 +176,17 @@ declare -A VARS_TAINTED=()
|
||||
# isolation would miss in whichever direction it picked.
|
||||
collect_vars() {
|
||||
local -n source_lines="$1"
|
||||
local line name value
|
||||
local line name value append
|
||||
|
||||
VARS=()
|
||||
VARS_TAINTED=()
|
||||
for line in "${source_lines[@]}"; do
|
||||
[[ $line =~ ^[[:space:]]*# ]] && continue
|
||||
[[ $line =~ ^[[:space:]]*(local|declare|export|readonly|typeset)?[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)=(.*)$ ]] || continue
|
||||
[[ $line =~ ^[[:space:]]*(local|declare|export|readonly|typeset)?[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)(\+?)=(.*)$ ]] || continue
|
||||
|
||||
name=${BASH_REMATCH[2]}
|
||||
value=${BASH_REMATCH[3]}
|
||||
append=${BASH_REMATCH[3]}
|
||||
value=${BASH_REMATCH[4]}
|
||||
value=${value%%[[:space:]]#*}
|
||||
value=${value%[[:space:]]}
|
||||
if [[ $value == \"*\" || $value == \'*\' ]]; then
|
||||
@@ -193,6 +194,11 @@ collect_vars() {
|
||||
fi
|
||||
|
||||
mentions_user_writable_root "$value" && VARS_TAINTED["$name"]=1
|
||||
|
||||
# An append never wins the value -- an array grown across a file resolves to
|
||||
# nothing useful -- but it does carry the taint, or a name could reach a user
|
||||
# root through += and never be judged on it.
|
||||
[[ -n $append ]] && continue
|
||||
[[ -v VARS[$name] ]] || VARS["$name"]=$value
|
||||
done
|
||||
}
|
||||
@@ -457,9 +463,8 @@ privileged_destination() {
|
||||
fi
|
||||
done < <(command_destinations "$line")
|
||||
|
||||
# An elevated write whose destination cannot be resolved counts as
|
||||
# privileged: sudo tee is not aimed at a user's own dotfile, and assuming
|
||||
# otherwise is how this bug class survived six reviews.
|
||||
# An elevated write whose destination cannot be resolved counts as privileged:
|
||||
# sudo tee is not aimed at a user's own dotfile.
|
||||
if ((elevated == 0)) && ((${#unresolved[@]} > 0)); then
|
||||
printf '%s' "${unresolved[0]} (unresolved destination of an elevated write)"
|
||||
return 0
|
||||
|
||||
Reference in New Issue
Block a user