🛡️ Sentinel: [MEDIUM] Fix TOCTOU vulnerability and unbounded memory consumption in Packet Builder#291
🛡️ Sentinel: [MEDIUM] Fix TOCTOU vulnerability and unbounded memory consumption in Packet Builder#291EffortlessSteven wants to merge 2 commits intomainfrom
Conversation
|
👋 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 You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA security vulnerability addressing TOCTOU race conditions and unbounded memory consumption is documented in the security sentinel. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/security/sentinel.md:
- Line 148: The new heading "## 2026-03-23 - TOCTOU and Unbounded Memory
Consumption in File Reading" is glued to the previous list item causing
markdownlint MD022 and rendering issues; edit the markdown around that exact
heading to insert a blank line immediately before and another blank line
immediately after the heading so it stands alone as its own section.
In `@crates/xchecker-packet/src/builder.rs`:
- Around line 450-458: The code currently opens the path with
std::fs::File::open(&candidate.path) and only then checks metadata.is_file(),
which allows a TOCTOU where the file could be swapped to a symlink or FIFO;
update the open logic in the function containing this snippet so that you either
(a) open via std::fs::OpenOptions and set platform-specific custom_flags
(libc::O_NOFOLLOW | libc::O_NONBLOCK) so the open fails immediately on symlinks
and doesn't block on FIFOs, or (b) after opening the file descriptor, obtain the
file-type from the descriptor itself (e.g., via fstat on the File) and validate
that it is a regular file before proceeding (replace the current file.metadata()
/ metadata.is_file() pattern). Ensure you replace uses of
std::fs::File::open(&candidate.path) and metadata.is_file() with the chosen
safe-open and descriptor-based validation so candidate.path cannot be swapped
between selection and open.
- Around line 483-513: The current read uses std::io::Read::read_to_string with
std::io::Read::take which can cut a UTF-8 codepoint and cause InvalidData;
change the logic in builder.rs so you first read raw bytes into a Vec<u8> (use
std::io::Read::read_to_end on std::io::Read::take(&mut file, max_file_size + 1))
with the same with_context error wrapping, then check if the buffer.len() as u64
> max_file_size and apply the same Priority::Upstream error vs tracing::warn +
Ok(None) path on overflow, and only after the size check decode to a String
(e.g., String::from_utf8 or std::str::from_utf8) and assign to the existing
content variable so downstream logic is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 736ff654-f8d6-4353-8389-6b288cc91296
📒 Files selected for processing (2)
.jules/security/sentinel.mdcrates/xchecker-packet/src/builder.rs
|
|
||
| **Files Changed:** | ||
| - `crates/xchecker-utils/src/redaction.rs` | ||
| ## 2026-03-23 - TOCTOU and Unbounded Memory Consumption in File Reading |
There was a problem hiding this comment.
Restore blank lines around the new heading.
Line 148 is attached directly to the preceding list item, so markdownlint will keep failing MD022 and the new entry may not render as its own section. Add a blank line before and after the heading.
🧰 Tools
🪛 LanguageTool
[typographical] ~148-~148: To join two clauses or introduce examples, consider using an em dash.
Context: ...er-utils/src/redaction.rs` ## 2026-03-23 - TOCTOU and Unbounded Memory Consumption ...
(DASH_RULE)
🪛 markdownlint-cli2 (0.21.0)
[warning] 148-148: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 148-148: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/security/sentinel.md at line 148, The new heading "## 2026-03-23 -
TOCTOU and Unbounded Memory Consumption in File Reading" is glued to the
previous list item causing markdownlint MD022 and rendering issues; edit the
markdown around that exact heading to insert a blank line immediately before and
another blank line immediately after the heading so it stands alone as its own
section.
| // Optimization: Open file once to avoid redundant path resolution and TOCTOU | ||
| let mut file = std::fs::File::open(&candidate.path) | ||
| .with_context(|| format!("Failed to open file: {}", candidate.path))?; | ||
|
|
||
| // Note: file.metadata() follows symlinks if opened via fs::File::open | ||
| let metadata = file.metadata() | ||
| .with_context(|| format!("Failed to get file metadata: {}", candidate.path))?; | ||
|
|
||
| if !metadata.is_file() { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Rust std::fs::File::open follow symlinks, and on Unix can opening a FIFO for reading block until a writer appears? What Rust/POSIX approach is recommended to refuse those targets (O_NOFOLLOW, O_NONBLOCK, openat2, etc.)?
💡 Result:
Yes, Rust std::fs::File::open follows symlinks, as it uses the standard POSIX open(2) call without O_NOFOLLOW, which resolves symlinks in the path by default. Yes, on Unix, opening a FIFO (named pipe) for reading with O_RDONLY (without O_NONBLOCK) blocks until a writer opens the other end, per POSIX semantics. Recommended Rust/POSIX approach to refuse symlinks and block on FIFOs: - Use std::fs::OpenOptions with Unix extensions: options.read(true).custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK). - O_NOFOLLOW: Fail with ELOOP if the final path component is a symlink (symlinks in prefix are followed). - O_NONBLOCK: For FIFOs, open immediately without blocking; subsequent reads return EAGAIN/EWOULDBLOCK if no data/writer. - To strictly refuse all symlinks (including prefix), use Linux-specific openat2(2) with RESOLVE_NOFOLLOW | RESOLVE_NO_XDEV (requires #[cfg(linux)] and raw FFI or crates like rustix, not in std). - Example: use std::fs::OpenOptions; use std::os::unix::fs::OpenOptionsExt; use std::io; let mut options = OpenOptions::new; options.read(true); options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); let file = options.open("path")?; This opens read-only, fails on symlink targets, non-blocking open for FIFOs.
Citations:
- 1: https://doc.rust-lang.org/std/fs/struct.OpenOptions.html
- 2: https://man7.org/linux/man-pages/man2/open.2.html
- 3: https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
- 4: https://github.com/rust-lang/rust/blob/master/library/std/src/fs.rs
- 5: https://doc.rust-lang.org/stable/std/os/unix/fs/trait.OpenOptionsExt.html
- 6: https://stackoverflow.com/questions/8507810/why-does-my-program-hang-when-opening-a-mkfifo-ed-pipe
- 7: https://stackoverflow.com/questions/5782279/why-does-a-read-only-open-of-a-named-pipe-block
- 8: https://doc.rust-lang.org/src/std/sys/fs/unix.rs.html
Eliminate symlink/FIFO TOCTOU between candidate selection and file open.
select_candidates() passes a path string to the worker. Between that point and Line 451, the filesystem entry can be swapped to a symlink or FIFO. File::open (which follows symlinks and blocks on FIFOs until a writer appears) executes before Line 458's is_file() check, reintroducing arbitrary-file-read and packet-hang failures.
Use std::fs::OpenOptions with custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) to fail immediately on symlinks and open FIFOs non-blocking. Alternatively, validate as-file after opening by obtaining and checking inode type from the open file descriptor itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/xchecker-packet/src/builder.rs` around lines 450 - 458, The code
currently opens the path with std::fs::File::open(&candidate.path) and only then
checks metadata.is_file(), which allows a TOCTOU where the file could be swapped
to a symlink or FIFO; update the open logic in the function containing this
snippet so that you either (a) open via std::fs::OpenOptions and set
platform-specific custom_flags (libc::O_NOFOLLOW | libc::O_NONBLOCK) so the open
fails immediately on symlinks and doesn't block on FIFOs, or (b) after opening
the file descriptor, obtain the file-type from the descriptor itself (e.g., via
fstat on the File) and validate that it is a regular file before proceeding
(replace the current file.metadata() / metadata.is_file() pattern). Ensure you
replace uses of std::fs::File::open(&candidate.path) and metadata.is_file() with
the chosen safe-open and descriptor-based validation so candidate.path cannot be
swapped between selection and open.
| // Pre-allocate buffer to avoid reallocations | ||
| let mut content = String::with_capacity(metadata.len() as usize); | ||
|
|
||
| // Security: Use take() to enforce hard limit on read size | ||
| // This prevents DoS if the file grows concurrently (TOCTOU race) | ||
| std::io::Read::read_to_string( | ||
| &mut std::io::Read::take(&mut file, max_file_size + 1), | ||
| &mut content, | ||
| ) | ||
| .with_context(|| format!("Failed to read file: {}", candidate.path))?; | ||
|
|
||
| // Post-read size check: handles case where file grew during read | ||
| if content.len() as u64 > max_file_size { | ||
| if candidate.priority == Priority::Upstream { | ||
| return Err(anyhow::anyhow!( | ||
| "Upstream file {} exceeds size limit of {} bytes (read: {}). \ | ||
| Critical context files must fit within the configured limit.", | ||
| candidate.path, | ||
| max_file_size, | ||
| content.len() | ||
| )); | ||
| } | ||
|
|
||
| tracing::warn!( | ||
| "Skipping large file: {} ({} bytes > limit {}) - file grew during read", | ||
| candidate.path, | ||
| content.len(), | ||
| max_file_size | ||
| ); | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust, does std::io::Read::read_to_stringreturn an error when aRead::take(...) wrapper stops in the middle of a UTF-8 code point?
💡 Result:
Yes, std::io::Read::read_to_string returns an error when a Read::take(...) wrapper stops in the middle of a UTF-8 code point.
Citations:
- 1: https://doc.rust-lang.org/1.62.0/std/io/trait.Read.html
- 2: https://doc.rust-lang.org/std/io/trait.Read.html
- 3: https://doc.rust-lang.org/stable/std/io/fn.read_to_string.html
- 4: http://doc.rust-lang.org/stable/src/std/io/impls.rs.html
- 5: https://doc.rust-lang.org/std/io/struct.Take.html
🏁 Script executed:
# Check the file and specific lines to confirm the code matches the review
fd "builder.rs" crates/xchecker-packet/src/ -x cat -n {} | sed -n '483,513p'Repository: EffortlessMetrics/xchecker-dev
Length of output: 1468
🏁 Script executed:
# Verify the full context of the function to understand error handling
cd crates/xchecker-packet/src && cat -n builder.rs | sed -n '450,530p'Repository: EffortlessMetrics/xchecker-dev
Length of output: 3612
Read UTF-8 text into bytes first, then validate size, then decode—read_to_string with take() fails on UTF-8 boundaries.
If take(max_file_size + 1) stops in the middle of a UTF-8 code point, read_to_string returns InvalidData at line 492 and propagates immediately via .with_context(). This prevents the graceful skip path at line 512 from executing for non-upstream files that grew during read, turning a benign size overage into a hard packet-build failure.
The pre-read metadata check (line 462) only catches files that already exceed the limit before the read begins. The file can grow in the TOCTOU window between metadata retrieval (line 455) and the actual read (lines 488–492). When a non-upstream file grows and causes take() to truncate mid-UTF-8 sequence, the error should be handled gracefully, not treated as a fatal I/O failure.
Read into Vec<u8> first with read_to_end, enforce the byte limit against the buffer, then decode UTF-8 only after confirming the buffer is within bounds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/xchecker-packet/src/builder.rs` around lines 483 - 513, The current
read uses std::io::Read::read_to_string with std::io::Read::take which can cut a
UTF-8 codepoint and cause InvalidData; change the logic in builder.rs so you
first read raw bytes into a Vec<u8> (use std::io::Read::read_to_end on
std::io::Read::take(&mut file, max_file_size + 1)) with the same with_context
error wrapping, then check if the buffer.len() as u64 > max_file_size and apply
the same Priority::Upstream error vs tracing::warn + Ok(None) path on overflow,
and only after the size check decode to a String (e.g., String::from_utf8 or
std::str::from_utf8) and assign to the existing content variable so downstream
logic is unchanged.
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: The
process_candidate_filefunction inContentSelectorusedfs::metadata(path)followed byfs::read_to_string(path), which introduces a Time-Of-Check to Time-Of-Use (TOCTOU) race condition. If a file grew between the metadata check and the read operation, it could lead to unbounded memory consumption (DoS).🎯 Impact: An attacker could cause the application to crash or consume excessive memory by rapidly increasing the size of a file after its metadata was checked but before it was read into memory.
🔧 Fix: Refactored
process_candidate_fileto acquire a file handle first usingfs::File::open. It then checks the size using the file handle's metadata (file.metadata().len()) and usesstd::io::Read::taketo strictly bound the read operation up to the configured limit.✅ Verification: Verified by running
cargo checkandcargo test -p xchecker-packet, which all passed successfully. The fix prevents unbounded memory allocation while safely handling concurrent file modifications.PR created automatically by Jules for task 8108001732240879206 started by @EffortlessSteven