🛡️ Sentinel: [HIGH] Fix shell quoting and command processing vulnerabilities #31
Conversation
…ilities - Fix password truncation by quoting variables in adduser - Prevent backslash interpretation with read -r - Fix logic error to execute all VPNCMD_* commands - Disable globbing during command processing to prevent injection 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 documentation file detailing Bash scripting vulnerabilities is added, and an entrypoint script is refactored to improve input parsing security. Changes include using quoted variables, the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 addresses several high-severity shell scripting vulnerabilities within the Sentinel system's initialization process. The changes ensure robust handling of user inputs, particularly passwords, and correct execution of administrative commands, significantly improving the system's security posture and operational reliability by preventing data corruption and unintended command behavior. 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 critical security vulnerabilities related to shell quoting and command processing in entrypoint.sh. The fixes for unquoted variables in adduser, using read -r, and correcting the command processing loop logic are well-implemented. However, the new implementation for handling VPNCMD_* commands, while correctly preventing globbing, introduces a flaw where command arguments containing spaces will be parsed incorrectly. I've provided suggestions to use xargs for robust, quote-aware parsing of these command strings, which is safer than using eval.
| for cmd in "${CMDS[@]}"; do | ||
| vpncmd_server $cmd | ||
| done |
There was a problem hiding this comment.
While set -f correctly prevents globbing, the unquoted expansion of $cmd performs word splitting that doesn't respect quotes within the command string. This will cause arguments containing spaces to be split incorrectly. For example, a command like UserSetPassword user "new password" would be broken. Using xargs can parse the command string correctly and handle quoted arguments, providing a safer alternative to eval.
| for cmd in "${CMDS[@]}"; do | |
| vpncmd_server $cmd | |
| done | |
| for cmd in "${CMDS[@]}"; do | |
| # Use xargs to correctly parse the command string, respecting quoted arguments. | |
| # This is safer than `eval` as it doesn't perform command substitution. | |
| # The vpncmd_server function is inlined here to be callable from xargs. | |
| echo "$cmd" | xargs --no-run-if-empty /usr/local/bin/vpncmd localhost /SERVER /CSV /CMD | |
| done |
| for cmd in "${CMDS[@]}"; do | ||
| vpncmd_hub $cmd | ||
| done |
There was a problem hiding this comment.
Similar to the VPNCMD_SERVER loop, the unquoted expansion of $cmd here will incorrectly split arguments that contain spaces. Using xargs provides a robust way to parse the command string while respecting quotes.
| for cmd in "${CMDS[@]}"; do | |
| vpncmd_hub $cmd | |
| done | |
| for cmd in "${CMDS[@]}"; do | |
| # Use xargs to correctly parse the command string, respecting quoted arguments. | |
| # This is safer than `eval` as it doesn't perform command substitution. | |
| # The vpncmd_hub function is inlined here to be callable from xargs. | |
| echo "$cmd" | xargs --no-run-if-empty /usr/local/bin/vpncmd localhost /SERVER /CSV /HUB:DEFAULT /CMD | |
| done |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@copyables/entrypoint.sh`:
- Around line 159-164: The loop that splits VPNCMD_SERVER into CMDS uses IFS=";"
read -ra CMDS <<<"$VPNCMD_SERVER" which omits the -r flag and thus allows
backslashes to be processed as escape sequences; update the read usage (both
here and in the similar block handling VPNCMD_* arrays) to use read -r -a (e.g.,
read -r -a CMDS) so backslashes are preserved in command arguments passed to
vpncmd_server, leaving IFS and the rest of the loop intact.
| set -f | ||
| IFS=";" read -ra CMDS <<<"$VPNCMD_SERVER" | ||
| for cmd in "${CMDS[@]}"; do | ||
| vpncmd_server $cmd | ||
| done | ||
| set +f |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the entrypoint.sh file
find . -name "entrypoint.sh" -type f | head -20Repository: bluPhy/docker-softether
Length of output: 92
🏁 Script executed:
# If found, let's read the relevant lines from the file
# Read around lines 159-164 and 168-173
if [ -f "copyables/entrypoint.sh" ]; then
echo "=== Lines 155-175 of copyables/entrypoint.sh ==="
sed -n '155,175p' copyables/entrypoint.sh | cat -n
fiRepository: bluPhy/docker-softether
Length of output: 687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
VPNCMD_SERVER='CmdA /ARG:pa\\ss;CmdB'
echo "Testing read behavior with and without -r flag:"
echo "Input: $VPNCMD_SERVER"
echo ""
IFS=";" read -a CMDS <<<"$VPNCMD_SERVER"
printf 'without -r: %q\n' "${CMDS[@]}"
IFS=";" read -r -a CMDS <<<"$VPNCMD_SERVER"
printf 'with -r: %q\n' "${CMDS[@]}"Repository: bluPhy/docker-softether
Length of output: 234
Add -r to VPNCMD array reads to preserve backslashes in command arguments.
Lines 160 and 169 use read -a without -r, causing backslashes in VPNCMD_* commands to be interpreted as escape sequences. Without the -r flag, double backslashes are consumed and become single backslashes, which alters the intended command arguments.
Proposed fix
- IFS=";" read -ra CMDS <<<"$VPNCMD_SERVER"
+ IFS=";" read -r -a CMDS <<<"$VPNCMD_SERVER"- IFS=";" read -ra CMDS <<<"$VPNCMD_HUB"
+ IFS=";" read -r -a CMDS <<<"$VPNCMD_HUB"Also applies to: 168-173
🤖 Prompt for AI Agents
In `@copyables/entrypoint.sh` around lines 159 - 164, The loop that splits
VPNCMD_SERVER into CMDS uses IFS=";" read -ra CMDS <<<"$VPNCMD_SERVER" which
omits the -r flag and thus allows backslashes to be processed as escape
sequences; update the read usage (both here and in the similar block handling
VPNCMD_* arrays) to use read -r -a (e.g., read -r -a CMDS) so backslashes are
preserved in command arguments passed to vpncmd_server, leaving IFS and the rest
of the loop intact.
🛡️ Sentinel: [HIGH] Fix shell quoting and command processing vulnerabilities
🚨 Severity: HIGH
💡 Vulnerability:
adduserloop caused password truncation if passwords contained spaces or glob characters.readwithout-rflag interpreted backslashes in passwords and commands, causing data corruption.VPNCMD_*environment variable processing caused only the first command to be executed.VPNCMD_*commands allowed shell globbing (wildcard expansion), potentially altering arguments unpredictably.🎯 Impact:
VPNCMD_SERVER(e.g., disabling logs after enabling cipher) might fail silently if passed as multiple commands.🔧 Fix:
$usernameand$passwordinaddusercalls.-rto allreadcommands.VPNCMD_*loops to correctly iterate over all semicolon-separated commands.set -f/set +fto disable globbing while preserving argument splitting.✅ Verification:
bash -n.PR created automatically by Jules for task 12595697505069078969 started by @bluPhy
Summary by CodeRabbit
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.