From 50d12c6aba53f88849b4c97b82978132151e64a1 Mon Sep 17 00:00:00 2001 From: SnapOtter Date: Sat, 1 Aug 2026 14:54:56 +0800 Subject: [PATCH] fix(doc-engine): keep protect-pdf passwords out of qpdf's argv (#708) qpdf expands argument files before parsing options, so a bare positional password beginning with @ was resolved as a path and the file's contents became the encryption key: exit 0, and the user's own password no longer opened the PDF. Drive the encrypt through a job-JSON file so neither password reaches argv. The =-joined flag form needs qpdf 11.7+, and the released image carries 11.3.0 which rejects it; job JSON works on both (verified 11.3.0 and 12.1.0, R = 6 each). --- packages/doc-engine/src/pdf-ops.ts | 52 ++++++++++++++--- packages/doc-engine/tests/pdf-ops.test.ts | 57 +++++++++++++++---- .../tools/document/protect-pdf.test.ts | 49 ++++++++++++++++ 3 files changed, 139 insertions(+), 19 deletions(-) diff --git a/packages/doc-engine/src/pdf-ops.ts b/packages/doc-engine/src/pdf-ops.ts index f21446ec..ef8c6e91 100644 --- a/packages/doc-engine/src/pdf-ops.ts +++ b/packages/doc-engine/src/pdf-ops.ts @@ -1,3 +1,7 @@ +import { randomUUID } from "node:crypto"; +import { rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { runQpdf } from "./qpdf.js"; // qpdf page ranges: digits, commas, hyphens, r-prefixed (r1 = last), and z (last page). @@ -41,14 +45,31 @@ export async function qpdfRotate( } /* - * Security note: passwords are passed as argv elements to spawn() (no shell). - * They are visible in /proc//cmdline for the ~1s process lifetime. This - * is acceptable for the single-tenant container threat model. If multi-tenant - * isolation is ever needed, switch to qpdf's --password-file or @argfile - * syntax with a 0600 temp file in the scratch dir, deleted in a finally block. + * Security note: qpdfEncrypt passes its passwords through a 0600 job-JSON file that + * is unlinked in a finally block, so they stay out of argv. qpdfDecrypt still passes + * one as an argv element to spawn() (no shell), visible in /proc//cmdline for + * the ~1s process lifetime, which is acceptable for the single-tenant container + * threat model. Its `--password=` form is a single token, so the argument-file + * pre-pass described below cannot fire on it. */ -/** AES-256 encrypt with user + owner passwords (qpdf --encrypt user owner 256 --). */ +/** + * AES-256 encrypt with user + owner passwords, via a qpdf job-JSON file. + * + * The passwords deliberately never appear in argv. qpdf runs an argument-file + * pre-pass over every argv element before it parses options, so a bare positional + * password beginning with `@` is resolved as a path and that file's lines are + * spliced into qpdf's own argv. A single-line file then encrypts the document under + * that file's contents at exit 0, handing the user a PDF their own password does not + * open, and a multi-line one shifts the arguments enough to surface part of the file + * in qpdf's error text. + * + * The `=`-joined flag form (`--encrypt --user-password=...`) also avoids this, but + * only on qpdf 11.7 and newer; the shipped image carries 11.3, which rejects it + * outright. Job JSON is accepted by both (verified against 11.3.0 and 12.1.0) and + * has the side benefit of keeping the passwords out of /proc//cmdline, which + * the note above asks for. + */ export async function qpdfEncrypt( inputPath: string, userPassword: string, @@ -57,10 +78,23 @@ export async function qpdfEncrypt( ): Promise { assertPassword(userPassword); assertPassword(ownerPassword); - await runQpdf( - [inputPath, "--encrypt", userPassword, ownerPassword, "256", "--", outPath], - 60_000, + + const jobPath = join(tmpdir(), `snapotter-qpdf-job-${randomUUID()}.json`); + await writeFile( + jobPath, + JSON.stringify({ + inputFile: inputPath, + outputFile: outPath, + encrypt: { userPassword, ownerPassword, "256bit": {} }, + }), + { mode: 0o600 }, ); + + try { + await runQpdf([`--job-json-file=${jobPath}`], 60_000); + } finally { + await rm(jobPath, { force: true }); + } } /** Decrypt with a known password; qpdf rejects wrong passwords with exit 2. */ diff --git a/packages/doc-engine/tests/pdf-ops.test.ts b/packages/doc-engine/tests/pdf-ops.test.ts index 0c6510f1..1bd293e3 100644 --- a/packages/doc-engine/tests/pdf-ops.test.ts +++ b/packages/doc-engine/tests/pdf-ops.test.ts @@ -1,5 +1,8 @@ import { spawn } from "node:child_process"; +import { existsSync } from "node:fs"; +import { readFile } from "node:fs/promises"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { settleClose } from "./helpers/fake-child.js"; import { makeSpawnHelpers } from "./helpers/spawn-capture.js"; vi.mock("node:child_process", () => ({ spawn: vi.fn() })); @@ -137,20 +140,54 @@ describe("qpdfRotate", () => { }); describe("qpdfEncrypt", () => { - it("builds --encrypt 256 -- ", async () => { + it("drives qpdf through a job-JSON file rather than argv", async () => { h.nextClose({ code: 0 }); await import("../src/pdf-ops.js").then((m) => m.qpdfEncrypt("/in.pdf", "userpw", "ownerpw", "/out.pdf"), ); - expect(h.lastArgs()).toEqual([ - "/in.pdf", - "--encrypt", - "userpw", - "ownerpw", - "256", - "--", - "/out.pdf", - ]); + const args = h.lastArgs(); + expect(args).toHaveLength(1); + expect(args[0]).toMatch(/^--job-json-file=.*\.json$/); + }); + + // qpdf runs an argument-file pre-pass over every argv element before it parses + // options, so a bare positional value starting with `@` is read as a path and that + // file's lines are spliced into qpdf's own argv. A one-line file then encrypts the + // document under the file's contents, at exit 0, instead of the chosen password. + // Keeping passwords out of argv entirely is what closes it. + it.each([ + ["argument-file sigil", "@/etc/hostname"], + ["leading dashes", "--allow-insecure"], + ])("never puts a password with %s into argv", async (_label, password) => { + h.nextClose({ code: 0 }); + await import("../src/pdf-ops.js").then((m) => + m.qpdfEncrypt("/in.pdf", password, "ownerpw", "/out.pdf"), + ); + expect(JSON.stringify(h.lastArgs())).not.toContain(password); + }); + + it("writes the literal passwords into the job file and removes it afterwards", async () => { + // A child that never settles on its own, so the job file can be read while qpdf + // is notionally still running. It is unlinked once the promise resolves. + const child = h.nextManual(); + const { qpdfEncrypt } = await import("../src/pdf-ops.js"); + const pending = qpdfEncrypt("/in.pdf", "@/etc/hostname", "ownerpw", "/out.pdf"); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalled()); + const jobPath = h.lastArgs()[0].replace("--job-json-file=", ""); + const contents = JSON.parse(await readFile(jobPath, "utf8")); + + settleClose(child, { code: 0 }); + await pending; + + expect(contents.inputFile).toBe("/in.pdf"); + expect(contents.outputFile).toBe("/out.pdf"); + expect(contents.encrypt).toMatchObject({ + userPassword: "@/etc/hostname", + ownerPassword: "ownerpw", + "256bit": {}, + }); + expect(existsSync(jobPath)).toBe(false); }); it("rejects an empty user password before spawning", async () => { diff --git a/tests/integration/tools/document/protect-pdf.test.ts b/tests/integration/tools/document/protect-pdf.test.ts index 78b85628..5ea73c6b 100644 --- a/tests/integration/tools/document/protect-pdf.test.ts +++ b/tests/integration/tools/document/protect-pdf.test.ts @@ -1,3 +1,7 @@ +import { randomUUID } from "node:crypto"; +import { rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { qpdfAvailable } from "@snapotter/doc-engine"; import { eq } from "drizzle-orm"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; @@ -80,4 +84,49 @@ describe.skipIf(!qpdfAvailable())("protect-pdf (requires qpdf)", () => { const res = await runTool({ userPassword: "" }); expect(res.statusCode).toBe(400); }, 60_000); + + // Round-trips a password whose first character is qpdf's argument-file sigil. + // Passed as a bare positional, qpdf resolves it as a path and encrypts under + // that file's contents instead, so the user's own password no longer opens the + // document. Unlocking with the literal string is what proves it stayed literal. + it("treats a password starting with the argument-file sigil as a literal", async () => { + const canaryPath = join(tmpdir(), `snapotter-qpdf-canary-${randomUUID()}.txt`); + await writeFile(canaryPath, "CANARY_FILE_CONTENTS\n"); + + try { + const literalPassword = `@${canaryPath}`; + const res = await runTool({ userPassword: literalPassword }); + expect(res.statusCode).toBe(200); + + const dl = await testApp.app.inject({ + method: "GET", + url: JSON.parse(res.body).downloadUrl, + }); + expect(dl.statusCode).toBe(200); + + const unlockWith = async (password: string) => { + const { body, contentType } = createMultipartPayload([ + { + name: "file", + filename: "locked.pdf", + contentType: "application/pdf", + content: dl.rawPayload, + }, + { name: "settings", content: JSON.stringify({ password }) }, + ]); + return testApp.app.inject({ + method: "POST", + url: "/api/v1/tools/pdf/unlock-pdf", + headers: { authorization: `Bearer ${adminToken}`, "content-type": contentType }, + body, + }); + }; + + expect((await unlockWith(literalPassword)).statusCode).toBe(200); + // The canary's contents must never have become the encryption key. + expect((await unlockWith("CANARY_FILE_CONTENTS")).statusCode).not.toBe(200); + } finally { + await rm(canaryPath, { force: true }); + } + }, 90_000); });