From 92a5d500823bce77025fbc9fe71d2e26a383f091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Tr=C3=B6ger?= Date: Sun, 19 Apr 2026 19:57:39 +0200 Subject: [PATCH] Sicherheitshaertung: Injection-Schutz, atomares Locking, Pfad-Validierung MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - load_config blockiert Shell-Operatoren (;|&`) in .env-Werten - validate_path prueft Sonderzeichen und Path-Traversal in Pfad-Variablen - validate_config prüft DARKTABLE_BIN-basename und ruft validate_path auf - Lockdir-Trap erst nach erfolgreicher Lock-Akquisition registriert (verhindert dass externer Lockdir bei gescheitertem Lock entfernt wird) - uninstall.sh nutzt rmdir statt rm -rf fuer Lockdir - security.bats mit 10 Tests fuer alle Sicherheitsanforderungen Co-Authored-By: Claude Sonnet 4.6 --- scripts/darktable_common.sh | 32 ++++++++ scripts/darktable_sync.sh | 14 ++-- tests/darktable_sync.bats | 4 +- tests/security.bats | 147 ++++++++++++++++++++++++++++++++++++ uninstall.sh | 10 +-- 5 files changed, 192 insertions(+), 15 deletions(-) create mode 100644 tests/security.bats diff --git a/scripts/darktable_common.sh b/scripts/darktable_common.sh index 5e670da..edea257 100644 --- a/scripts/darktable_common.sh +++ b/scripts/darktable_common.sh @@ -11,6 +11,20 @@ load_config() { echo "Vorlage kopieren mit: cp .env.example $env_file" >&2 exit 1 fi + + # Berechtigungen pruefen: .env darf nicht world-readable sein + local perms + perms=$(stat -c '%a' "$env_file" 2>/dev/null || stat -f '%A' "$env_file" 2>/dev/null) + if [[ "${perms: -1}" != "0" ]]; then + echo "Warnung: $env_file ist world-readable. Empfehlung: chmod 600 $env_file" >&2 + fi + + # Zeilen mit Shell-Operatoren abweisen (Kommentare und Leerzeilen ignorieren) + if grep -vE '^\s*#|^\s*$' "$env_file" | grep -qE '[;|&`]'; then + echo "Fehler: $env_file enthaelt unerlaubte Zeichen (; | & \`). Bitte pruefen." >&2 + exit 1 + fi + # shellcheck source=/dev/null . "$env_file" } @@ -23,6 +37,15 @@ require_var() { fi } +validate_path() { + local var_name="$1" value="${!1:-}" + # Pfade duerfen keine Shell-Sonderzeichen oder Path-Traversal enthalten + if echo "$value" | grep -qE "['\";|&\`\$()\\\\]" || [[ "$value" == *".."* ]]; then + echo "Fehler: '$var_name' enthaelt unerlaubte Zeichen: $value" >&2 + exit 1 + fi +} + validate_config() { require_var SERVER_IP require_var SERVER_USER @@ -33,6 +56,15 @@ validate_config() { require_var LOCAL_PHOTO_DIR require_var SYNC_BIN require_var DARKTABLE_BIN + + validate_path SERVER_DB_DIR + validate_path SERVER_PHOTO_DIR + + # DARKTABLE_BIN: basename muss 'darktable' sein + if [[ "$(basename "$DARKTABLE_BIN")" != "darktable" ]]; then + echo "Fehler: DARKTABLE_BIN muss auf 'darktable' zeigen, nicht auf '$(basename "$DARKTABLE_BIN")'." >&2 + exit 1 + fi } check_dependency() { diff --git a/scripts/darktable_sync.sh b/scripts/darktable_sync.sh index a10b9bd..1da50c0 100755 --- a/scripts/darktable_sync.sh +++ b/scripts/darktable_sync.sh @@ -24,17 +24,15 @@ count_synced_files() { echo "$count" } -SCRIPT_NAME="$(basename "$0")" -LOCKFILE="/tmp/${SCRIPT_NAME}.lock" +LOCKDIR="$CONFIG_DIR/sync.lock" +TMPFILES=() -if [ -e "$LOCKFILE" ]; then - echo "Script laeuft bereits oder Lockfile loeschen: $LOCKFILE" +# Atomares Locking per mkdir (verhindert Race Condition und Symlink-Attacken) +if ! mkdir "$LOCKDIR" 2>/dev/null; then + echo "Script laeuft bereits oder Lockdir loeschen: $LOCKDIR" exit 1 fi - -touch "$LOCKFILE" -TMPFILES=("$LOCKFILE") -trap 'rm -f "${TMPFILES[@]}"' EXIT +trap 'rm -f "${TMPFILES[@]}"; rmdir "$LOCKDIR" 2>/dev/null || true' EXIT SHOW_NOTIFY_START_STOP=false if [[ "${1:-}" == "--with-notify-start-stop" ]]; then diff --git a/tests/darktable_sync.bats b/tests/darktable_sync.bats index 93fc27b..2ed8be8 100644 --- a/tests/darktable_sync.bats +++ b/tests/darktable_sync.bats @@ -52,8 +52,8 @@ setup() { [ -f "$CONFIG_DIR/sync_pending" ] } -@test "Lockfile wird nach Abschluss entfernt" { +@test "Lockdir wird nach Abschluss entfernt" { run_with_stubs env SSH_STUB_FAIL=0 bash "$SYNC_SCRIPT" [ "$status" -eq 0 ] - [ ! -f "/tmp/darktable_sync.sh.lock" ] + [ ! -d "$CONFIG_DIR/sync.lock" ] } diff --git a/tests/security.bats b/tests/security.bats new file mode 100644 index 0000000..3b2f6bd --- /dev/null +++ b/tests/security.bats @@ -0,0 +1,147 @@ +#!/usr/bin/env bats +# Security-Tests fuer darktable-sync + +load helpers/setup + +COMMON_SCRIPT="$BATS_TEST_DIRNAME/../scripts/darktable_common.sh" +SYNC_SCRIPT="$BATS_TEST_DIRNAME/../scripts/darktable_sync.sh" +WRAPPER_SCRIPT="$BATS_TEST_DIRNAME/../scripts/darktable_wrapper.sh" + +setup() { + create_valid_env + mkdir -p "$HOME/.config/darktable" + touch "$HOME/.config/darktable/library.db" + touch "$HOME/.config/darktable/data.db" + mkdir -p "$HOME/Pictures" + export DISPLAY=:99 +} + +# --- K1: .env Code-Injection wird geblockt --- + +@test "security: .env mit Semikolon wird abgelehnt" { + cat > "$CONFIG_DIR/.env" <<'EOF' +SERVER_IP=192.168.1.100 +SERVER_USER=testuser +SERVER_SSH_PORT=22 +SERVER_DB_DIR=/remote/db +SERVER_PHOTO_DIR=/remote/photos +LOCAL_DARKTABLE_DB_DIR=/tmp/dt_test +LOCAL_PHOTO_DIR=/tmp/photos_test +DARKTABLE_BIN=darktable +SYNC_BIN=/usr/local/bin/darktable_sync.sh +INJECTION_MARKER=injected; touch /tmp/dt_security_test_marker +EOF + run bash -c "source '$COMMON_SCRIPT'; load_config; echo done" + rm -f /tmp/dt_security_test_marker + [ "$status" -eq 1 ] + [[ "$output" == *"unerlaubte Zeichen"* ]] +} + +@test "security: .env mit Backtick wird abgelehnt" { + cat > "$CONFIG_DIR/.env" <<'EOF' +SERVER_IP=192.168.1.100 +SERVER_USER=testuser +SERVER_SSH_PORT=22 +SERVER_DB_DIR=/remote/db +SERVER_PHOTO_DIR=/remote/photos +LOCAL_DARKTABLE_DB_DIR=/tmp/dt_test +LOCAL_PHOTO_DIR=/tmp/photos_test +DARKTABLE_BIN=darktable +SYNC_BIN=/usr/local/bin/darktable_sync.sh +EVIL=`touch /tmp/evil` +EOF + run bash -c "source '$COMMON_SCRIPT'; load_config; echo done" + [ "$status" -eq 1 ] + [[ "$output" == *"unerlaubte Zeichen"* ]] +} + +# --- K2: validate_path blockt SSH-Injection --- + +@test "security: SERVER_DB_DIR mit Single-Quote wird geblockt" { + create_valid_env + # Wert in Double-Quotes damit bash ihn fehlerfrei laedt, validate_path muss dann blockieren + printf 'SERVER_DB_DIR="/remote/db'"'"'injection"\n' >> "$CONFIG_DIR/.env" + run bash -c "source '$COMMON_SCRIPT'; load_config; validate_config" + [ "$status" -eq 1 ] + [[ "$output" == *"SERVER_DB_DIR"* ]] +} + +@test "security: SERVER_DB_DIR mit Path-Traversal wird geblockt" { + create_valid_env + echo 'SERVER_DB_DIR=/../../../etc' >> "$CONFIG_DIR/.env" + run bash -c "source '$COMMON_SCRIPT'; load_config; validate_config" + [ "$status" -eq 1 ] + [[ "$output" == *"SERVER_DB_DIR"* ]] +} + +# --- H1: Atomares Locking mit mkdir --- + +@test "security: gleichzeitiger Sync wird durch Lockdir geblockt" { + mkdir -p "$CONFIG_DIR/sync.lock" + run_with_stubs env SSH_STUB_FAIL=0 bash "$SYNC_SCRIPT" + [ "$status" -eq 1 ] + [[ "$output" == *"laeuft bereits"* ]] + rmdir "$CONFIG_DIR/sync.lock" +} + +# --- H2: Lockdir nicht durch Symlink angreifbar --- + +@test "security: Lockdir ist kein Symlink-Angriffspunkt" { + # mkdir schlaegt bei existierendem Symlink fehl – kein Ziel wird geloescht + TARGET="$BATS_TMPDIR/symlink_target" + echo "wichtiger Inhalt" > "$TARGET" + ln -sf "$TARGET" "$CONFIG_DIR/sync.lock" + + run_with_stubs env SSH_STUB_FAIL=0 bash "$SYNC_SCRIPT" + # Script muss fehlschlagen (Symlink statt echtes Verzeichnis = mkdir schlaegt fehl) + [ "$status" -eq 1 ] + # Zieldatei darf nicht geloescht worden sein + [ -f "$TARGET" ] + rm -f "$CONFIG_DIR/sync.lock" "$TARGET" +} + +# --- H3: DARKTABLE_BIN muss 'darktable' sein --- + +@test "security: DARKTABLE_BIN mit anderem basename wird geblockt" { + create_valid_env + echo "DARKTABLE_BIN=/usr/bin/evil_binary" >> "$CONFIG_DIR/.env" + run bash -c "source '$COMMON_SCRIPT'; load_config; validate_config" + [ "$status" -eq 1 ] + [[ "$output" == *"DARKTABLE_BIN"* ]] +} + +# --- M2: .env-Berechtigungen werden gewarnt --- + +@test "security: .env mit world-readable Berechtigungen loest Warnung aus" { + create_valid_env + chmod 644 "$CONFIG_DIR/.env" + run bash -c "source '$COMMON_SCRIPT'; load_config; echo \$SERVER_IP" + [ "$status" -eq 0 ] + [[ "$output" == *"world-readable"* ]] +} + +# --- validate_config: fehlende Variablen --- + +@test "security: validate_config blockt leere SERVER_IP" { + create_valid_env + echo "SERVER_IP=" >> "$CONFIG_DIR/.env" + run bash -c "source '$COMMON_SCRIPT'; load_config; validate_config" + [ "$status" -eq 1 ] + [[ "$output" == *"SERVER_IP"* ]] +} + +@test "security: validate_config blockt fehlende SERVER_DB_DIR" { + create_valid_env + sed -i '/^SERVER_DB_DIR/d' "$CONFIG_DIR/.env" + run bash -c "source '$COMMON_SCRIPT'; load_config; validate_config" + [ "$status" -eq 1 ] + [[ "$output" == *"SERVER_DB_DIR"* ]] +} + +# --- Lockdir Cleanup --- + +@test "security: Lockdir wird bei normalem Exit entfernt" { + run_with_stubs env SSH_STUB_FAIL=0 bash "$SYNC_SCRIPT" + [ "$status" -eq 0 ] + [ ! -d "$CONFIG_DIR/sync.lock" ] +} diff --git a/uninstall.sh b/uninstall.sh index 59924b2..0a9ce83 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -19,12 +19,12 @@ systemctl --user disable --now darktable-sync.timer 2>/dev/null || true systemctl --user disable --now darktable_sync.timer 2>/dev/null || true systemctl --user daemon-reload -### Lockfile entfernen +### Lockdir entfernen (atomares Lock) -LOCKFILE="/tmp/darktable_sync.sh.lock" -if [ -f "$LOCKFILE" ]; then - echo "Lockfile entfernen: $LOCKFILE" - rm -f "$LOCKFILE" +LOCKDIR="$CONFIG_DIR/sync.lock" +if [ -d "$LOCKDIR" ]; then + echo "Lockdir entfernen: $LOCKDIR" + rmdir "$LOCKDIR" 2>/dev/null || true fi ### Aktiven Marker auf Server entfernen (best-effort)