🛡️ Sentinel: [CRITICAL] Fix PDF compilation RCE and timeout vulnerabilities#353
🛡️ Sentinel: [CRITICAL] Fix PDF compilation RCE and timeout vulnerabilities#353anchapin wants to merge 1 commit into
Conversation
…lities Added `-no-shell-escape` (pdflatex) and `--pdf-engine-opt=-no-shell-escape` (pandoc) flags to subprocesses in `cli/pdf/converter.py` and `cli/generators/cover_letter_generator.py` to prevent Remote Code Execution via LaTeX `\write18` primitive. Also implemented 30-second timeouts with proper process cleanup (`process.kill()`, secondary `.communicate()`, and early exit) to mitigate Denial of Service (DoS) attacks caused by infinite compilation loops. Added security journal entry for PDF compilation hazards. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
|
👋 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. |
Reviewer's GuideHardens PDF compilation by disabling LaTeX shell escapes and enforcing timeouts around pdflatex/pandoc subprocesses, plus documenting the vulnerability and mitigation in the Sentinel security log. Sequence diagram for secured pdflatex PDF compilation with timeoutsequenceDiagram
participant Caller
participant Converter as converter._compile_pdflatex
participant Subprocess as subprocess_Popen
participant Process as pdflatex_process
Caller->>Converter: _compile_pdflatex(tex_path, output_path, working_dir)
Converter->>Subprocess: Popen(["pdflatex","-interaction=nonstopmode","-no-shell-escape",tex_path.name])
Subprocess-->>Converter: Process
Converter->>Process: communicate(timeout=30)
alt [subprocess.TimeoutExpired]
Converter->>Process: kill()
Converter->>Process: communicate()
Converter-->>Caller: False
else [no timeout]
Process-->>Converter: stdout, stderr
Converter-->>Caller: True/False
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The subprocess timeout and kill/cleanup pattern is duplicated across multiple compilation functions; consider extracting a shared helper to ensure consistent behavior and make future hardening (e.g., changing timeout) easier.
- The 30-second timeout is currently hard-coded in several places; moving this to a single constant or configuration value would make it simpler to tune or override for different environments.
- You are capturing stdout/stderr from pdflatex/pandoc but never use them on failure or timeout; either log/inspect these streams in the error path for easier debugging, or avoid capturing them to reduce unnecessary resource use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The subprocess timeout and kill/cleanup pattern is duplicated across multiple compilation functions; consider extracting a shared helper to ensure consistent behavior and make future hardening (e.g., changing timeout) easier.
- The 30-second timeout is currently hard-coded in several places; moving this to a single constant or configuration value would make it simpler to tune or override for different environments.
- You are capturing stdout/stderr from pdflatex/pandoc but never use them on failure or timeout; either log/inspect these streams in the error path for easier debugging, or avoid capturing them to reduce unnecessary resource use.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🚨 Severity: CRITICAL
💡 Vulnerability: Unsandboxed PDF compilation allowed arbitrary shell command execution (RCE) via LaTeX control sequences injected into templates. The lack of a subprocess timeout also allowed malformed input to stall the compilation indefinitely (DoS).
🎯 Impact: Malicious input or AI hallucinations could lead to complete system compromise via Remote Code Execution, or hang the server thread completely causing Denial of Service.
🔧 Fix: Added
-no-shell-escapeand--pdf-engine-opt=-no-shell-escapeflags to allsubprocess.Popeninvocations forpdflatexandpandocincli/pdf/converter.pyandcli/generators/cover_letter_generator.py. Implemented strict 30-second timeouts (process.communicate(timeout=30)) with rigorous cleanup (catchingTimeoutExpired, killing the process, and ensuring zombies are cleared) that returnsFalsesafely instead of halting the system.✅ Verification: Verified compilation parameters and timeout behavior via
python -m pytest tests/test_pdf_security.py tests/test_cover_letter_security.py. Ran full suite to ensure no regressions. All 681 tests passed.PR created automatically by Jules for task 15607158112052287102 started by @anchapin
Summary by Sourcery
Harden PDF generation by sandboxing LaTeX compilation and enforcing timeouts to prevent RCE and DoS via untrusted templates.
Bug Fixes:
Documentation: