Skip to content

Comments

🛡️ Sentinel: [CRITICAL] Restrict Rhai shell execution and fix argument injection#31

Closed
JustinWoodring wants to merge 1 commit intomainfrom
sentinel-security-fix-rhai-shell-14236109661549707331
Closed

🛡️ Sentinel: [CRITICAL] Restrict Rhai shell execution and fix argument injection#31
JustinWoodring wants to merge 1 commit intomainfrom
sentinel-security-fix-rhai-shell-14236109661549707331

Conversation

@JustinWoodring
Copy link
Owner

This PR addresses two critical security issues in the slatron-server Rhai scripting engine:

  1. Arbitrary Command Execution: The shell_execute function previously allowed executing any command available on the server. This has been restricted to a whitelist of safe/expected commands (yt-dlp, ffmpeg, ffprobe).
  2. Argument Injection: The download_file function used curl with user-supplied URLs as arguments. A URL starting with - could be interpreted as a flag. This is now blocked.

Tests have been added to verify these fixes.


PR created automatically by Jules for task 14236109661549707331 started by @JustinWoodring

…t injection

- **shell_execute**: Added `ALLOWED_COMMANDS` whitelist (`yt-dlp`, `ffmpeg`, `ffprobe`) to prevent arbitrary RCE. Attempts to execute other commands now return code -1.
- **download_file**: Added validation to reject URLs starting with `-` to prevent argument injection in the underlying `curl` command.
- **Tests**: Added regression tests in `slatron-server/src/rhai_engine/security_test.rs` covering both scenarios.

Sentinel Journal updated in `.jules/sentinel.md`.

Co-authored-by: JustinWoodring <41842051+JustinWoodring@users.noreply.github.com>
@google-labs-jules
Copy link
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@claude
Copy link

claude bot commented Jan 30, 2026


Code Review

I've identified 2 security issues that should be addressed:

1. Missing Argument Injection Protection for `output_path` Parameter

Location:

return false;
}
// Security Check: Prevent Argument Injection
if url.starts_with('-') {
tracing::error!("Security Alert: Script attempted argument injection: {}", url);
return false;
}

The PR adds argument injection protection for the `url` parameter by checking `url.starts_with('-')`, but does NOT add the same protection for the `output_path` parameter. Since `output_path` is passed to curl as `-o <output_path>`, if it starts with `-`, it will be interpreted as a curl flag.

Concrete exploit:
```rhai
download_file("https://attacker.com/file", "-K/etc/passwd")
```

This would result in: `curl -L -o -K/etc/passwd https://attacker.com/file\`, where `-K/etc/passwd` is interpreted as the curl config flag, reading `/etc/passwd` as a config file.

Fix: Add the same check for `output_path`:
```rust
if output_path.starts_with('-') {
tracing::error!("Security Alert: Script attempted argument injection via output_path: {}", output_path);
return false;
}
```

Also affects: `slatron-node/src/rhai_engine/mod.rs` has the same vulnerability and needs the same fix for both `url` AND `output_path`.


2. Node-Side Shell Execution Not Fixed

Location:

engine.register_fn("shell_execute", |cmd: String| -> String {
use std::process::Command;
match Command::new("sh").arg("-c").arg(&cmd).output() {
Ok(output) => String::from_utf8_lossy(&output.stdout).to_string(),
Err(e) => format!("Error: {}", e),
}
});

The PR restricts `shell_execute` on the server side, but the node-side implementation remains completely unrestricted and uses the dangerous `sh -c` pattern:

```rust
engine.register_fn("shell_execute", |cmd: String| -> String {
match Command::new("sh").arg("-c").arg(&cmd).output() {
// ...
}
});
```

This allows arbitrary command execution on nodes. The same allowlist restriction should be applied to the node-side implementation.

Fix: Apply the same `ALLOWED_COMMANDS` allowlist to `slatron-node/src/rhai_engine/mod.rs`.


Summary: While this PR improves server-side security, the argument injection fix is incomplete (missing `output_path` validation) and the command allowlist is not applied to the node side, leaving a significant security gap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant