fix: mitigate CVE-2024-4367 FontMatrix code injection in pdf viewer

[DeepDiver1975]

Summary

Mitigates CVE-2024-4367 (FontMatrix code injection) in the bundled pdf.js. A missing type check on the PDF /FontMatrix attribute allowed an attacker-controlled string to flow into new Function() in the glyph renderer, executing JavaScript in the viewer origin (XSS → account takeover through a public share / attachment popup).

The pre-existing isEvalSupported=false hardening was bypassable: in workersrc.js it was set after a locale lookup (parent.OC.getLocale()) inside a single try with a bare catch. When the viewer runs top-level (the ?file=blob popup) parent.OC can throw, the catch swallows it, and eval stays enabled. The viewer CSP also explicitly allowed unsafe-eval.

Changes (defense in depth)

• pdf.worker.js — backport the upstream type check: a /FontMatrix that is not exactly six numbers is replaced by the identity matrix.
• pdf.js — coerce every glyph-command argument to a number before it is interpolated into new Function() (the intentional scale identifiers are preserved).
• workersrc.js — set isEvalSupported=false and enableScripting first and unconditionally; isolate the throwing locale lookup; detect the blob popup by parsing the file query parameter for a blob: URL instead of a loose indexOf('?file=blob') substring match (which was satisfiable from the attacker-controlled URL hash).
• DisplayController — drop allowEvalScript(true) so the new Function sink is unreachable in the browser.

Tests

Adds tests/security/ with the PoC PDF (benign marker payload), node verification harnesses for both source guards, and a blob-popup detection test. All pass locally.

Follow-up

This backports the CVE fix into the current pdf.js 3.15.2 bundle. Upgrading the bundle to pdf.js ≥ 4.2.67 remains a recommended larger follow-up.

Fixes OC10-111

🤖 Generated with Claude Code

[jvillafanez]

I guess we want to set the value of enableScripting, so we aren't interested in the error. Anyway, it might be better to include a comment to ensure it's intentional

[jvillafanez]

It might be better to move this line with the rest of the PDFViewerApplicationOptions.set(...) sentences.

[DeepDiver1975]

Code Review: CVE-2024-4367 FontMatrix code injection

Overview

Backports the CVE-2024-4367 fix into the bundled pdf.js 3.15.2 using defense in depth across four layers: the worker /FontMatrix type check, the glyph-command numeric coercion in pdf.js, the fail-safe isEvalSupported=false ordering + blob-popup detection in workersrc.js, and allowEvalScript(false) in the CSP. The layered approach is well-judged — any single layer holds if the others regress.

I verified the scale special-case is correct: it is the only glyph command that legitimately emits non-numeric args (["size","-size"] at pdf.worker.js:54810); every other emitter (transform, translate, moveTo, …) passes numeric arrays. The worker guard sits at the case 73 join point after all font-loading branches, so it runs on every path.

Strengths

• Correct root cause + sink coverage. Guard A stops the bad matrix at the source; Guard B independently neutralizes the new Function() sink even if Guard A were bypassed. verify-cve-2024-4367.mjs explicitly tests the "Guard A failed" scenario (B1–B4) — the right adversarial framing.
• Real bypass fixed, not just the CVE. The #?file=blob hash-fragment observation is a genuine find — the old substring check was satisfiable from the attacker-controlled fragment.
• Fail-safe ordering. Moving isEvalSupported=false ahead of the throwing locale lookup correctly fixes the swallowed-exception bypass.

Issues & Suggestions

Medium — .mjs security tests aren't wired into CI and duplicate the implementation. tests/security/*.mjs are not referenced by the Makefile or any workflow, so they only run on a manual node ... invocation. They also re-declare copies of sanitizeFontMatrix, buildBody, and isBlobDownloadPopup rather than importing the real code — so the shipped bundle can regress while the tests stay green. These are genuine regression guards for a security fix; they should run automatically, and the README should acknowledge the reproduce-not-exercise drift risk.

Low — isBlobDownloadPopup() can throw in deferredViewerConfig. decodeURIComponent() throws URIError on a malformed %-sequence (e.g. ?file=%). In redirectIfNotDisplayedInFrame the call is inside try/catch and fails safe, but the call in the close-button block (workersrc.js:144) is outside any try. A malformed URL would abort deferredViewerConfig there — after the security options are set (no security impact) but before workerSrc/locale. Wrapping the decodeURIComponent inside isBlobDownloadPopup (return false on throw) makes it robust everywhere.

Low — style nits. workersrc.js still ends without a trailing newline. The intentional empty catch (e) {} blocks would benefit from a one-word // best-effort marker so a future reader doesn't "fix" them.

Security assessment

The core fix is sound and correctly targets the CVE; the three independent browser-side layers plus the CSP change make eval-based glyph injection unreachable through every path I traced. The #?file=blob frame-escape hardening is a valuable bonus. My only substantive concern is test durability: the strongest verification currently doesn't run in CI and can silently drift from the shipped bundle.

Verdict

Approve with minor follow-ups. The security change is correct and well-layered; nothing blocks merge. Recommend wiring the security tests into CI (or documenting them as manual-only + drift-prone) and optionally hardening the decodeURIComponent call. Upgrading pdf.js to ≥ 4.2.67 remains the right long-term follow-up.

🤖 Generated with Claude Code

[DeepDiver1975]

Code review

Reviewed at head SHA a57d8fe against the repo and the core CSP classes.

Overview

Solid, well-reasoned backport of the CVE-2024-4367 fix (FontMatrix → new Function() code injection) into the bundled pdf.js 3.15.2, with genuine defense-in-depth across four independent layers:

1. Worker (pdf.worker.js) — authoritative upstream fix: reject a /FontMatrix that isn't exactly six numbers, falling back to the identity matrix.
2. Sink (pdf.js) — coerce non-numeric glyph-command args to 0 before interpolation into the generated function body.
3. Fail-safe eval disabling (workersrc.js) — set isEvalSupported=false first/unconditionally and harden the frame-escape check.
4. CSP (DisplayController) — allowEvalScript(false) so the sink is unreachable in-browser regardless of the JS.

Verified

• Action SHAs are correct and current. Both actions/checkout@9c091bb… and actions/setup-node@82076278… resolve to v7.0.0, which is also releases/latest. ✅
• CSP comment claim is accurate. lib/public/AppFramework/Http/ContentSecurityPolicy.php has protected $evalScriptAllowed = true;, so the controller's ContentSecurityPolicy instance really does default to unsafe-eval. Explicitly calling allowEvalScript(false) (rather than just dropping the old true) is genuinely required. ✅
• Blob-popup detection matches the real popup format. web/viewer.js opens "?file=" + encodeURIComponent(blobUrl + "#" + filename), so the entire blob:...#name lands in location.search. The new parser decodes file= and checks indexOf('blob:') === 0 — correct, and it fails closed on the attacker #?file=blob hash trick. ✅

Code quality

• Thoroughly commented with the why — rare and valuable for a security backport. The enableScripting read is isolated so a DOM hiccup can't leave scripting in an unknown state, and the locale lookup (the original swallowed-exception culprit) is now in its own try after the security options are set.
• The layers are genuinely independent: even if the very first set('isEvalSupported', false) threw, layer 4 (CSP unsafe-eval off) still makes new Function unreachable.
• The scale-command exception in the sink is safe: scale is emitted with literal ["size","-size"] identifiers, never from font-controlled data.

Minor / non-blocking

• Test reproductions drift, honestly disclosed. verify-cve-2024-4367.mjs re-declares the guard logic instead of importing the minified bundle; the S1–S4 source tripwire mitigates this. S2's regex is brittle to reformatting and could false-fail on any future bundle re-minification even if the guard is intact — worth noting.
• test-blob-popup-detection.mjs isn't a faithful copy — its local isBlobDownloadPopup omits the shipped try/catch around decodeURIComponent, so a regression in the malformed-%-sequence handling wouldn't be caught. Consider copying that branch or adding a ?file=%E0%A4%A malformed case.
• PoC PDF is a binary blob — benign payload (documented), but a note on how it was generated (or a generator script) would help future maintainers.
• Nit: the CVE regression CI job has no needs: dependency; fine functionally.

Risk

Low. Disabling eval doesn't regress rendering (pdf.js falls back to the path-based glyph generator). The one behavioral change worth a manual smoke test is the stricter frame-escape guard: confirm the legitimate attachment popup still opens and a normal PDF renders with eval disabled — the README's E2E step covers this.

🤖 Generated with Claude Code