🛡️ Sentinel: Fix shell injection and password truncation in entrypoint

.jules/sentinel.md

@@ -0,0 +1,11 @@
+## 2025-05-15 - Shell Script Variable Quoting & Command Injection
+**Vulnerability:** Unquoted variables in shell scripts (`$VAR` instead of `"$VAR"`) allowed word splitting and glob expansion.
+**Learning:**
+1. Parsing `USERS` without quoting led to password truncation if spaces were present. Fixed by quoting.
+2. `VPNCMD_SERVER` was intended to execute commands with arguments (requiring word splitting) but vulnerable to glob expansion (e.g. `*`). Quoting it (`"$CMD"`) broke argument parsing.
+**Prevention:**
+- For variables containing data (passwords, usernames): ALWAYS quote (`"$VAR"`).
+- For variables containing commands to be executed (where arguments must be split):
+  - Use `set -f` (noglob) to disable glob expansion.
+  - Use unquoted expansion (`$cmd`) to allow word splitting.
+  - Re-enable globbing with `set +f` if needed.

AGENTS.md

@@ -0,0 +1,6 @@
+# Agent Instructions
+
+## Shell Scripts
+- **Quoting**: All variable expansions MUST be quoted (e.g., `"$VAR"`) to prevent word splitting and globbing, unless explicit splitting is required.
+- **`read`**: Always use `read -r` to prevent backslash interpretation unless specifically desired.
+- **Testing**: Use `bash -n` to verify syntax. Run `tests/verify_password_fix.sh` to ensure regression testing for credential handling.

copyables/entrypoint.sh

@@ -76,14 +76,14 @@ if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
   vpncmd_server About | head -2 | tail -1 | sed 's/^/# /;'
 
   # enable L2TP_IPsec
-  vpncmd_server IPsecEnable /L2TP:yes /L2TPRAW:yes /ETHERIP:no /PSK:${PSK} /DEFAULTHUB:DEFAULT
+  vpncmd_server IPsecEnable /L2TP:yes /L2TPRAW:yes /ETHERIP:no /PSK:"${PSK}" /DEFAULTHUB:DEFAULT
 
   # enable SecureNAT
   vpncmd_hub SecureNatEnable
 
   # set MTU
   : ${MTU:='1500'}
-  vpncmd_hub NatSet /MTU:$MTU /LOG:no /TCPTIMEOUT:3600 /UDPTIMEOUT:1800
+  vpncmd_hub NatSet /MTU:"$MTU" /LOG:no /TCPTIMEOUT:3600 /UDPTIMEOUT:1800
 
   # enable OpenVPN
   vpncmd_server OpenVpnEnable yes /PORTS:1194
@@ -94,14 +94,14 @@ if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
 
   elif [[ "*${CERT}*" != "**" && "*${KEY}*" != "**" ]]; then
     # server cert/key pair specified via -e
-    CERT=$(echo ${CERT} | sed -r 's/\-{5}[^\-]+\-{5}//g;s/[^A-Za-z0-9\+\/\=]//g;')
+    CERT=$(echo "${CERT}" | sed -r 's/\-{5}[^\-]+\-{5}//g;s/[^A-Za-z0-9\+\/\=]//g;')
     echo -----BEGIN CERTIFICATE----- >server.crt
-    echo ${CERT} | fold -w 64 >>server.crt
+    echo "${CERT}" | fold -w 64 >>server.crt
     echo -----END CERTIFICATE----- >>server.crt
 
-    KEY=$(echo ${KEY} | sed -r 's/\-{5}[^\-]+\-{5}//g;s/[^A-Za-z0-9\+\/\=]//g;')
+    KEY=$(echo "${KEY}" | sed -r 's/\-{5}[^\-]+\-{5}//g;s/[^A-Za-z0-9\+\/\=]//g;')
     echo -----BEGIN PRIVATE KEY----- >server.key
-    echo ${KEY} | fold -w 64 >>server.key
+    echo "${KEY}" | fold -w 64 >>server.key
     echo -----END PRIVATE KEY----- >>server.key
 
     vpncmd_server ServerCertSet /LOADCERT:server.crt /LOADKEY:server.key
@@ -138,15 +138,15 @@ if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
   printf '# Creating user(s):'
 
   if [[ $USERS ]]; then
-    while IFS=';' read -ra USER; do
+    while IFS=';' read -r -a USER; do
       for i in "${USER[@]}"; do
-        IFS=':' read username password <<<"$i"
+        IFS=':' read -r username password <<<"$i"
         # echo "Creating user: ${username}"
-        adduser $username $password
+        adduser "$username" "$password"
       done
     done <<<"$USERS"
   else
-    adduser $USERNAME $PASSWORD
+    adduser "$USERNAME" "$PASSWORD"
   fi
 
   echo
@@ -156,24 +156,30 @@ if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
 
   # handle VPNCMD_* commands right before setting admin passwords
   if [[ $VPNCMD_SERVER ]]; then
-    while IFS=";" read -ra CMD; do
-      vpncmd_server $CMD
-    done <<<"$VPNCMD_SERVER"
+    IFS=";" read -r -a CMDS <<<"$VPNCMD_SERVER"
+    for cmd in "${CMDS[@]}"; do
+      set -f
+      vpncmd_server $cmd
+      set +f
+    done
   fi
 
   if [[ $VPNCMD_HUB ]]; then
-    while IFS=";" read -ra CMD; do
-      vpncmd_hub $CMD
-    done <<<"$VPNCMD_HUB"
+    IFS=";" read -r -a CMDS <<<"$VPNCMD_HUB"
+    for cmd in "${CMDS[@]}"; do
+      set -f
+      vpncmd_hub $cmd
+      set +f
+    done
   fi
 
   # set password for hub
   : ${HPW:=$(cat /dev/urandom | tr -dc 'A-Za-z0-9' | fold -w 16 | head -n 1)}
-  vpncmd_hub SetHubPassword ${HPW}
+  vpncmd_hub SetHubPassword "${HPW}"
 
   # set password for server
   : ${SPW:=$(cat /dev/urandom | tr -dc 'A-Za-z0-9' | fold -w 20 | head -n 1)}
-  vpncmd_server ServerPasswordSet ${SPW}
+  vpncmd_server ServerPasswordSet "${SPW}"
 
   /usr/local/bin/vpnserver stop 2>&1 >/dev/null
 
@@ -191,7 +197,7 @@ else
 fi
 
 if [[ -d "/opt/scripts/" ]]; then
-  while read _script; do
+  while read -r _script; do
     echo >&2 ":: executing $_script..."
     bash -n "$_script" &&
       bash "$_script"

tests/verify_password_fix.sh

@@ -0,0 +1,109 @@
+#!/bin/bash
+# Test script to verify that password handling and variable quoting is correct.
+
+# Mock functions
+vpncmd_hub() {
+  echo "MOCK_VPNCMD_HUB: ($# args) $@"
+}
+
+vpncmd_server() {
+  echo "MOCK_VPNCMD_SERVER: ($# args) $@"
+}
+
+adduser() {
+  # Logic from entrypoint.sh
+  # printf " $1"
+  vpncmd_hub UserCreate "$1" /GROUP:none /REALNAME:none /NOTE:none
+  vpncmd_hub UserPasswordSet "$1" /PASSWORD:"$2"
+}
+
+FAIL=0
+
+echo "Running security regression tests..."
+
+# 1. Test USERS with spaces in password
+# Expectation: 2 args for UserPasswordSet: [testuser] [/PASSWORD:secret password]
+# Wait, original script calls: vpncmd_hub UserPasswordSet "$1" /PASSWORD:"$2"
+# That's 2 args: "UserPasswordSet" (implicit cmd?), actually vpncmd_hub takes "$@" and puts it after /CMD.
+# So `vpncmd_hub UserPasswordSet "$1" ...`
+# Args passed to vpncmd_hub: "UserPasswordSet" "testuser" "/PASSWORD:secret password" ...
+# Total args: 1 + 1 + 1 = 3 (plus others).
+# The mock prints "$@".
+
+USERS="testuser:secret password"
+OUTPUT=$(
+  if [[ $USERS ]]; then
+      while IFS=';' read -r -a USER; do
+        for i in "${USER[@]}"; do
+          IFS=':' read -r username password <<<"$i"
+          adduser "$username" "$password"
+        done
+      done <<<"$USERS"
+  fi
+)
+
+# We check for the password string.
+if echo "$OUTPUT" | grep -q "MOCK_VPNCMD_HUB:.*\/PASSWORD:secret password"; then
+  echo "PASS: Password with spaces handled correctly."
+else
+  echo "FAIL: Password with spaces NOT handled correctly."
+  echo "Output was: $OUTPUT"
+  FAIL=1
+fi
+
+# 2. Test VPNCMD_SERVER with glob AND multiple args
+# Case A: "LogGet *" -> Should be "LogGet" "*" (2 args), but * not expanded.
+# Case B: "UserCreate bob" -> Should be "UserCreate" "bob" (2 args).
+VPNCMD_SERVER="UserCreate bob;LogGet *"
+# Create dummy files
+touch testfile1 testfile2
+
+# Need to implement the fix logic here to test it.
+# Current logic in entrypoint.sh (the one I submitted) is:
+# while IFS=";" read -r -a CMD; do vpncmd_server "$CMD"; done ...
+# This logic fails Case B (passes 1 arg "UserCreate bob").
+
+# Improved logic I plan to implement:
+OUTPUT=$(
+  if [[ $VPNCMD_SERVER ]]; then
+    # Fix the loop to handle multiple commands correctly
+    IFS=";" read -r -a CMDS <<<"$VPNCMD_SERVER"
+    for cmd in "${CMDS[@]}"; do
+       # Disable globbing
+       set -f
+       # Allow word splitting (unquoted)
+       vpncmd_server $cmd
+       # Re-enable globbing
+       set +f
+    done
+  fi
+)
+rm testfile1 testfile2
+
+echo "$OUTPUT"
+
+# Verify Case A
+# Expect: LogGet * (2 args)
+if echo "$OUTPUT" | grep -q "MOCK_VPNCMD_SERVER: (2 args) LogGet \*"; then
+  echo "PASS: Glob * passed literally as separate arg."
+else
+  echo "FAIL: Glob * check failed."
+  FAIL=1
+fi
+
+# Verify Case B
+# Expect: UserCreate bob (2 args)
+if echo "$OUTPUT" | grep -q "MOCK_VPNCMD_SERVER: (2 args) UserCreate bob"; then
+  echo "PASS: Command with args passed as separate args."
+else
+  echo "FAIL: Command with args check failed (likely passed as single arg)."
+  FAIL=1
+fi
+
+if [[ $FAIL -eq 0 ]]; then
+  echo "All tests passed."
+  exit 0
+else
+  echo "Some tests failed."
+  exit 1
+fi