From 1e19f087766c6ff6150af053029fc423c59ca102 Mon Sep 17 00:00:00 2001
From: nevaforget <dom@moonarch.de>
Date: Tue, 31 Mar 2026 11:06:14 +0200
Subject: [PATCH] fix: shell script quoting and argument injection hardening

Audit fixes for command injection risks in helper scripts:
- moonarch-cpugov: eval for quoted COMMANDS expansion (pkexec context)
- moonarch-btnote: while+read with process substitution, quoted vars
- moonarch-vpn: -- guard before connection name in nmcli calls
- post-install.sh: else-logging when USER_DEFAULTS dir missing
---
 DECISIONS.md                 | 7 +++++++
 defaults/bin/moonarch-btnote | 9 +++++----
 defaults/bin/moonarch-cpugov | 5 +++--
 defaults/bin/moonarch-vpn    | 4 ++--
 scripts/post-install.sh      | 2 ++
 5 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/DECISIONS.md b/DECISIONS.md
index a0ffcfa..d45a710 100644
--- a/DECISIONS.md
+++ b/DECISIONS.md
@@ -1,5 +1,12 @@
 # Decisions
 
+## 2026-03-31 – Audit: shell script quoting fixes, PKGBUILD permissions
+
+- **Who**: Ragnar, Dom
+- **Why**: Security audit found command injection risk in moonarch-cpugov (unquoted array expansion with pkexec), word-splitting in moonarch-btnote (upower output from Bluetooth devices), and nmcli argument injection in moonarch-vpn. PKGBUILD for moongreet had world-readable cache dir.
+- **Tradeoffs**: `eval` in cpugov is safe because COMMANDS values are hardcoded string literals, not user input. Alternative (function dispatch) would be cleaner but over-engineered for 3 fixed entries. moonarch-btnote switched from for-loop to while+read with process substitution to avoid subshell.
+- **How**: (1) `eval "${COMMANDS[$choice]}"` in cpugov. (2) `while IFS= read -r` + process substitution + quoted `$DEVICE_DATA` in btnote. (3) `--` guard before `$connection` in vpn nmcli calls. (4) `install -dm700` for moongreet cache dirs in PKGBUILD. (5) `else err` logging in post-install.sh when USER_DEFAULTS missing.
+
 ## 2026-03-29 – /opt/moonarch stays root-owned, no chown to user
 - **Who**: Dominik, Ragnar
 - **Why**: Multi-user system — chown to UID 1000 locks out other users from moonarch-update
diff --git a/defaults/bin/moonarch-btnote b/defaults/bin/moonarch-btnote
index e94ea7c..b02f9f7 100755
--- a/defaults/bin/moonarch-btnote
+++ b/defaults/bin/moonarch-btnote
@@ -5,15 +5,16 @@
 NOTIFY_AT_PERCENTAGE=70
 ICON="battery-empty"
 
-for d in $(upower -e); do
+while IFS= read -r d; do
+  [ -z "$d" ] && continue
   DEVICE_DATA=$(upower -i "$d")
-  PERCENTAGE=$(echo $DEVICE_DATA | grep -Po '(?<=(percentage: )).*(?= icon)')
+  PERCENTAGE=$(echo "$DEVICE_DATA" | grep -Po '(?<=(percentage: )).*(?= icon)')
   PER_INT=$(echo "${PERCENTAGE//%}")
-  DEVICE_NAME=$(echo $DEVICE_DATA | grep -Po '(?<=(model: )).*(?= serial)')
+  DEVICE_NAME=$(echo "$DEVICE_DATA" | grep -Po '(?<=(model: )).*(?= serial)')
 
   if [ -n "$DEVICE_NAME" ] && [ -n "$PER_INT" ] && [ "$PER_INT" -lt "$NOTIFY_AT_PERCENTAGE" ]; then
     notify-send -t 5000 -e "Low battery $DEVICE_NAME $PER_INT%" -i "$ICON" \
       -h string:x-canonical-private-synchronous:battery \
       -h int:value:"$PER_INT" -u critical
   fi
-done
+done < <(upower -e)
diff --git a/defaults/bin/moonarch-cpugov b/defaults/bin/moonarch-cpugov
index 5e9dcc1..fd2bb54 100755
--- a/defaults/bin/moonarch-cpugov
+++ b/defaults/bin/moonarch-cpugov
@@ -60,8 +60,9 @@ fi
 # check if choice exists
 if test "${COMMANDS[$choice]+isset}"
 then
-    # Execute the choice
-    ${COMMANDS[$choice]}
+    # Execute the choice — eval required because COMMANDS values contain
+    # multi-word strings that must be interpreted as full commands.
+    eval "${COMMANDS[$choice]}"
 
     notify-send -h string:x-canonical-private-synchronous:cpugov -i cpu "CPU Mode" "Set to $choice ${LABELS[$choice]}"
 else
diff --git a/defaults/bin/moonarch-vpn b/defaults/bin/moonarch-vpn
index 5167966..8dfc156 100755
--- a/defaults/bin/moonarch-vpn
+++ b/defaults/bin/moonarch-vpn
@@ -41,7 +41,7 @@ function connect_vpn() {
     local connection="$1"
     local feedback
 
-    if feedback=$(nmcli connection up "$connection" 2>&1); then
+    if feedback=$(nmcli connection up -- "$connection" 2>&1); then
         notify-send "VPN" "Connected to '$connection'"
     else
         notify-send -u critical "VPN" "Connection failed: $feedback"
@@ -53,7 +53,7 @@ function disconnect_vpn() {
     local connection="$1"
     local feedback
 
-    if feedback=$(nmcli connection down "$connection" 2>&1); then
+    if feedback=$(nmcli connection down -- "$connection" 2>&1); then
         notify-send "VPN" "Disconnected from '$connection'"
     else
         notify-send -u critical "VPN" "Disconnect failed: $feedback"
diff --git a/scripts/post-install.sh b/scripts/post-install.sh
index 7746e24..f9b77ef 100755
--- a/scripts/post-install.sh
+++ b/scripts/post-install.sh
@@ -89,6 +89,8 @@ if [[ -d "$USER_DEFAULTS" ]]; then
             fi
         done
     done
+else
+    err "USER_DEFAULTS not found: $USER_DEFAULTS — skipping user config defaults."
 fi
 
 # --- Zsh user config ---