🛡️ Sentinel: [CRITICAL] Fix Path Truncation Vulnerability via Null Bytes#3522
🛡️ Sentinel: [CRITICAL] Fix Path Truncation Vulnerability via Null Bytes#3522EffortlessSteven wants to merge 2 commits intomainfrom
Conversation
Added explicit rejection of null bytes ('\0') in `validate_model_request` to
prevent path truncation vulnerabilities when Rust strings are passed to
underlying C/OS file APIs. Included a unit test verifying this behavior.
|
👋 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 explicitly rejecting null bytes in model paths and includes a new unit test for verification. The review feedback suggests enhancing the API's error reporting by separating the null byte check from path traversal checks to provide more specific error messages, along with a corresponding update to the test case.
| if model_path.contains("..") || model_path.contains("~") || model_path.contains('\0') { | ||
| return Err(ValidationError::InvalidFieldValue( | ||
| "Invalid characters in model path".to_string(), | ||
| )); |
There was a problem hiding this comment.
While the null byte check correctly addresses the vulnerability, grouping it with path traversal checks under a generic "Invalid characters" error message reduces the specificity of the API feedback. Splitting these checks allows for more precise error reporting, which is beneficial for both security auditing and client-side debugging. Additionally, using a character literal for the ~ check is slightly more idiomatic in Rust.
| if model_path.contains("..") || model_path.contains("~") || model_path.contains('\0') { | |
| return Err(ValidationError::InvalidFieldValue( | |
| "Invalid characters in model path".to_string(), | |
| )); | |
| if model_path.contains('\0') { | |
| return Err(ValidationError::InvalidFieldValue( | |
| "Null byte detected in model path".to_string(), | |
| )); | |
| } | |
| if model_path.contains("..") || model_path.contains('~') { | |
| return Err(ValidationError::InvalidFieldValue( | |
| "Path traversal patterns (.. or ~) are not allowed in model path".to_string(), | |
| )); |
|
|
||
| assert!(matches!( | ||
| validator.validate_model_request("malicious_path\0.gguf"), | ||
| Err(ValidationError::InvalidFieldValue(msg)) if msg == "Invalid characters in model path" |
There was a problem hiding this comment.
Update the test case to match the more specific error message suggested for the validation logic.
| Err(ValidationError::InvalidFieldValue(msg)) if msg == "Invalid characters in model path" | |
| Err(ValidationError::InvalidFieldValue(msg)) if msg == "Null byte detected in model path" |
Reduced test debug symbols and added disk space clearing steps to CPU Tests in gpu-ci-matrix.yml to fix runner space exhaustion.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
validate_model_requestendpoint verified.ggufand.safetensorsextensions but did not reject null bytes (\0). When Rust strings are passed to underlying C/C++ libraries or OS system calls (e.g., viaCString), they are truncated at the null byte.🎯 Impact: An attacker could bypass the extension constraint by supplying a malicious path like
../../../etc/passwd\0.gguf. The Rust.ends_with(".gguf")check would pass, but the underlying OS call would truncate it, potentially loading arbitrary files or probing the system.🔧 Fix: Explicitly check for and reject null bytes (
\0) when validating user-supplied file paths insecurity.rs.✅ Verification: Ran
cargo test -p bitnet-server --lib security::tests::test_model_path_null_byte_rejectionsuccessfully.PR created automatically by Jules for task 1221079664746197106 started by @EffortlessSteven