🛡️ Sentinel: [HIGH] Harden entrypoint.sh script execution #30
Conversation
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. |
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 significantly hardens 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
|
WalkthroughThis PR adds a security journal entry documenting shell script variable expansion vulnerabilities and updates an entrypoint script to improve robustness through proper variable quoting and safer read patterns. 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 |
![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-162: The loop calling vpncmd_server currently invokes it with
an unquoted scalar ("vpncmd_server $cmd") which allows pathname expansion; to
fix, re-parse each command string into an array and call vpncmd_server with the
array expansion (e.g., read -r -a ARGS <<< "$cmd" then call vpncmd_server
"${ARGS[@]}") or temporarily disable globbing with set -f around the invocation
and restore it, and apply the same change to the other occurrence mentioned (the
block at the later loop around lines 167-170); reference vpncmd_server, CMDS and
cmd when making the change.
| while IFS=";" read -r -a CMDS; do | ||
| for cmd in "${CMDS[@]}"; do | ||
| vpncmd_server $cmd | ||
| done |
There was a problem hiding this comment.
Prevent globbing when dispatching VPNCMD commands.
vpncmd_* $cmd still expands globs and can mutate arguments from user-provided env vars. Re-parse into an array and pass quoted to keep splitting but block globbing.
🔧 Proposed fix
- for cmd in "${CMDS[@]}"; do
- vpncmd_server $cmd
- done
+ for cmd in "${CMDS[@]}"; do
+ [[ -z "$cmd" ]] && continue
+ read -r -a CMD_PARTS <<<"$cmd"
+ vpncmd_server "${CMD_PARTS[@]}"
+ done
@@
- for cmd in "${CMDS[@]}"; do
- vpncmd_hub $cmd
- done
+ for cmd in "${CMDS[@]}"; do
+ [[ -z "$cmd" ]] && continue
+ read -r -a CMD_PARTS <<<"$cmd"
+ vpncmd_hub "${CMD_PARTS[@]}"
+ doneAlso applies to: 167-170
🤖 Prompt for AI Agents
In `@copyables/entrypoint.sh` around lines 159 - 162, The loop calling
vpncmd_server currently invokes it with an unquoted scalar ("vpncmd_server
$cmd") which allows pathname expansion; to fix, re-parse each command string
into an array and call vpncmd_server with the array expansion (e.g., read -r -a
ARGS <<< "$cmd" then call vpncmd_server "${ARGS[@]}") or temporarily disable
globbing with set -f around the invocation and restore it, and apply the same
change to the other occurrence mentioned (the block at the later loop around
lines 167-170); reference vpncmd_server, CMDS and cmd when making the change.
![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 aims to enhance the security and robustness of the entrypoint.sh script by addressing critical vulnerabilities related to command injection, data integrity, and input parsing. While it correctly implements read -r for safer input handling and quotes variables to prevent unintended word splitting and globbing, the hardening is incomplete. The $cmd variable remains unquoted in the loops for VPNCMD_SERVER and VPNCMD_HUB, leaving the script vulnerable to globbing and incorrect argument splitting if environment variables are untrusted. The PR also includes valuable documentation in .jules/sentinel.md regarding identified vulnerabilities and their prevention.
| vpncmd_server $CMD | ||
| while IFS=";" read -r -a CMDS; do | ||
| for cmd in "${CMDS[@]}"; do | ||
| vpncmd_server $cmd |
There was a problem hiding this comment.
The original logic only executed the first command in a semicolon-separated list, silently ignoring subsequent commands, which was a critical logic flaw. However, the variable $cmd is still expanded without double quotes, which can lead to globbing and insecure word splitting. If an attacker can influence VPNCMD_SERVER (e.g., by including a * character), the shell will expand it to filenames, passing unexpected arguments to the VPN command. This undermines the PR's security hardening goals. A temporary array should be used to safely split the command string.
| vpncmd_server $cmd | |
| read -r -a cmd_args <<< "$cmd" | |
| vpncmd_server "${cmd_args[@]}" |
| vpncmd_hub $CMD | ||
| while IFS=";" read -r -a CMDS; do | ||
| for cmd in "${CMDS[@]}"; do | ||
| vpncmd_hub $cmd |
There was a problem hiding this comment.
Similar to the VPNCMD_SERVER block, ensuring all commands from VPNCMD_HUB are executed is essential for proper configuration and security. However, the $cmd variable is expanded without double quotes, which triggers globbing and insecure word splitting. This should be handled safely using an array to prevent unexpected behavior if the command contains glob characters or spaces, which could lead to silent failures of critical steps.
| vpncmd_hub $cmd | |
| read -r -a cmd_args <<< "$cmd" | |
| vpncmd_hub "${cmd_args[@]}" |
| CONFIG=/var/lib/softether/vpn_server.config | ||
|
|
||
| if [ ! -f $CONFIG ] || [ ! -s $CONFIG ]; then | ||
| if [ ! -f "$CONFIG" ] || [ ! -s "$CONFIG" ]; then |
There was a problem hiding this comment.
The change to quote $CONFIG is a good practice. Unquoted variables can lead to unexpected behavior due to word splitting or globbing, especially if the path contains spaces or special characters. This improves the script's robustness and security.
🛡️ Sentinel: [HIGH] Harden entrypoint.sh script execution
Severity: HIGH
Vulnerability:
entrypoint.shscript incorrectly processed semicolon-separated commands inVPNCMD_SERVERandVPNCMD_HUB. It read them into an array but only executed the first element ($CMDinstead of${CMDS[@]}), silently ignoring subsequent commands which might be critical for security configuration.readwas used without-r, causing backslashes in passwords to be interpreted as escape characters.Fix:
read -rto all read commands.$username,$password,$CONFIG, etc.) to prevent splitting.VPNCMD_SERVERandVPNCMD_HUB..jules/sentinel.mdto document the learning.Verification:
bash -n.PR created automatically by Jules for task 5829468812831589290 started by @bluPhy
Summary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.