diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 3a1d237..54766d4 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,7 @@ **Vulnerability:** The `CoverLetterGenerator` used a standard Jinja2 environment (intended for HTML/XML or plain text) to render LaTeX templates. This allowed malicious user input (or AI hallucinations) containing LaTeX control characters (e.g., `\input{...}`) to be injected directly into the LaTeX source, leading to potential Local File Inclusion (LFI) or other exploits. **Learning:** Jinja2's default `autoescape` is context-aware based on file extensions, but usually only for HTML/XML. It does NOT automatically escape LaTeX special characters. Relying on manual filters (like `| latex_escape`) in templates is error-prone and brittle, as developers might forget to apply them to every variable. **Prevention:** Always use a dedicated Jinja2 environment for LaTeX generation that enforces auto-escaping via a `finalize` hook (e.g., `tex_env.finalize = latex_escape`). This ensures *all* variable output is sanitized by default, providing defense-in-depth even if the template author forgets explicit filters. +## 2026-04-28 - [High] RCE and DoS in Subprocess PDF Compilation +**Vulnerability:** The `PDFConverter` and `CoverLetterGenerator` modules used `subprocess.Popen` to execute `pdflatex` and `pandoc` without the `-no-shell-escape` flag, allowing arbitrary command execution if malicious LaTeX was compiled. Additionally, the `communicate()` calls lacked timeouts, allowing a malicious payload to cause a Denial of Service (DoS) via an infinite compilation loop. +**Learning:** Security fixes applied to a core module (like `TemplateGenerator`) often leave identical vulnerabilities in duplicated or extracted code (like `PDFConverter` and `CoverLetterGenerator`). All instances of external command execution must be hardened uniformly. +**Prevention:** Always search the entire codebase for similar usages of vulnerable patterns (e.g., `grep -rn 'subprocess.Popen' .`) when fixing an issue to ensure comprehensive coverage. Always enforce timeouts and explicit process cleanup when using `subprocess`. diff --git a/cli/generators/cover_letter_generator.py b/cli/generators/cover_letter_generator.py index aaf0b61..4cdccba 100644 --- a/cli/generators/cover_letter_generator.py +++ b/cli/generators/cover_letter_generator.py @@ -771,12 +771,16 @@ def _compile_pdf(self, output_path: Path, tex_content: str) -> bool: try: # Use Popen with explicit cleanup to avoid double-free issues process = subprocess.Popen( - ["pdflatex", "-interaction=nonstopmode", tex_path.name], + ["pdflatex", "-interaction=nonstopmode", "-no-shell-escape", tex_path.name], stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=tex_path.parent, ) - stdout, stderr = process.communicate() + try: + stdout, stderr = process.communicate(timeout=30) + except subprocess.TimeoutExpired: + process.kill() + stdout, stderr = process.communicate() if process.returncode == 0 or output_path.exists(): pdf_created = True except (subprocess.CalledProcessError, FileNotFoundError): @@ -787,11 +791,22 @@ def _compile_pdf(self, output_path: Path, tex_content: str) -> bool: # Fallback to pandoc try: process = subprocess.Popen( - ["pandoc", str(tex_path), "-o", str(output_path), "--pdf-engine=xelatex"], + [ + "pandoc", + str(tex_path), + "-o", + str(output_path), + "--pdf-engine=xelatex", + "--pdf-engine-opt=-no-shell-escape", + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) - stdout, stderr = process.communicate() + try: + stdout, stderr = process.communicate(timeout=30) + except subprocess.TimeoutExpired: + process.kill() + stdout, stderr = process.communicate() if process.returncode == 0 or output_path.exists(): pdf_created = True except (subprocess.CalledProcessError, FileNotFoundError): diff --git a/cli/pdf/converter.py b/cli/pdf/converter.py index 0b0a200..b02d55e 100644 --- a/cli/pdf/converter.py +++ b/cli/pdf/converter.py @@ -86,12 +86,16 @@ def _compile_pdflatex( """ try: process = subprocess.Popen( - ["pdflatex", "-interaction=nonstopmode", tex_path.name], + ["pdflatex", "-interaction=nonstopmode", "-no-shell-escape", tex_path.name], stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=working_dir, ) - stdout, stderr = process.communicate() + try: + stdout, stderr = process.communicate(timeout=30) + except subprocess.TimeoutExpired: + process.kill() + stdout, stderr = process.communicate() if process.returncode == 0 or output_path.exists(): return True @@ -121,12 +125,23 @@ def _compile_pandoc( """ try: process = subprocess.Popen( - ["pandoc", str(tex_path), "-o", str(output_path), "--pdf-engine=xelatex"], + [ + "pandoc", + str(tex_path), + "-o", + str(output_path), + "--pdf-engine=xelatex", + "--pdf-engine-opt=-no-shell-escape", + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=working_dir, ) - stdout, stderr = process.communicate() + try: + stdout, stderr = process.communicate(timeout=30) + except subprocess.TimeoutExpired: + process.kill() + stdout, stderr = process.communicate() if process.returncode == 0 or output_path.exists(): return True diff --git a/tests/test_pdf_security.py b/tests/test_pdf_security.py index d040cc6..58dedb6 100644 --- a/tests/test_pdf_security.py +++ b/tests/test_pdf_security.py @@ -4,6 +4,8 @@ from unittest.mock import MagicMock, patch from cli.generators.template import TemplateGenerator +from cli.generators.cover_letter_generator import CoverLetterGenerator +from cli.pdf.converter import PDFConverter class TestPDFSecurity(unittest.TestCase): @@ -55,6 +57,77 @@ def test_pdflatex_arguments(self, mock_popen): self.assertIn("-interaction=nonstopmode", command) self.assertIn("pdflatex", command) + @patch("cli.pdf.converter.subprocess.Popen") + def test_converter_pdflatex_timeout_and_args(self, mock_popen): + process_mock = MagicMock() + process_mock.communicate.side_effect = [ + subprocess.TimeoutExpired(cmd="pdflatex", timeout=30), + (b"", b""), + ] + process_mock.returncode = 0 + mock_popen.return_value = process_mock + + converter = PDFConverter() + + with patch.object(Path, "exists", return_value=True): + converter._compile_pdflatex(Path("output.tex"), Path("output.pdf"), Path(".")) + + process_mock.kill.assert_called_once() + process_mock.communicate.assert_any_call(timeout=30) + + args, _ = mock_popen.call_args + command = args[0] + self.assertIn("-no-shell-escape", command) + self.assertIn("-interaction=nonstopmode", command) + self.assertIn("pdflatex", command) + + @patch("cli.pdf.converter.subprocess.Popen") + def test_converter_pandoc_timeout_and_args(self, mock_popen): + process_mock = MagicMock() + process_mock.communicate.side_effect = [ + subprocess.TimeoutExpired(cmd="pandoc", timeout=30), + (b"", b""), + ] + process_mock.returncode = 0 + mock_popen.return_value = process_mock + + converter = PDFConverter() + + with patch.object(Path, "exists", return_value=True): + converter._compile_pandoc(Path("output.tex"), Path("output.pdf"), Path(".")) + + process_mock.kill.assert_called_once() + process_mock.communicate.assert_any_call(timeout=30) + + args, _ = mock_popen.call_args + command = args[0] + self.assertIn("--pdf-engine-opt=-no-shell-escape", command) + self.assertIn("pandoc", command) + + @patch("subprocess.Popen") + def test_cover_letter_pdflatex_timeout_and_args(self, mock_popen): + process_mock = MagicMock() + process_mock.communicate.side_effect = [ + subprocess.TimeoutExpired(cmd="pdflatex", timeout=30), + (b"", b""), + ] + process_mock.returncode = 0 + mock_popen.return_value = process_mock + + generator = CoverLetterGenerator() + + with patch.object(Path, "exists", return_value=True): + generator._compile_pdf(Path("output.pdf"), "content") + + process_mock.kill.assert_called_once() + process_mock.communicate.assert_any_call(timeout=30) + + args, _ = mock_popen.call_args + command = args[0] + self.assertIn("-no-shell-escape", command) + self.assertIn("-interaction=nonstopmode", command) + self.assertIn("pdflatex", command) + if __name__ == "__main__": unittest.main()