The workflow went in at severity: error on the assumption that a
never-linted repository would have a backlog worth grandfathering. It
did not -- error found nothing, and warning found exactly two things, so
the cautious setting was protecting against a problem that was not
there.
zimbra_backup.sh: SC2024, sudo does not affect redirects. The
`> "$BACKUP_FILE"` runs as root rather than as the sudo'd zimbra user,
so backups landed root-owned inside a directory the script deliberately
chowns to zimbra:zimbra. Kept the redirect -- root can always write
there, and piping into `tee` would put tee's status in $? and hide a
zmmailbox failure -- and handed ownership over explicitly afterwards.
The suppression is narrow and states why.
disk_cleanup.sh: SC2034, total_freed was assigned and never read.
CI now holds at warning with a clean tree, so anything that trips it is
new rather than inherited.
Every file here is a shell script that the README invites people to run
as root, and nothing has been linting them: .github/workflows was
deleted in 5eadf64 and never replaced. The bugs found in this week's
audit -- values interpolated into a command string, an unguarded
find -delete, exit statuses read from $? after the fact -- are largely
the class a linter catches for free.
Starts at severity: error deliberately. The existing scripts have never
been through a lint pass, so failing on warnings from day one would
block every PR on pre-existing findings rather than on new ones. The
intent is to tighten to warning, then to nothing, as the backlog is
cleared.
Two problems, both data loss.
It gzipped any *.log older than the threshold. gzip writes the .gz and
unlinks the original, so a daemon holding that file open keeps writing
to an unlinked inode and those writes become unreachable -- the exact
failure real logrotate avoids with copytruncate or a postrotate signal.
We can do neither from here, so files currently held open are now
skipped and left for logrotate, and the run says so. If lsof is missing
the check cannot run, and that is reported rather than assumed safe.
It also deleted every .gz older than a hardcoded 90 days, on every run,
with no flag, no dry-run and no confirmation -- destroying archives on
any host with longer retention. Deletion is now opt-in via --purge-days,
prints what it will remove, and needs an interactive confirmation or
--yes. Without a tty and without --yes it refuses instead of proceeding.
Adds --dry-run, --days, -h, and set -euo pipefail. A bare numeric first
argument still works, so existing `log_rotate.sh 14` callers and cron
entries are unaffected.
Verified against a fixture directory: old logs compressed and recent
ones left alone, archives surviving when --purge-days is absent, purge
refusing non-interactively without --yes, and a file held open by a
running process skipped rather than compressed.
--dirs accepted any path and fed it to `find -delete` running as root,
with no confirmation: `--clean --dirs /home` removed every file in /home
past the age threshold, and `--dirs /` did it system-wide.
Now refuses protected directories, comparing the readlink -f resolved
path so a symlink or /tmp/../home cannot smuggle one through, and
requires absolute paths. Anything outside the /tmp,/var/tmp defaults
also needs an interactive confirmation -- or --yes, so unattended use
stays possible; without a tty and without --yes it refuses rather than
hanging in cron.
usage() sliced fixed line numbers (head -22 | tail -n +18) and had
already outgrown them, truncating --help mid-list at --age. So --dirs,
the one option that could destroy a system, was the one option --help
never mentioned. Replaced with a sed range that tracks the comment block
wherever it moves.
The first version of the protected-path check let "/" through: it
compared against "${p%/}", which turns the "/" entry into an empty
string that matches nothing. Caught by testing the guard against the
paths it exists to stop, rather than assuming it worked.
Both scripts built a command string by interpolating user input into
bash -c:
sudo -u zimbra bash -c "... -m '$EMAIL' ..."
The single quotes inside the double-quoted string are not protection --
the outer shell expands $EMAIL first. An address of
x' ; id ; echo '
closes the quote and runs arbitrary commands. Both scripts require root
and invoke this through sudo -u zimbra, so injected commands execute as
the account that owns the entire mail store. Verified against the exact
quoting pattern before and after the change.
Fixed by single-quoting the script body so nothing is interpolated, and
passing values as positional arguments. The bash -c wrapper is kept
deliberately rather than calling zmmailbox directly, since it may depend
on shell setup and this could not be tested against a live Zimbra.
Two related holes in the same input paths:
- $EMAIL is also part of the backup filename, so a "/" wrote outside
$BACKUP_DIR. Now validated as a plain address.
- The restore prompt took a filename and concatenated it into a path, so
"../../etc/shadow" escaped $BACKUP_DIR. Now rejects anything
containing a separator.
Also switched the backup listing from `ls | grep "$EMAIL"` to a find
with grep -F: unquoted the address was treated as a regex, so "." in it
matched any character.
jcoffey-dev
LINUXexpert.org