diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..08fab89 --- /dev/null +++ b/.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. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..8aea4d9 --- /dev/null +++ b/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. diff --git a/copyables/entrypoint.sh b/copyables/entrypoint.sh index 0d224a0..0ac8822 100644 --- a/copyables/entrypoint.sh +++ b/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" diff --git a/tests/verify_password_fix.sh b/tests/verify_password_fix.sh new file mode 100644 index 0000000..896072b --- /dev/null +++ b/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