backup.sh named archives with %Y%m%d, so a second run on the same day
silently overwrote the first -- destroying a good backup at the moment
someone was trying to take another. Names now carry seconds, and an
existing path is refused rather than clobbered. The archive is also
written to a .partial name and renamed only on success, with a trap to
clean up, so an interrupted run cannot leave a truncated file that looks
like a backup.
restore.sh extracted with tar's defaults. An archive is untrusted input
-- whoever produced it chooses the paths, ownership and modes inside it
-- and extracting as root let the tarball dictate uid/gid and restore
setuid bits directly. Now --no-same-owner --no-same-permissions, with -P
still absent so tar keeps stripping leading "/" and refusing ".."
members. It also lists what it is about to extract and confirms first,
since it silently overwrote whatever was already in the target.
Both scripts also gained set -euo pipefail, and both replaced the
`cmd; if [ $? -eq 0 ]` pattern with a direct `if cmd; then`, which is
what that idiom was reaching for and gets wrong as soon as any command
is inserted between the two lines.
Verified end to end: two same-second-apart backups both survive, no
.partial residue, restore refuses non-interactively, and the round trip
diffs identical.
set -euo pipefail across all four, but added deliberately rather than
pasted in -- each script needed the places where a non-zero exit is
normal handled first, or strict mode would have made them worse:
- sys_monitor / process_monitor: `ps | head -n 6` is a latent SIGPIPE.
head closes the pipe after six lines, and on a host with enough
processes ps fills the buffer and exits 141, which pipefail turns into
a script abort -- on exactly the busy machine you wanted to inspect.
Confirmed the mechanism (a large producer into head returns 141) and
those pipelines now tolerate it.
- security_audit: find exits non-zero when it cannot read a directory,
which is routine when walking the whole filesystem. Without handling,
set -e aborted the audit part way while still looking complete. Also
notes that a clean report as non-root means little, since find cannot
descend where it may not read.
- network_info: iptables needs root, so the last section aborted the
script for ordinary users. Now reports the failure, and falls back to
nft where iptables is absent.
process_monitor also no longer kills on sight. `pkill -x` by name can
match several processes at once, and as root that is an easy way to take
down more than intended. It now prints what it matched and asks, with
FORCE=1 for unattended use and a refusal rather than a hang when there
is no tty.
All four run clean; the kill path was tested against a live process and
left it alive.
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