🛡️ Sentinel: [CRITICAL] Fix Path Truncation Vulnerability#3471
🛡️ Sentinel: [CRITICAL] Fix Path Truncation Vulnerability#3471EffortlessSteven wants to merge 2 commits intomainfrom
Conversation
Updates the `validate_model_request` function in `bitnet-server` to explicitly reject any paths containing null bytes (`\0`). This prevents attackers from bypassing file extension restrictions (e.g., passing `/models/secret\0.gguf` to read `/models/secret`) by exploiting C-level string truncation bugs in the underlying filesystem APIs. Adds a test to verify null bytes are correctly rejected.
|
👋 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. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 check for null bytes in model paths to prevent injection vulnerabilities and adds a corresponding regression test. The reviewer suggests strengthening the validation by using char::is_control to block all control characters and recommends adding further test cases to cover these scenarios.
|
|
||
| // Prevent path traversal attacks | ||
| if model_path.contains("..") || model_path.contains("~") { | ||
| if model_path.contains("..") || model_path.contains("~") || model_path.contains('\0') { |
There was a problem hiding this comment.
While adding the check for the null byte (\0) is a great fix for the reported path truncation vulnerability, we can make this validation even more robust by disallowing all control characters in the model path, not just the null byte. File paths should not contain control characters, as they can lead to various injection or interpretation issues.
You can achieve this by using char::is_control. This would also cover the null byte case since \'\0\'.is_control() is true.
| if model_path.contains("..") || model_path.contains("~") || model_path.contains('\0') { | |
| if model_path.contains("..") || model_path.contains("~") || model_path.chars().any(char::is_control) { |
| // Null byte injection attempt | ||
| assert!(matches!( | ||
| validator.validate_model_request("/models/llama.gguf\0.gguf"), | ||
| Err(ValidationError::InvalidFieldValue(msg)) if msg == "Invalid characters in model path" | ||
| )); |
There was a problem hiding this comment.
This is a great regression test for the null byte injection! To complement the suggested change of blocking all control characters, it would be beneficial to add another test case that verifies other control characters are also rejected. This ensures the validation is robust against a broader class of invalid inputs.
// Null byte injection attempt
assert!(matches!(
validator.validate_model_request("/models/llama.gguf\0.gguf"),
Err(ValidationError::InvalidFieldValue(msg)) if msg == "Invalid characters in model path"
));
// Other control character injection attempt
assert!(matches!(
validator.validate_model_request("/models/llama\x07.gguf"), // Using BELL character
Err(ValidationError::InvalidFieldValue(msg)) if msg == "Invalid characters in model path"
));Updates the `validate_model_request` function in `bitnet-server` to explicitly reject any paths containing null bytes (`\0`). This prevents attackers from bypassing file extension restrictions (e.g., passing `/models/secret\0.gguf` to read `/models/secret`) by exploiting C-level string truncation bugs in the underlying filesystem APIs. Adds a test to verify null bytes are correctly rejected. Also fixes GitHub Actions CI failure due to running out of disk space during `cargo test --workspace` on Ubuntu 22.04 runner. This removes unnecessary toolchains like Android, dotnet and ghc, and minimizes test artifacts by disabling debug symbols.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Path Truncation Vulnerability via Null Byte Injection.
The model path validation checked for file extensions (
.gguf) and traversal strings (..) but failed to reject strings containing a null byte (\0). In Rust,&strcan contain null bytes, but when passed to underlying C-style OS APIs, the path is truncated at the null byte. An attacker could bypass the extension validation by supplying a path like/models/secret\0.gguf. The validation would pass (ends in.gguf), but the OS would attempt to read/models/secret.🎯 Impact: Potential arbitrary file read within the allowed model directories (or broader if combined with other vulnerabilities), bypassing security controls designed to restrict file types.
🔧 Fix: Updated the
validate_model_requestlogic incrates/bitnet-server/src/security.rsto explicitly check for and reject paths containing\0. Added a corresponding regression test.✅ Verification: Run
cargo test -p bitnet-server --lib security::tests.PR created automatically by Jules for task 3383889181527515022 started by @EffortlessSteven