Fix security and correctness bugs found in code review

- Add shquote() to escape single quotes in paths passed to remote_exec,
  preventing shell injection via REMOTE_BASE containing single quotes
- Apply shquote to remote_exec calls in remotes.sh, backup.sh, transfer.sh, ssh.sh
- Add DISK_USAGE_THRESHOLD validation in config.sh
- Export SMTP_PASSWORD (was missing from export list)
- Fix WEB_PORT default mismatch: use 2323 consistently in from_conf and settings save
- Narrow exception catch in remotes.py disk info fetch to KeyError/LookupError
- Quote REMOTE_KEY in build_rsync_ssh_cmd for paths with spaces

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
shuki
2026-03-06 08:13:20 +02:00
parent 1c21e7a9df
commit 63cc7f842e
9 changed files with 29 additions and 12 deletions

View File

@@ -181,7 +181,8 @@ METAEOF
elif [[ "${REMOTE_TYPE:-ssh}" == "local" ]]; then
echo "$meta_json" > "$snap_dir/${ts}.partial/meta.json" || log_warn "Failed to write meta.json"
else
echo "$meta_json" | remote_exec "cat > '$snap_dir/${ts}.partial/meta.json'" || log_warn "Failed to write meta.json"
local sq_partial; sq_partial="$(shquote "$snap_dir/${ts}.partial")"
echo "$meta_json" | remote_exec "cat > '${sq_partial}/meta.json'" || log_warn "Failed to write meta.json"
fi
# 11. Generate manifest.txt
@@ -193,7 +194,8 @@ METAEOF
elif [[ "${REMOTE_TYPE:-ssh}" == "local" ]]; then
find "$snap_dir/${ts}.partial" -type f 2>/dev/null > "$snap_dir/${ts}.partial/manifest.txt" || log_warn "Failed to write manifest.txt"
else
remote_exec "find '$snap_dir/${ts}.partial' -type f > '$snap_dir/${ts}.partial/manifest.txt'" 2>/dev/null || log_warn "Failed to write manifest.txt"
local sq_partial; sq_partial="$(shquote "$snap_dir/${ts}.partial")"
remote_exec "find '${sq_partial}' -type f > '${sq_partial}/manifest.txt'" 2>/dev/null || log_warn "Failed to write manifest.txt"
fi
# 12. Finalize snapshot
@@ -211,7 +213,8 @@ METAEOF
elif [[ "${REMOTE_TYPE:-ssh}" == "local" ]]; then
total_size=$(du -sb "$snap_dir/$ts" 2>/dev/null | cut -f1) || total_size=0
else
total_size=$(remote_exec "du -sb '$snap_dir/$ts' 2>/dev/null | cut -f1" 2>/dev/null) || total_size=0
local sq_snap; sq_snap="$(shquote "$snap_dir/$ts")"
total_size=$(remote_exec "du -sb '$sq_snap' 2>/dev/null | cut -f1" 2>/dev/null) || total_size=0
fi
log_info "Backup completed for $target_name: $ts ($(human_size "${total_size:-0}") in $(human_duration "$duration"))"

View File

@@ -64,7 +64,7 @@ load_config() {
export BACKUP_MODE BWLIMIT RETENTION_COUNT
export LOG_LEVEL LOG_RETAIN NOTIFY_EMAIL NOTIFY_ON
export SMTP_HOST SMTP_PORT SMTP_USER SMTP_FROM SMTP_SECURITY
export SMTP_HOST SMTP_PORT SMTP_USER SMTP_PASSWORD SMTP_FROM SMTP_SECURITY
export SSH_TIMEOUT SSH_RETRIES RSYNC_EXTRA_OPTS DISK_USAGE_THRESHOLD
}
@@ -128,6 +128,11 @@ validate_config() {
((errors++)) || true
fi
if [[ -n "${DISK_USAGE_THRESHOLD:-}" ]] && [[ ! "$DISK_USAGE_THRESHOLD" =~ ^[0-9]+$ ]]; then
log_error "DISK_USAGE_THRESHOLD must be a non-negative integer (0-100), got: $DISK_USAGE_THRESHOLD"
((errors++)) || true
fi
# Validate RSYNC_EXTRA_OPTS characters (prevent flag injection)
if [[ -n "${RSYNC_EXTRA_OPTS:-}" ]] && [[ ! "$RSYNC_EXTRA_OPTS" =~ ^[a-zA-Z0-9\ ._=/,-]+$ ]]; then
log_error "RSYNC_EXTRA_OPTS contains invalid characters: $RSYNC_EXTRA_OPTS"

View File

@@ -293,7 +293,7 @@ get_target_remotes() {
# Return the disk usage percentage (integer, no %) for REMOTE_BASE.
# Returns 0 (unknown) on unsupported remote types.
remote_disk_usage_pct() {
local base="${REMOTE_BASE:-/}"
local base; base="$(shquote "${REMOTE_BASE:-/}")"
local df_line=""
case "${REMOTE_TYPE:-ssh}" in
ssh)
@@ -332,7 +332,7 @@ check_remote_disk_space() {
# Compact one-line disk info: "USED/TOTAL (FREE free) PCT"
remote_disk_info_short() {
local base="${REMOTE_BASE:-/}"
local base; base="$(shquote "${REMOTE_BASE:-/}")"
local df_out=""
case "${REMOTE_TYPE:-ssh}" in
ssh)

View File

@@ -64,7 +64,7 @@ test_ssh_connection() {
}
ensure_remote_dir() {
local dir="$1"
local dir; dir="$(shquote "$1")"
remote_exec "mkdir -p '$dir'" || {
log_error "Failed to create remote directory: $dir"
return 1
@@ -75,6 +75,6 @@ build_rsync_ssh_cmd() {
if _is_password_mode; then
echo "ssh -p $REMOTE_PORT -o StrictHostKeyChecking=yes -o ConnectTimeout=$SSH_TIMEOUT"
else
echo "ssh -i $REMOTE_KEY -p $REMOTE_PORT -o StrictHostKeyChecking=yes -o BatchMode=yes -o ConnectTimeout=$SSH_TIMEOUT"
echo "ssh -i \"$REMOTE_KEY\" -p $REMOTE_PORT -o StrictHostKeyChecking=yes -o BatchMode=yes -o ConnectTimeout=$SSH_TIMEOUT"
fi
}

View File

@@ -227,7 +227,9 @@ finalize_snapshot() {
return 1
}
else
remote_exec "mv '$snap_dir/${timestamp}.partial' '$snap_dir/$timestamp'" || {
local sq_partial; sq_partial="$(shquote "$snap_dir/${timestamp}.partial")"
local sq_final; sq_final="$(shquote "$snap_dir/$timestamp")"
remote_exec "mv '$sq_partial' '$sq_final'" || {
log_error "Failed to finalize snapshot for $target_name: $timestamp"
return 1
}

View File

@@ -10,6 +10,12 @@ die() {
exit "$code"
}
# Escape a string for safe use inside single quotes in shell commands.
# Usage: shquote "$var" → outputs the value with ' escaped to '\''
shquote() {
printf '%s' "$1" | sed "s/'/'\\\\''/g"
}
timestamp() {
date -u +"%Y-%m-%dT%H%M%S"
}

View File

@@ -232,7 +232,7 @@ class AppSettings:
rsync_extra_opts=data.get("RSYNC_EXTRA_OPTS", ""),
disk_usage_threshold=data.get("DISK_USAGE_THRESHOLD", "95"),
work_dir=data.get("WORK_DIR", "/usr/local/gniza/workdir"),
web_port=data.get("WEB_PORT", "8080"),
web_port=data.get("WEB_PORT", "2323"),
web_host=data.get("WEB_HOST", "0.0.0.0"),
web_api_key=data.get("WEB_API_KEY", ""),
)

View File

@@ -92,7 +92,8 @@ class RemotesScreen(Screen):
try:
table = self.query_one("#remotes-table", DataTable)
table.update_cell(name, self._disk_col_key, disk_text, update_width=True)
except Exception:
except (KeyError, LookupError):
# Row may have been removed if user navigated away and back
pass
@work

View File

@@ -104,7 +104,7 @@ class SettingsScreen(Screen):
rsync_extra_opts=self.query_one("#set-rsyncopts", Input).value.strip(),
disk_usage_threshold=self.query_one("#set-diskthreshold", Input).value.strip() or "95",
work_dir=self.query_one("#set-workdir", Input).value.strip() or "/usr/local/gniza/workdir",
web_port=self.query_one("#set-web-port", Input).value.strip() or "8080",
web_port=self.query_one("#set-web-port", Input).value.strip() or "2323",
web_host=self.query_one("#set-web-host", Input).value.strip() or "0.0.0.0",
web_api_key=self.query_one("#set-web-key", Input).value,
)