🛡️ Sentinel: Fix injection risks and logic bugs in entrypoint.sh#35
🛡️ Sentinel: Fix injection risks and logic bugs in entrypoint.sh#35
Conversation
- Quote variables in `adduser` to prevent globbing and argument splitting. - Use `printf " %s"` to prevent format string vulnerabilities. - Fix `VPNCMD_*` loop logic to correctly iterate over semicolon-separated commands. - Use `set -f` to prevent globbing when executing dynamic commands. - Use `read -r` to preserve backslashes in passwords. - Create .jules/sentinel.md for security learnings. Co-authored-by: bluPhy <11618798+bluPhy@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughA new security documentation file is added to document shell variable expansion vulnerabilities, and an entrypoint shell script is hardened by improving string quoting, input parsing with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @bluPhy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers critical security enhancements and bug fixes to the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses several high-severity security risks, including argument injection, globbing, and format string vulnerabilities, and fixes a logic bug in command processing. However, the refactored loops for processing VPNCMD_SERVER and VPNCMD_HUB introduce new logic bugs that prevent the use of arguments with spaces and quotes. Additionally, these sensitive environment variables are not cleared after use, leading to potential information exposure. There are also opportunities to improve script performance by using shell built-ins instead of forking external processes in a loop.
| cmd=$(echo "$cmd" | xargs) | ||
| if [[ -n "$cmd" ]]; then | ||
| set -f | ||
| vpncmd_server $cmd |
There was a problem hiding this comment.
The current use of echo | xargs to trim whitespace followed by unquoted expansion of $cmd introduces a logic bug, breaking the ability to pass arguments with spaces and quotes. xargs strips quotes and backslashes, and unquoted expansion performs word splitting, causing commands like UserCreate "my user" to fail. Additionally, echo "$cmd" can misinterpret commands starting with a hyphen. For improved robustness and performance, consider using a shell built-in like read for trimming whitespace to avoid forking new processes, or use xargs to execute the command directly, which correctly handles quoted arguments and whitespace.
| cmd=$(echo "$cmd" | xargs) | |
| if [[ -n "$cmd" ]]; then | |
| set -f | |
| vpncmd_server $cmd | |
| read -r cmd <<< "$cmd" |
| if [[ $VPNCMD_SERVER ]]; then | ||
| while IFS=";" read -ra CMD; do | ||
| vpncmd_server $CMD | ||
| done <<<"$VPNCMD_SERVER" | ||
| IFS=";" read -ra COMMANDS <<<"$VPNCMD_SERVER" | ||
| for cmd in "${COMMANDS[@]}"; do | ||
| # trim whitespace | ||
| cmd=$(echo "$cmd" | xargs) | ||
| if [[ -n "$cmd" ]]; then | ||
| set -f | ||
| vpncmd_server $cmd | ||
| set +f | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| if [[ $VPNCMD_HUB ]]; then | ||
| while IFS=";" read -ra CMD; do | ||
| vpncmd_hub $CMD | ||
| done <<<"$VPNCMD_HUB" | ||
| IFS=";" read -ra COMMANDS <<<"$VPNCMD_HUB" | ||
| for cmd in "${COMMANDS[@]}"; do | ||
| # trim whitespace | ||
| cmd=$(echo "$cmd" | xargs) | ||
| if [[ -n "$cmd" ]]; then | ||
| set -f | ||
| vpncmd_hub $cmd | ||
| set +f | ||
| fi | ||
| done | ||
| fi |
There was a problem hiding this comment.
The environment variables VPNCMD_SERVER and VPNCMD_HUB may contain sensitive management commands and passwords. These are not cleared after use, allowing subsequent processes to read them. They should be masked after the loops finish executing to prevent information exposure. While the provided suggestion addresses the masking, consider optimizing the echo | xargs pipeline for trimming whitespace within these loops for better performance by using shell built-ins instead of forking new processes.
if [[ $VPNCMD_SERVER ]]; then
IFS=";" read -ra COMMANDS <<<"$VPNCMD_SERVER"
for cmd in "${COMMANDS[@]}"; do
# trim whitespace
cmd=$(echo "$cmd" | xargs)
if [[ -n "$cmd" ]]; then
set -f
vpncmd_server $cmd
set +f
fi
done
export VPNCMD_SERVER='**'
fi
if [[ $VPNCMD_HUB ]]; then
IFS=";" read -ra COMMANDS <<<"$VPNCMD_HUB"
for cmd in "${COMMANDS[@]}"; do
# trim whitespace
cmd=$(echo "$cmd" | xargs)
if [[ -n "$cmd" ]]; then
set -f
vpncmd_hub $cmd
set +f
fi
done
export VPNCMD_HUB='**'
fi![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.jules/sentinel.md:
- Around line 1-4: Update the date in the header "## 2025-02-02 - Unquoted
Variable Expansion in Entrypoint Scripts" to the correct date for this PR (e.g.,
"## 2026-02-02 - Unquoted Variable Expansion in Entrypoint Scripts") or confirm
and document if the original 2025-02-02 is intentional; ensure the header line
is the only change and keep the rest of the note unchanged.
In `@copyables/entrypoint.sh`:
- Around line 159-168: The loops reading VPNCMD_SERVER (and the similar
VPNCMD_CLIENT loop) currently use cmd=$(echo "$cmd" | xargs) which strips
quotes/backslashes and breaks quoted arguments; replace the xargs trimming with
POSIX/Bash parameter-expansion trimming to preserve quotes/backslashes, e.g.
trim leading/trailing whitespace via cmd="${cmd#"${cmd%%[![:space:]]*}"}";
cmd="${cmd%"${cmd##*[![:space:]]}"}", and then call vpncmd_server (and other
vpncmd_* functions) with the trimmed value quoted like vpncmd_server "$cmd"
(keep set -f/set +f as needed to disable globbing).
| ## 2025-02-02 - Unquoted Variable Expansion in Entrypoint Scripts | ||
| **Vulnerability:** Command injection and argument hijacking via unquoted variables in shell scripts. | ||
| **Learning:** Shell variables passed to functions or commands without quotes are subject to word splitting and globbing. In `entrypoint.sh`, `adduser $username $password` allowed a password containing globs (e.g., `*`) to expand to filenames, or spaces to shift arguments. | ||
| **Prevention:** Always quote variable expansions (`"$var"`). For dynamic commands where word splitting is desired but globbing is not, use `set -f` before execution and `set +f` after. |
There was a problem hiding this comment.
Confirm the date in the security note.
Line 1 records 2025-02-02, but this PR is dated 2026-02-02; please verify the intended date for the entry.
🤖 Prompt for AI Agents
In @.jules/sentinel.md around lines 1 - 4, Update the date in the header "##
2025-02-02 - Unquoted Variable Expansion in Entrypoint Scripts" to the correct
date for this PR (e.g., "## 2026-02-02 - Unquoted Variable Expansion in
Entrypoint Scripts") or confirm and document if the original 2025-02-02 is
intentional; ensure the header line is the only change and keep the rest of the
note unchanged.
| IFS=";" read -ra COMMANDS <<<"$VPNCMD_SERVER" | ||
| for cmd in "${COMMANDS[@]}"; do | ||
| # trim whitespace | ||
| cmd=$(echo "$cmd" | xargs) | ||
| if [[ -n "$cmd" ]]; then | ||
| set -f | ||
| vpncmd_server $cmd | ||
| set +f | ||
| fi | ||
| done |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the entrypoint.sh file
find . -name "entrypoint.sh" -type fRepository: bluPhy/docker-softether
Length of output: 92
🏁 Script executed:
# Check the lines mentioned in the review (159-168 and 172-181)
sed -n '150,190p' copyables/entrypoint.shRepository: bluPhy/docker-softether
Length of output: 1041
🏁 Script executed:
# Run the repro script from the review to verify xargs behavior
bash <<'EOF'
#!/bin/bash
cmd='UserPasswordSet /PASSWORD:"a b"'
trimmed="$(echo "$cmd" | xargs)"
echo "xargs: $trimmed"
set -f
set -- $trimmed
printf 'args(%d):\n' "$#"
printf ' - %s\n' "$@"
EOFRepository: bluPhy/docker-softether
Length of output: 151
🏁 Script executed:
# Test the proposed sed fix
bash <<'EOF'
#!/bin/bash
cmd='UserPasswordSet /PASSWORD:"a b"'
trimmed="$(sed -e 's/^[[:space:]]*//; s/[[:space:]]*$//' <<<"$cmd")"
echo "sed: $trimmed"
set -f
set -- $trimmed
printf 'args(%d):\n' "$#"
printf ' - %s\n' "$@"
EOFRepository: bluPhy/docker-softether
Length of output: 153
xargs trimming removes quotes/backslashes from VPNCMD commands, breaking quoted arguments.
Lines 159-168 and 172-181 use xargs for whitespace trimming, which strips quotes and performs word splitting. Commands with quoted arguments (e.g., /PASSWORD:"a b") lose their quotes, resulting in malformed arguments passed to vpncmd_* functions.
🔧 Suggested fix (preserve quotes/backslashes while trimming)
- for cmd in "${COMMANDS[@]}"; do
- # trim whitespace
- cmd=$(echo "$cmd" | xargs)
+ for cmd in "${COMMANDS[@]}"; do
+ # trim whitespace without stripping quotes/backslashes
+ cmd="$(sed -e 's/^[[:space:]]*//; s/[[:space:]]*$//' <<<"$cmd")"
if [[ -n "$cmd" ]]; then
set -f
vpncmd_server $cmd
set +f
fi
done
@@
- for cmd in "${COMMANDS[@]}"; do
- # trim whitespace
- cmd=$(echo "$cmd" | xargs)
+ for cmd in "${COMMANDS[@]}"; do
+ # trim whitespace without stripping quotes/backslashes
+ cmd="$(sed -e 's/^[[:space:]]*//; s/[[:space:]]*$//' <<<"$cmd")"
if [[ -n "$cmd" ]]; then
set -f
vpncmd_hub $cmd
set +f
fi
done📝 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.
| IFS=";" read -ra COMMANDS <<<"$VPNCMD_SERVER" | |
| for cmd in "${COMMANDS[@]}"; do | |
| # trim whitespace | |
| cmd=$(echo "$cmd" | xargs) | |
| if [[ -n "$cmd" ]]; then | |
| set -f | |
| vpncmd_server $cmd | |
| set +f | |
| fi | |
| done | |
| IFS=";" read -ra COMMANDS <<<"$VPNCMD_SERVER" | |
| for cmd in "${COMMANDS[@]}"; do | |
| # trim whitespace without stripping quotes/backslashes | |
| cmd="$(sed -e 's/^[[:space:]]*//; s/[[:space:]]*$//' <<<"$cmd")" | |
| if [[ -n "$cmd" ]]; then | |
| set -f | |
| vpncmd_server $cmd | |
| set +f | |
| fi | |
| done |
🤖 Prompt for AI Agents
In `@copyables/entrypoint.sh` around lines 159 - 168, The loops reading
VPNCMD_SERVER (and the similar VPNCMD_CLIENT loop) currently use cmd=$(echo
"$cmd" | xargs) which strips quotes/backslashes and breaks quoted arguments;
replace the xargs trimming with POSIX/Bash parameter-expansion trimming to
preserve quotes/backslashes, e.g. trim leading/trailing whitespace via
cmd="${cmd#"${cmd%%[![:space:]]*}"}"; cmd="${cmd%"${cmd##*[![:space:]]}"}", and
then call vpncmd_server (and other vpncmd_* functions) with the trimmed value
quoted like vpncmd_server "$cmd" (keep set -f/set +f as needed to disable
globbing).
1 participant
🛡️ Sentinel Security Fixes
Severity: HIGH
Vulnerability:
adduserandVPNCMD_*execution allowed shell globbing. A password like*could expand to filenames, leaking information or causing unexpected behavior.VPNCMD_SERVERandVPNCMD_HUBwas broken. It only executed the first command in the list, ignoring subsequent commands.printf " $1"is theoretically vulnerable to format string attacks if$1contains%.Fix:
adduser.VPNCMD_*loops to correctly split by semicolon and iterate.set -f(disable globbing) +set +faround dynamic command execution to allow word splitting (for arguments) but prevent wildcard expansion.printf " %s" "$1"for safe printing.read -rto preserve backslashes in inputs.Verification:
Verified with a test script simulating
addusercalls andVPNCMDloop processing, confirming correct argument handling and glob prevention. Ranbash -nto verify syntax.PR created automatically by Jules for task 1117922649488346055 started by @bluPhy
Summary by CodeRabbit