🛡️ Sentinel: [CRITICAL] Fix pdflatex RCE vulnerability via missing no-shell-escape flag#284
🛡️ Sentinel: [CRITICAL] Fix pdflatex RCE vulnerability via missing no-shell-escape flag#284
Conversation
…-shell-escape flag Adds missing `-no-shell-escape` flags and 30-second timeouts to secondary PDF compilation processes (`cli/pdf/converter.py` and `cli/generators/cover_letter_generator.py`) to prevent Remote Code Execution (RCE) and Denial of Service (DoS) vulnerabilities from untrusted LaTeX inputs. 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 GuideAdds hardened LaTeX/PDF compilation by enforcing no-shell-escape flags and timeouts for pdflatex/pandoc in secondary PDF generators, and documents the vulnerability/fix in the Sentinel security log. Sequence diagram for hardened CoverLetterGenerator PDF compilationsequenceDiagram
actor User
participant CoverLetterGenerator
participant pdflatex_process
participant pandoc_process
participant FileSystem
User->>CoverLetterGenerator: request_pdf_generation()
CoverLetterGenerator->>FileSystem: write_tex_file()
CoverLetterGenerator->>pdflatex_process: Popen pdflatex -interaction=nonstopmode -no-shell-escape
alt pdflatex completes within 30s
CoverLetterGenerator->>pdflatex_process: communicate timeout=30
pdflatex_process-->>CoverLetterGenerator: stdout, stderr, returncode
alt pdflatex success or output_path exists
CoverLetterGenerator->>CoverLetterGenerator: pdf_created = True
CoverLetterGenerator-->>User: return True
else pdflatex failure
CoverLetterGenerator->>pandoc_process: Popen pandoc --pdf-engine=xelatex --pdf-engine-opt=-no-shell-escape
alt pandoc completes within 30s
CoverLetterGenerator->>pandoc_process: communicate timeout=30
pandoc_process-->>CoverLetterGenerator: stdout, stderr, returncode
alt pandoc success or output_path exists
CoverLetterGenerator->>CoverLetterGenerator: pdf_created = True
CoverLetterGenerator-->>User: return True
else pandoc failure
CoverLetterGenerator-->>User: return False
end
else pandoc timeout
CoverLetterGenerator->>pandoc_process: kill()
CoverLetterGenerator->>pandoc_process: communicate()
CoverLetterGenerator-->>User: return False
end
end
else pdflatex timeout
CoverLetterGenerator->>pdflatex_process: kill()
CoverLetterGenerator->>pdflatex_process: communicate()
CoverLetterGenerator-->>User: return False
end
Flow diagram for secure LaTeX compilation subprocess patternflowchart TD
A[start_subprocess_with_latex_command] --> B[set_command_flags_include_no_shell_escape]
B --> C[Popen_with_stdout_stderr_pipes_and_working_directory]
C --> D[communicate_with_timeout_30s]
D -->|completed_before_timeout| E[check_returncode_or_output_file_exists]
D -->|TimeoutExpired| F[call_kill_on_process]
F --> G[communicate_to_drain_pipes]
G --> H[return_False]
E -->|success| I[return_True]
E -->|failure| H[return_False]
File-Level Changes
Possibly linked issues
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 new timeout/
Popenhandling is duplicated in multiple places; consider extracting a shared helper (e.g.,run_pdflatex/run_pandocor a genericrun_with_timeout) to centralize the flags, timeout, and cleanup logic. - Right now
stdout/stderrfrom the LaTeX and pandoc processes are captured and then discarded; consider logging them or surfacing minimal error information to aid debugging when compilation or timeouts occur. - The 30-second timeout is hardcoded in several calls; consider defining a single configuration constant for PDF compilation timeouts so it’s easier to tune and keep consistent across the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new timeout/`Popen` handling is duplicated in multiple places; consider extracting a shared helper (e.g., `run_pdflatex`/`run_pandoc` or a generic `run_with_timeout`) to centralize the flags, timeout, and cleanup logic.
- Right now `stdout`/`stderr` from the LaTeX and pandoc processes are captured and then discarded; consider logging them or surfacing minimal error information to aid debugging when compilation or timeouts occur.
- The 30-second timeout is hardcoded in several calls; consider defining a single configuration constant for PDF compilation timeouts so it’s easier to tune and keep consistent across the codebase.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🚨 Severity: CRITICAL
💡 Vulnerability: Secondary PDF compilation processes (
cli/pdf/converter.pyandCoverLetterGenerator) failed to include the-no-shell-escapeflag forpdflatex(and--pdf-engine-opt=-no-shell-escapeforpandoc) and lacked execution timeouts, leading to potential Remote Code Execution (RCE) via LaTeX injections and Denial of Service (DoS).🎯 Impact: An attacker could execute arbitrary commands on the host machine by injecting malicious LaTeX code into the PDF generation pipeline if the inputs weren't properly sanitized. Additionally, crafted inputs could cause infinite compilation loops, exhausting server resources.
🔧 Fix: Added
-no-shell-escapearguments topdflatexand--pdf-engine-opt=-no-shell-escapetopandoc. Added explicit 30-second timeouts and proper process cleanup usingsubprocess.TimeoutExpiredhandling.✅ Verification: Ran Bandit which resulted in no issues found (except a known false positive). Executed full test suite (
python -m pytest tests/) which passed, including the security tests intest_pdf_security.py. Verified the changes viaread_fileinspection.PR created automatically by Jules for task 7283270687554969405 started by @anchapin
Summary by Sourcery
Harden PDF generation against LaTeX-based RCE and resource exhaustion and document the incident in Sentinel.
Bug Fixes:
Enhancements:
Documentation: