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