π‘οΈ Sentinel: [CRITICAL] Fix LaTeX RCE and DoS in Subprocess Compilation#348
π‘οΈ Sentinel: [CRITICAL] Fix LaTeX RCE and DoS in Subprocess Compilation#348anchapin wants to merge 2 commits into
Conversation
Adds `-no-shell-escape` flags to pdflatex and pandoc commands in `cli/pdf/converter.py` and `cli/generators/cover_letter_generator.py`. Also implements explicit subprocess timeouts with cleanup (process.kill()) to prevent hanging processes and Denial of Service (DoS). 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 GuideHarden LaTeX PDF generation against RCE and DoS by disabling shell escapes for pdflatex/pandoc and enforcing timeouts with proper cleanup on all LaTeX compilation subprocesses, plus documenting the incident in sentinel.md. Sequence diagram for secure LaTeX PDF compilation with timeoutssequenceDiagram
actor User
participant CoverLetterGenerator as CoverLetterGenerator
participant PDFConverter as PDFConverter
participant pdflatex as pdflatex_process
participant pandoc as pandoc_process
User->>CoverLetterGenerator: _compile_pdf(output_path, tex_content)
CoverLetterGenerator->>pdflatex: subprocess.Popen(["pdflatex", "-interaction=nonstopmode", "-no-shell-escape", tex_path.name])
pdflatex-->>CoverLetterGenerator: process
CoverLetterGenerator->>pdflatex: process.communicate(timeout=30)
alt [timeout on pdflatex]
CoverLetterGenerator->>pdflatex: process.kill()
CoverLetterGenerator->>pdflatex: process.communicate()
CoverLetterGenerator-->>User: return False
else [pdflatex fails or no PDF]
CoverLetterGenerator->>pandoc: subprocess.Popen(["pandoc", tex_path, "-o", output_path, "--pdf-engine=xelatex", "--pdf-engine-opt=-no-shell-escape"])
pandoc-->>CoverLetterGenerator: process
CoverLetterGenerator->>pandoc: process.communicate(timeout=30)
alt [timeout on pandoc]
CoverLetterGenerator->>pandoc: process.kill()
CoverLetterGenerator->>pandoc: process.communicate()
CoverLetterGenerator-->>User: return False
else [pandoc succeeds]
CoverLetterGenerator-->>User: return True
end
end
User->>PDFConverter: _compile_pdflatex(tex_path, output_path, working_dir)
PDFConverter->>pdflatex: subprocess.Popen(["pdflatex", "-interaction=nonstopmode", "-no-shell-escape", tex_path.name])
pdflatex-->>PDFConverter: process
PDFConverter->>pdflatex: process.communicate(timeout=30)
alt [timeout on pdflatex]
PDFConverter->>pdflatex: process.kill()
PDFConverter->>pdflatex: process.communicate()
PDFConverter-->>User: return 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/kill pattern is duplicated in multiple places; consider extracting a small helper (e.g.
run_with_timeout(...)) so all LaTeX compilation calls share the same behavior and are easier to maintain consistently. - On timeout you currently discard
stdout/stderrand only returnFalse; capturing or logging at leaststderron failures and timeouts would make diagnosing LaTeX compilation issues much easier in production. - The 30-second timeout is hard-coded in several calls; it may be worth centralizing this as a configuration constant so it can be tuned per environment without code changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The subprocess timeout/kill pattern is duplicated in multiple places; consider extracting a small helper (e.g. `run_with_timeout(...)`) so all LaTeX compilation calls share the same behavior and are easier to maintain consistently.
- On timeout you currently discard `stdout`/`stderr` and only return `False`; capturing or logging at least `stderr` on failures and timeouts would make diagnosing LaTeX compilation issues much easier in production.
- The 30-second timeout is hard-coded in several calls; it may be worth centralizing this as a configuration constant so it can be tuned per environment without code changes.Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
Adds `-no-shell-escape` flags to pdflatex and pandoc commands in `cli/pdf/converter.py` and `cli/generators/cover_letter_generator.py`. Also implements explicit subprocess timeouts with cleanup (process.kill()) to prevent hanging processes and Denial of Service (DoS). Fixed code formatting issue. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
π¨ Severity: CRITICAL
π‘ Vulnerability: Subprocess calls to
pdflatexandpandoclacked the-no-shell-escapeflag. This could allow Remote Code Execution (RCE) if untrusted/injected LaTeX commands like\write18{...}are processed. In addition, an absence of timeouts meant compiling invalid syntax could cause infinite hanging processes, leading to DoS.π― Impact: Attackers could execute arbitrary shell commands on the server via generated/injected LaTeX or cause widespread denial of service by exhausting server resources with hanging LaTeX compilation processes.
π§ Fix: Added
-no-shell-escapetopdflatexand--pdf-engine-opt=-no-shell-escapetopandocfallback commands inPDFConverterandCoverLetterGenerator. Addedtimeout=30to allsubprocess.communicate()calls, catchingsubprocess.TimeoutExpiredto explicitly callprocess.kill()and cleanly terminate.β Verification: Ran
pip install -e ".[ai,dev]"and verified withpython -m pytest tests/test_pdf_security.pythat timeouts and secure flags function correctly without regressions.PR created automatically by Jules for task 17652427337237090992 started by @anchapin
Summary by Sourcery
Harden LaTeX-based PDF generation against remote code execution and hangs by securing subprocess invocations for pdflatex and pandoc.
Bug Fixes:
Documentation: