Skip to content

🛡️ Sentinel: [HIGH] Fix shell injection and unquoted variables #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bluPhy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

🛡️ Sentinel: [HIGH] Fix shell injection and unquoted variables #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2025-05-18 - Shell Script Injection Risks via Unquoted Variables
**Vulnerability:** Unquoted variables in `entrypoint.sh` allowed argument splitting (passwords with spaces) and glob expansion (passwords with wildcards matching filenames).
**Learning:** Shell scripts are vulnerable to implicit expansion. `password="secret *"` expanded to filenames if unquoted, potentially exposing file existence or leaking data if passed to a command that prints arguments.
**Prevention:** Always quote variables (`"$VAR"`). Use `read -r` to disable backslash interpretation. Use `printf " %s" "$VAR"` to avoid format string injection.
30 changes: 15 additions & 15 deletions copyables/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,21 @@ set -e

CONFIG=/var/lib/softether/vpn_server.config

if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
if [ ! -f "$CONFIG" ] || [ ! -s "$CONFIG" ]; then
# Generate a random PSK if not provided
: ${PSK:=$(cat /dev/urandom | tr -dc 'A-Za-z0-9' | fold -w 20 | head -n 1)}

printf '# '
printf '=%.0s' {1..24}
echo

if [[ $USERS ]]; then
if [[ "$USERS" ]]; then
echo '# <use the password specified at -e USERS>'
else
: ${USERNAME:=user$(cat /dev/urandom | tr -dc '0-9' | fold -w 4 | head -n 1)}
echo \# ${USERNAME}

if [[ $PASSWORD ]]; then
if [[ "$PASSWORD" ]]; then
echo '# <use the password specified at -e PASSWORD>'
else
PASSWORD=$(cat /dev/urandom | tr -dc '0-9' | fold -w 20 | head -n 1 | sed 's/.\{4\}/&./g;s/.$//;')
Expand Down Expand Up @@ -130,23 +130,23 @@ if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
# add user

adduser() {
printf " $1"
printf " %s" "$1"
vpncmd_hub UserCreate "$1" /GROUP:none /REALNAME:none /NOTE:none
vpncmd_hub UserPasswordSet "$1" /PASSWORD:"$2"
}

printf '# Creating user(s):'

if [[ $USERS ]]; then
while IFS=';' read -ra USER; do
if [[ "$USERS" ]]; then
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
Expand All @@ -155,15 +155,15 @@ if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then
export PASSWORD='**'

# handle VPNCMD_* commands right before setting admin passwords
if [[ $VPNCMD_SERVER ]]; then
while IFS=";" read -ra CMD; do
vpncmd_server $CMD
if [[ "$VPNCMD_SERVER" ]]; then
while IFS=";" read -r -a CMD; do
vpncmd_server "$CMD"
done <<<"$VPNCMD_SERVER"
Comment on lines +159 to 161
Copy link

@gemini-code-assist gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The fix for processing VPNCMD_SERVER commands is incorrect and introduces a regression.

  1. Array Handling: CMD is an array created by read -a. In bash, "$CMD" only expands to the first element of the array ("${CMD[0]}").
  2. Argument Splitting: By quoting "$CMD", the shell passes the entire command string (including arguments) as a single argument to the vpncmd_server function. Since vpncmd expects the command and its arguments as separate parameters, this will cause commands with arguments to fail.
  3. Logic Error: The read -a command with IFS=";" splits the entire input string into the array. The while loop only executes once because read consumes the whole line. Since only the first element of the array is used, all subsequent commands in the semicolon-separated list are ignored.

This breaks the custom configuration functionality and can lead to a failure to apply important security settings.

Suggested change
while IFS=";" read -r -a CMD; do
vpncmd_server "$CMD"
done <<<"$VPNCMD_SERVER"
while IFS=";" read -r -a CMDS; do
for cmd in "${CMDS[@]}"; do
vpncmd_server $cmd
done
done <<<"$VPNCMD_SERVER"

fi

if [[ $VPNCMD_HUB ]]; then
while IFS=";" read -ra CMD; do
vpncmd_hub $CMD
if [[ "$VPNCMD_HUB" ]]; then
while IFS=";" read -r -a CMD; do
vpncmd_hub "$CMD"
done <<<"$VPNCMD_HUB"
Comment on lines +165 to 167
Copy link

@gemini-code-assist gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Same issue as with VPNCMD_SERVER. The current implementation only executes the first command and fails if it has arguments.

Suggested change
while IFS=";" read -r -a CMD; do
vpncmd_hub "$CMD"
done <<<"$VPNCMD_HUB"
while IFS=";" read -r -a CMDS; do
for cmd in "${CMDS[@]}"; do
vpncmd_hub $cmd
done
done <<<"$VPNCMD_HUB"

fi
Comment on lines +158 to 168
Copy link

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Bug: "$CMD" only expands to the first array element — all commands after the first semicolon are silently dropped.

read -r -a CMD splits the input by IFS=";" into an array, but "$CMD" (without an index) is equivalent to "${CMD[0]}". This means only the first semicolon-separated command is executed; the rest are silently ignored. ShellCheck SC2128 flags this exact issue.

Compare with the USERS block (lines 141–146) which correctly uses an inner for loop to iterate all array elements. Apply the same pattern here:

Proposed fix 
   if [[ "$VPNCMD_SERVER" ]]; then
     while IFS=";" read -r -a CMD; do
-      vpncmd_server "$CMD"
+      for c in "${CMD[@]}"; do
+        vpncmd_server "$c"
+      done
     done <<<"$VPNCMD_SERVER"
   fi
 
   if [[ "$VPNCMD_HUB" ]]; then
     while IFS=";" read -r -a CMD; do
-      vpncmd_hub "$CMD"
+      for c in "${CMD[@]}"; do
+        vpncmd_hub "$c"
+      done
     done <<<"$VPNCMD_HUB"
   fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [[ "$VPNCMD_SERVER" ]]; then
while IFS=";" read -r -a CMD; do
vpncmd_server "$CMD"
done <<<"$VPNCMD_SERVER"
fi
if [[ $VPNCMD_HUB ]]; then
while IFS=";" read -ra CMD; do
vpncmd_hub $CMD
if [[ "$VPNCMD_HUB" ]]; then
while IFS=";" read -r -a CMD; do
vpncmd_hub "$CMD"
done <<<"$VPNCMD_HUB"
fi
if [[ "$VPNCMD_SERVER" ]]; then
while IFS=";" read -r -a CMD; do
for c in "${CMD[@]}"; do
vpncmd_server "$c"
done
done <<<"$VPNCMD_SERVER"
fi
if [[ "$VPNCMD_HUB" ]]; then
while IFS=";" read -r -a CMD; do
for c in "${CMD[@]}"; do
vpncmd_hub "$c"
done
done <<<"$VPNCMD_HUB"
fi
🧰 Tools
🪛 Shellcheck (0.11.0)

[warning] 160-160: Expanding an array without an index only gives the first element.

(SC2128)

[warning] 166-166: Expanding an array without an index only gives the first element.

(SC2128)

🤖 Prompt for AI Agents 
In `@copyables/entrypoint.sh` around lines 158 - 168, The loops reading
VPNCMD_SERVER and VPNCMD_HUB currently use read -a CMD but call
vpncmd_server/vpncmd_hub with "$CMD" which expands to only ${CMD[0]}; change
each while-read block to iterate the array elements (use "${CMD[@]}" or an inner
for-loop) and call vpncmd_server or vpncmd_hub once per element so every
semicolon-separated command is executed (refer to VPNCMD_SERVER, VPNCMD_HUB, the
CMD array, and functions vpncmd_server/vpncmd_hub).


Expand Down
8 changes: 4 additions & 4 deletions copyables/gencert.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
#!/bin/bash
set -e

/usr/bin/vpnserver start 2>&1 >/dev/null
/usr/local/bin/vpnserver start 2>&1 >/dev/null

# while-loop to wait until server comes up
# switch cipher
while :; do
set +e
/usr/bin/vpncmd localhost /SERVER /CSV /CMD OpenVpnEnable yes /PORTS:1194 2>&1 >/dev/null
/usr/local/bin/vpncmd localhost /SERVER /CSV /CMD OpenVpnEnable yes /PORTS:1194 2>&1 >/dev/null
[[ $? -eq 0 ]] && break
set -e
sleep 1
done

/usr/bin/vpncmd localhost /SERVER /CSV /CMD ServerCertGet cert
/usr/bin/vpncmd localhost /SERVER /CSV /CMD ServerKeyGet key
/usr/local/bin/vpncmd localhost /SERVER /CSV /CMD ServerCertGet cert
/usr/local/bin/vpncmd localhost /SERVER /CSV /CMD ServerKeyGet key

CERT=$(cat cert | sed -r 's/\-{5}[^\-]+\-{5}//g;s/[^A-Za-z0-9\+\/\=]//g;' | tr -d '\r\n')
KEY=$(cat key | sed -r 's/\-{5}[^\-]+\-{5}//g;s/[^A-Za-z0-9\+\/\=]//g;' | tr -d '\r\n')
Expand Down