🛡️ Sentinel: [CRITICAL] Fix Path Truncation Vulnerability via Null Byte Injection#3597
🛡️ Sentinel: [CRITICAL] Fix Path Truncation Vulnerability via Null Byte Injection#3597EffortlessSteven wants to merge 1 commit into
Conversation
…_model_request
The validate_model_request function previously used `.ends_with(".gguf")`
to validate file extensions but did not explicitly check for null bytes
(`\0`). This exposed the server to path truncation attacks, as underlying
C/C++ libraries (like llama.cpp) would truncate the string at the null
byte, bypassing the extension check. This commit adds an explicit rejection
of null bytes in the path validation logic and includes a regression test.
|
👋 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 addresses a path truncation vulnerability by implementing a check for null bytes in model paths and updating the security documentation and unit tests accordingly. The feedback suggests broadening the validation to reject all control characters, rather than just null bytes, to provide more robust protection against path obfuscation and injection attacks.
| // Prevent path traversal attacks | ||
| if model_path.contains("..") || model_path.contains("~") { | ||
| // Prevent path traversal attacks and null byte injection | ||
| if model_path.contains("..") || model_path.contains("~") || model_path.contains('\0') { |
There was a problem hiding this comment.
While explicitly checking for null bytes (\0) correctly addresses the path truncation vulnerability, it is generally safer to reject all control characters in file paths. Control characters like newlines or tabs are rarely valid in filenames and can be used to obfuscate paths or confuse logging systems. Consider broadening this check to use is_control() for more robust validation.
| if model_path.contains("..") || model_path.contains("~") || model_path.contains('\0') { | |
| if model_path.contains("..") || model_path.contains("~") || model_path.chars().any(|c| c.is_control()) { |
| ## 2024-05-24 - Path Truncation Vulnerability via Null Byte Injection | ||
| **Vulnerability:** The `validate_model_request` function validated file extensions using `.ends_with(".gguf")` but did not explicitly check for null bytes (`\0`). This allowed path truncation attacks where a malicious user could supply `/etc/passwd\0.gguf`. While the Rust path validation might accept it, underlying C/C++ libraries (like `llama.cpp`) would truncate the string at the null byte, interpreting it as `/etc/passwd`, potentially exposing sensitive data. | ||
| **Learning:** String validation methods like `.ends_with()` in languages with length-prefixed strings (like Rust) can be bypassed if the string is later passed to C-style null-terminated string APIs. | ||
| **Prevention:** Always explicitly reject null bytes (`\0`) when validating file paths or input that will eventually interface with C/OS APIs. |
There was a problem hiding this comment.
Consider broadening the prevention advice to include all control characters, as they are generally unsafe in file paths and can lead to various injection or obfuscation issues beyond just null byte truncation.
| **Prevention:** Always explicitly reject null bytes (`\0`) when validating file paths or input that will eventually interface with C/OS APIs. | |
| **Prevention:** Always explicitly reject control characters (including null bytes `\0`) when validating file paths or input that will eventually interface with C/OS APIs. |
|
Closing as superseded by #3626. Reason: duplicate null-byte model path validation fix. The canonical PR is ledger-aligned under |
Understood. Acknowledging that this work is superseded by #3626 and stopping work on this task. |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
validate_model_requestfunction validated file extensions using.ends_with(".gguf")but did not explicitly check for null bytes (\0). This allowed path truncation attacks where a malicious user could supply/etc/passwd\0.gguf.🎯 Impact: While the Rust path validation might accept it, underlying C/C++ libraries (like
llama.cpp) would truncate the string at the null byte, interpreting it as/etc/passwd, potentially exposing sensitive data or traversing the directory.🔧 Fix: Explicitly rejected null bytes (
\0) in the path validation logic.✅ Verification: Run
cargo test -p bitnet-server test_model_path_restriction -- --exactto verify the new tests pass.PR created automatically by Jules for task 14647859766468245499 started by @EffortlessSteven