🔒 Fix Command Injection in bs2system - #45
Conversation
The `bs2cmdlist` function in `tc.os.c` iterates over user input and passes parts of the string directly to `bs2system()`, an OS-specific command execution function similar to `system()`. This allows a potential attacker to execute arbitrary commands by embedding shell metacharacters in the input. This patch adds a sanitization check using `strpbrk` to detect and reject dangerous shell metacharacters (`&`, `|`, `<`, `>`, `$`, \`\`, `\n`, `\r`) in the string `str_beg` right before `bs2system` is called. If any are detected, it aborts the execution via `stderror(ERR_NAME | ERR_STRING, ...)`. Co-authored-by: orpheus497 <230802898+orpheus497@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. |
|
Warning Review limit reached
More reviews will be available in 28 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ✨ 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 |
![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 introduces a security check in tc.os.c to prevent command injection by rejecting unsafe characters in commands before they are executed. The review feedback points out a correctness issue where valid commands containing these characters inside quotes would be incorrectly rejected, and provides a robust code suggestion to track quote contexts during validation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (strpbrk(str_beg, "&|<>$`\n\r") != NULL) { | ||
| stderror(ERR_NAME | ERR_STRING, "unsafe character in command"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Using strpbrk directly on str_beg introduces a correctness issue where valid commands containing any of the blocked characters (such as & or $) inside single or double quotes (e.g., PRINT "Hello & Welcome") will be incorrectly rejected as unsafe.
Since bs2cmdlist already parses quotes to identify command boundaries, we should respect the quote context when validating characters. We can use an inline loop that tracks single and double quotes to only reject unsafe characters when they appear outside of quotes.
const char *p;
int sq = 0, dq = 0;
for (p = str_beg; *p != '\0'; p++)
{
if (*p == '\'' && !dq)
sq = !sq;
else if (*p == '"' && !sq)
dq = !dq;
else if (!sq && !dq && strchr("&|<>$\140\n\r", *p) != NULL)
{
stderror(ERR_NAME | ERR_STRING, "unsafe character in command");
return -1;
}
}There was a problem hiding this comment.
Jules
Using strpbrk directly on str_beg introduces a correctness issue where valid commands containing any of the blocked characters (such as & or $) inside single or double quotes (e.g., PRINT "Hello & Welcome") will be incorrectly rejected as unsafe.
Since bs2cmdlist already parses quotes to identify command boundaries, we should respect the quote context when validating characters. We can use an inline loop that tracks single and double quotes to only reject unsafe characters when they appear outside of quotes.
const char *p;
int sq = 0, dq = 0;
for (p = str_beg; *p != '\0'; p++)
{
if (*p == '\'' && !dq)
sq = !sq;
else if (*p == '"' && !sq)
dq = !dq;
else if (!sq && !dq && strchr("&|<>$\140\n\r", *p) != NULL)
{
stderror(ERR_NAME | ERR_STRING, "unsafe character in command");
return -1;
}
}
There was a problem hiding this comment.
You make a great point about quoted characters, implemented!
The `bs2cmdlist` function in `tc.os.c` iterates over user input and passes parts of the string directly to `bs2system()`, an OS-specific command execution function similar to `system()`. This allows a potential attacker to execute arbitrary commands by embedding shell metacharacters in the input. This patch adds a sanitization check to detect and reject dangerous shell metacharacters (`&`, `|`, `<`, `>`, `$`, \`\`, `\n`, `\r`) in the string `str_beg` right before `bs2system` is called. It correctly handles quoted sections, ensuring that valid commands containing characters like `&` inside quotes are not falsely rejected. If an unsafe character is found outside of quotes, it aborts the execution via `stderror(ERR_NAME | ERR_STRING, ...)`. Co-authored-by: orpheus497 <230802898+orpheus497@users.noreply.github.com>
1 participant
🎯 What:
Fixes a command injection vulnerability in
tc.os.cwhere an unsanitized string was passed directly to the OS-native execution functionbs2system().If an attacker can control or influence the string parsed by
bs2cmdlist, they could embed shell metacharacters to break out of the intended command context and execute arbitrary malicious commands on the system, leading to system compromise.🛡️ Solution:
Added a check right before the
bs2systemcall. The code now usesstrpbrkto inspectstr_begfor dangerous shell execution characters (&,|,<,>,$, ``,\n,\r). If any of these characters are found,tcsh'sstderrorfunction is called to raise an error and safely abort execution, preventing the command injection.PR created automatically by Jules for task 10081930905187937297 started by @orpheus497