From f8a86736f945e9599baac20568312b20b88e6097 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sat, 22 Aug 2026 22:22:05 -0700 Subject: [PATCH] Harden user_manage, update_system, and the Zimbra pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit user_manage.sh ran useradd/userdel/usermod with no privilege check at all, so an ordinary user got "Failed to create user." with no hint that root was the missing piece. Mutating subcommands now require root while listusers/listgroups stay open, and the check runs *after* the subcommand is recognised so a bare invocation still prints usage instead of complaining about privileges. Account names are validated before reaching useradd, and `deluser` -- which removes the home directory irrecoverably -- prints what it will delete and confirms first. zimbra_backup.sh reported success on failed backups. getRestURL can write an HTTP error body and still exit zero, so a "✅ Backup completed" could sit over a file containing an error page. Size and gzip -t checks now gate that, and a suspect file is renamed .suspect rather than deleted, so it can be looked at. zimbra_restore.sh likewise validates the archive before starting a restore from it. Both Zimbra scripts had `cmd` followed by `if [ $? -eq 0 ]`. Adding set -e to those would have made the error branches unreachable -- set -e exits before the check -- so the tests are inline instead. That would have been a silent regression rather than a visible one. update_system.sh gained strict mode, and a note on the pacman branch: Arch has no supported partial-upgrade path, and --noconfirm answers away the prompts that would otherwise warn. Integrity checks verified against an error page, a truncated archive, a non-gzip file and a real one. --- update_system.sh | 12 +++++++- user_manage.sh | 75 +++++++++++++++++++++++++++++++++++++++-------- zimbra_backup.sh | 31 ++++++++++++++++---- zimbra_restore.sh | 15 ++++++++-- 4 files changed, 111 insertions(+), 22 deletions(-) diff --git a/update_system.sh b/update_system.sh index 5888d9b..38eef10 100644 --- a/update_system.sh +++ b/update_system.sh @@ -17,7 +17,12 @@ # # Usage: update_system.sh (no arguments, run as root) # Description: Detects the Linux distro's package manager and installs all updates. - +# +# Unattended: every branch below passes -y or --noconfirm, so this will +# apply whatever the configured repositories offer without asking. + +set -euo pipefail + # Ensure running as root if [ "$EUID" -ne 0 ]; then echo "Please run as root to apply system updates." @@ -41,6 +46,11 @@ elif command -v zypper &> /dev/null; then zypper refresh && zypper update -y elif command -v pacman &> /dev/null; then echo "Updating with pacman..." + # Arch has no supported partial-upgrade path: -Syuu on a stale mirror + # list or a half-updated system can leave unbootable library mismatches, + # and --noconfirm answers away the prompts that would have warned. Kept + # for parity with the other package managers, but this is the branch to + # be most careful with. pacman -Syuu --noconfirm else echo "Error: No supported package manager found on this system." diff --git a/user_manage.sh b/user_manage.sh index fdcf82a..daf25de 100644 --- a/user_manage.sh +++ b/user_manage.sh @@ -21,45 +21,96 @@ # listusers, listgroups # Description: Automates user/group creation, deletion, and modifications. # Requires root privileges for most operations. - -subcmd="$1" + +set -euo pipefail + +subcmd="${1:-}" + +# The read-only subcommands work for anyone; everything else edits +# /etc/passwd, /etc/group or /etc/shadow. Without this the script ran +# useradd/userdel/usermod as an ordinary user and reported "Failed to +# create user." with no hint that privilege was the problem. +usage() { + echo "Usage: $0 {adduser|deluser|addgroup|delgroup|addtogroup|lock|unlock|listusers|listgroups}" +} + +case "$subcmd" in + # Read-only: anyone may run these. + listusers|listgroups) ;; + # Mutating: gate on root. Checked after the subcommand is recognised, + # so `user_manage.sh` with no arguments still prints usage instead of + # complaining about privileges. + adduser|deluser|addgroup|delgroup|addtogroup|lock|unlock) + if [ "$EUID" -ne 0 ]; then + echo "This action requires root. Re-run with sudo." >&2 + exit 1 + fi + ;; + *) + usage >&2 + exit 1 + ;; +esac + +# Usernames and group names reach useradd/userdel directly. Keep them to +# what a POSIX account name may contain, so nothing surprising is passed +# through as an option or a path. +valid_name() { + [[ "$1" =~ ^[a-z_][a-z0-9_-]{0,31}$ ]] +} + case "$subcmd" in adduser) - user="$2" + user="${2:-}" if [ -z "$user" ]; then echo "Username required. Usage: $0 adduser "; exit 1; fi + valid_name "$user" || { echo "Not a valid username: $user" >&2; exit 1; } # Create user with a home directory (-m) and default settings useradd -m "$user" && echo "User '$user' created." || echo "Failed to create user." ;; deluser) - user="$2" + user="${2:-}" if [ -z "$user" ]; then echo "Username required. Usage: $0 deluser "; exit 1; fi - # Delete user and remove home directory (-r) - userdel -r "$user" && echo "User '$user' deleted." || echo "Failed to delete user." + valid_name "$user" || { echo "Not a valid username: $user" >&2; exit 1; } + if ! id -u "$user" >/dev/null 2>&1; then echo "No such user: $user" >&2; exit 1; fi + # -r removes the home directory and mail spool. That is unrecoverable, + # so show what is about to go and confirm. + echo "About to delete user '$user' and remove:" + echo " home: $(getent passwd "$user" | cut -d: -f6)" + if [ "${ASSUME_YES:-}" != "1" ]; then + if [ ! -t 0 ]; then + echo "Refusing without confirmation; set ASSUME_YES=1 for unattended use." >&2 + exit 1 + fi + read -r -p "Delete user '$user' and their home directory? (yes/NO): " reply + [ "$reply" = "yes" ] || { echo "Cancelled."; exit 0; } + fi + userdel -r "$user" && echo "User '$user' deleted." || { echo "Failed to delete user." >&2; exit 1; } ;; addgroup) - group="$2" + group="${2:-}" if [ -z "$group" ]; then echo "Group name required. Usage: $0 addgroup "; exit 1; fi + valid_name "$group" || { echo "Not a valid group name: $group" >&2; exit 1; } groupadd "$group" && echo "Group '$group' created." || echo "Failed to create group." ;; delgroup) - group="$2" + group="${2:-}" if [ -z "$group" ]; then echo "Group name required. Usage: $0 delgroup "; exit 1; fi groupdel "$group" && echo "Group '$group' deleted." || echo "Failed to delete group." ;; addtogroup) - user="$2"; group="$3" + user="${2:-}"; group="$3" if [ -z "$user" ] || [ -z "$group" ]; then echo "Usage: $0 addtogroup "; exit 1; fi usermod -aG "$group" "$user" && echo "Added user '$user' to group '$group'." || echo "Failed to modify group membership." ;; lock) - user="$2" + user="${2:-}" if [ -z "$user" ]; then echo "Username required. Usage: $0 lock "; exit 1; fi usermod -L "$user" && echo "User '$user' account locked." || echo "Failed to lock account." ;; unlock) - user="$2" + user="${2:-}" if [ -z "$user" ]; then echo "Username required. Usage: $0 unlock "; exit 1; fi usermod -U "$user" && echo "User '$user' account unlocked." || echo "Failed to unlock account." ;; @@ -70,7 +121,7 @@ case "$subcmd" in cut -d: -f1 /etc/group ;; *) - echo "Usage: $0 {adduser|deluser|addgroup|delgroup|addtogroup|lock|unlock|listusers|listgroups}" + usage >&2 exit 1 ;; esac diff --git a/zimbra_backup.sh b/zimbra_backup.sh index da98b97..bd68ede 100644 --- a/zimbra_backup.sh +++ b/zimbra_backup.sh @@ -14,6 +14,8 @@ # You should have received a copy of the GNU General Public License along # with this program. If not, see . +set -euo pipefail + # Ensure script is run as root or sudo if [ "$EUID" -ne 0 ]; then echo "❌ This script must be run as root or with sudo." @@ -63,14 +65,31 @@ echo "📦 Starting backup..." # a zmmailbox failure), but it means the file lands root-owned inside a # directory chowned to zimbra, so ownership is handed over below. # shellcheck disable=SC2024 -sudo -u zimbra bash -c '/opt/zimbra/bin/zmmailbox -z -m "$1" getRestURL "//?fmt=tgz"' _ "$EMAIL" > "$BACKUP_FILE" - -# Verify success -if [ $? -eq 0 ]; then +# Tested inline rather than via `$?` on the next line: under set -e a +# failure would exit before any check ran, making the error branch below +# dead code. +if sudo -u zimbra bash -c '/opt/zimbra/bin/zmmailbox -z -m "$1" getRestURL "//?fmt=tgz"' _ "$EMAIL" > "$BACKUP_FILE"; then + # A zero exit is not sufficient: getRestURL can write an HTTP error + # body and still succeed, which previously produced a cheerful "✅" + # over a file containing an error page. Require a plausible size and + # a readable gzip container. + size="$(stat -c %s -- "$BACKUP_FILE" 2>/dev/null || echo 0)" + if [ "$size" -lt 1024 ]; then + echo "❌ Backup is only ${size} bytes -- almost certainly an error response, not a mailbox." + echo " Leaving it at ${BACKUP_FILE}.suspect for inspection." + mv -- "$BACKUP_FILE" "${BACKUP_FILE}.suspect" + exit 1 + fi + if ! gzip -t -- "$BACKUP_FILE" 2>/dev/null; then + echo "❌ Backup is not a valid gzip stream -- treating as failed." + echo " Leaving it at ${BACKUP_FILE}.suspect for inspection." + mv -- "$BACKUP_FILE" "${BACKUP_FILE}.suspect" + exit 1 + fi chown zimbra:zimbra "$BACKUP_FILE" 2>/dev/null || true - echo "✅ Backup completed: $BACKUP_FILE" + echo "✅ Backup completed: $BACKUP_FILE ($(numfmt --to=iec "$size" 2>/dev/null || echo "$size bytes"))" else echo "❌ Backup failed. Check if the user exists or zmmailbox is working." - rm -f "$BACKUP_FILE" + rm -f -- "$BACKUP_FILE" exit 1 fi diff --git a/zimbra_restore.sh b/zimbra_restore.sh index d6a70ee..0de22c8 100644 --- a/zimbra_restore.sh +++ b/zimbra_restore.sh @@ -14,6 +14,8 @@ # You should have received a copy of the GNU General Public License along # with this program. If not, see . +set -euo pipefail + # Ensure script is run as root or with sudo if [ "$EUID" -ne 0 ]; then echo "❌ This script must be run as root or with sudo." @@ -70,10 +72,17 @@ echo "🔄 Restoring backup..." # arrive as positional arguments. Interpolating them (as this line # previously did) let shell metacharacters in either value run commands # as the zimbra user. -sudo -u zimbra bash -c '/opt/zimbra/bin/zmmailbox -z -m "$1" postRestURL "/?fmt=tgz&resolve=skip" --file "$2"' _ "$EMAIL" "$FULL_PATH" +# Refuse a backup that is not a readable gzip stream before handing it to +# zmmailbox -- a truncated or error-page "backup" should fail here, with +# a clear reason, rather than part way through a restore. +if ! gzip -t -- "$FULL_PATH" 2>/dev/null; then + echo "❌ $FILENAME is not a valid gzip archive -- refusing to restore from it." + exit 1 +fi -# Check result -if [ $? -eq 0 ]; then +# Tested inline rather than via `$?`: under set -e a failure would exit +# before the check, making the error branch below unreachable. +if sudo -u zimbra bash -c '/opt/zimbra/bin/zmmailbox -z -m "$1" postRestURL "/?fmt=tgz&resolve=skip" --file "$2"' _ "$EMAIL" "$FULL_PATH"; then echo "✅ Restore completed successfully for $EMAIL" else echo "❌ Restore failed. Please verify mailbox exists and backup file integrity."