Merge pull request #21620 from calixteman/improve_modif_detections

Improve the detection of the changes made after a document has been signed

src/core/document.js

@@ -81,6 +81,7 @@ import { XFAFactory } from "./xfa/factory.js";
 import { XRef } from "./xref.js";
 
 const LETTER_SIZE_MEDIABOX = [0, 0, 612, 792];
+const SIGNATURE_TAIL_CHUNK_SIZE = 65536;
 
 class Page {
   #resourcesPromise = null;
@@ -1004,11 +1005,10 @@ function find(stream, signature, limit = 1024, backwards = false) {
 class PDFDocument {
   #pagePromises = new Map();
 
-  // Map<id, {data: Uint8Array[2], pkcs7: Uint8Array}> — populated by the
+  // Map<id, {byteRange: number[4], pkcs7: Uint8Array}> — populated by the
   // `signatures` getter, consumed by `getSignatureData`. We deliberately
-  // keep the byte payload out of the metadata array so it doesn't ride
-  // the worker→main `postMessage` boundary unless the viewer actually
-  // asks to verify (one shot per signature).
+  // keep the signed byte spans out of the metadata array and only slice
+  // them out of the stream when the viewer actually asks to verify.
   #signatureData = null;
 
   #version = null;
@@ -2026,12 +2026,47 @@ class PDFDocument {
     }
   }
 
-  // The /ByteRange of a signature that covers the whole document usually
-  // ends a few bytes before the file's actual end — the trailer,
-  // `startxref` offset and `%%EOF` marker are conventionally left outside
-  // the signed range. 100 bytes covers the worst case (large
-  // `startxref` offsets, optional whitespace) without false positives.
-  static #WHOLE_DOCUMENT_TAIL_FUZZ = 100;
+  async #getByteRange(begin, end) {
+    try {
+      return this.stream.getByteRange(begin, end);
+    } catch (ex) {
+      if (!(ex instanceof MissingDataException)) {
+        throw ex;
+      }
+      await this.pdfManager.requestRange(begin, end);
+      return this.#getByteRange(begin, end);
+    }
+  }
+
+  async #coversWholeDocument(signedEnd, modificationsAfterSignature) {
+    if (modificationsAfterSignature > 0) {
+      return false;
+    }
+
+    const fileLength = this.stream.end;
+    for (
+      let begin = signedEnd;
+      begin < fileLength;
+      begin += SIGNATURE_TAIL_CHUNK_SIZE
+    ) {
+      const end = Math.min(begin + SIGNATURE_TAIL_CHUNK_SIZE, fileLength);
+      const tail = await this.#getByteRange(begin, end);
+
+      for (const byte of tail) {
+        if (
+          byte !== 0x00 && // null
+          byte !== 0x09 && // horizontal tab
+          byte !== 0x0a && // line feed
+          byte !== 0x0c && // form feed
+          byte !== 0x0d && // carriage return
+          byte !== 0x20 // space
+        ) {
+          return false;
+        }
+      }
+    }
+    return true;
+  }
 
   #parseSignatureDict(field, sigDict, fieldRef) {
     const byteRange = sigDict.get("ByteRange");
@@ -2075,15 +2110,12 @@ class PDFDocument {
     if (
       a !== 0 ||
       b <= 0 ||
-      d < 0 ||
       a + b > c ||
       c + d > fileLength ||
       fileLength === 0
     ) {
       return null;
     }
-    const data = [stream.getByteRange(a, a + b), stream.getByteRange(c, c + d)];
-
     const pkcs7 = stringToBytes(contents);
 
     const t = field.get("T");
@@ -2097,13 +2129,6 @@ class PDFDocument {
     const refKey = fieldRef instanceof Ref ? fieldRef.toString() : "inline";
     const id = `${refKey}:${a}-${b}-${c}-${d}`;
 
-    // A signature "covers the whole document" iff the gap between the
-    // last signed byte and EOF is no more than the conventional
-    // trailer-slack window. Compare on the gap (not on the absolute
-    // lastSignedByte) so a tiny file with a tiny ByteRange isn't
-    // mis-flagged as full-coverage.
-    const tailGap = fileLength - (c + d);
-
     return {
       id,
       fieldName,
@@ -2119,11 +2144,8 @@ class PDFDocument {
       signatureType,
       byteRange,
       pkcs7,
-      data,
       revisionIndex: 0,
       parentId: null,
-      coversWholeDocument:
-        tailGap >= 0 && tailGap <= PDFDocument.#WHOLE_DOCUMENT_TAIL_FUZZ,
     };
   }
 
@@ -2143,6 +2165,18 @@ class PDFDocument {
         const collected = [];
         this.#collectSignatureFields(fields, collected, new RefSet());
 
+        await Promise.all(
+          collected.map(async signature => {
+            const signedEnd = signature.byteRange[2] + signature.byteRange[3];
+            signature.modificationsAfterSignature =
+              this.xref.countUpdatesAfter?.(signedEnd) ?? null;
+            signature.coversWholeDocument = await this.#coversWholeDocument(
+              signedEnd,
+              signature.modificationsAfterSignature
+            );
+          })
+        );
+
         // Group sub-signatures by ByteRange containment: outer revision is
         // the largest covering signature (largest c + d). Sort descending,
         // then point each later signature at the smallest enclosing parent
@@ -2165,14 +2199,14 @@ class PDFDocument {
             }
           }
         }
-        // Split bytes (`data`, `pkcs7`) out of the metadata so the array
-        // we ship to the main thread stays small. The viewer fetches the
-        // bytes on demand via `getSignatureData(id)`, one signature at a
-        // time, only when verification is about to run.
+        // Keep the PKCS#7 blob and byte-range information worker-side so the
+        // metadata array stays small. The viewer fetches the signed bytes on
+        // demand via `getSignatureData(id)`, one signature at a time, only
+        // when verification is about to run.
         const signatureData = new Map();
         const metadata = collected.map(sig => {
-          const { data, pkcs7, ...rest } = sig;
-          signatureData.set(sig.id, { data, pkcs7 });
+          const { pkcs7, ...rest } = sig;
+          signatureData.set(sig.id, { byteRange: sig.byteRange, pkcs7 });
           return rest;
         });
         this.#signatureData = signatureData;
@@ -2185,7 +2219,17 @@ class PDFDocument {
   async getSignatureData(id) {
     // Ensure parsing is finished and `#signatureData` is populated.
     await this.signatures;
-    return this.#signatureData?.get(id) ?? null;
+    const signature = this.#signatureData?.get(id);
+    if (!signature) {
+      return null;
+    }
+    const { byteRange, pkcs7 } = signature;
+    const [a, b, c, d] = byteRange;
+    const data = await Promise.all([
+      this.#getByteRange(a, a + b),
+      this.#getByteRange(c, c + d),
+    ]);
+    return { data, pkcs7 };
   }
 
   get hasJSActions() {

src/core/xref.js

@@ -38,6 +38,9 @@ class XRef {
     this.pdfManager = pdfManager;
     this.entries = [];
     this._xrefStms = new Set();
+    this._xrefSectionOffsets = new Set();
+    this._xrefSectionsComplete = true;
+    this._parsedWithRecovery = false;
     this._cacheMap = new Map(); // Prepare the XRef cache.
     this._pendingRefs = new RefSet();
     this._newPersistentRefNum = null;
@@ -97,6 +100,7 @@ class XRef {
   }
 
   parse(recoveryMode = false) {
+    this._parsedWithRecovery = recoveryMode;
     let trailerDict;
     if (!recoveryMode) {
       trailerDict = this.readXRef();
@@ -769,6 +773,8 @@ class XRef {
           throw new FormatError("Invalid XRef stream header");
         }
 
+        this._xrefSectionOffsets.add(startXRef);
+
         // Recursively get previous dictionary, if any
         obj = dict.get("Prev");
         if (Number.isInteger(obj)) {
@@ -782,6 +788,7 @@ class XRef {
         if (e instanceof MissingDataException) {
           throw e;
         }
+        this._xrefSectionsComplete = false;
         info("(while reading XRef): " + e);
       }
       this.startXRefQueue.shift();
@@ -796,6 +803,23 @@ class XRef {
     throw new XRefParseException();
   }
 
+  countUpdatesAfter(offset) {
+    if (this._parsedWithRecovery || !this._xrefSectionsComplete) {
+      return null;
+    }
+    const relativeOffset = offset - this.stream.start;
+    let count = 0;
+    for (const sectionOffset of this._xrefSectionOffsets) {
+      if (
+        sectionOffset >= relativeOffset &&
+        !this._xrefStms.has(sectionOffset)
+      ) {
+        count++;
+      }
+    }
+    return count;
+  }
+
   getEntry(i) {
     const xrefEntry = this.entries[i];
     if (xrefEntry && !xrefEntry.free && xrefEntry.offset) {

test/pdfs/.gitignore

@@ -948,3 +948,4 @@
 !saslprep-r6.pdf
 !jpx_smaskindata.pdf
 !nonisolated_blend_smask.pdf
+!signed_verified.pdf

test/unit/api_spec.js

@@ -2143,6 +2143,7 @@ describe("api", function () {
           revisionIndex: 0,
           parentId: null,
           coversWholeDocument: true,
+          modificationsAfterSignature: 0,
         },
         {
           id: "49R:0-60285-74083-357914",
@@ -2159,6 +2160,7 @@ describe("api", function () {
           revisionIndex: 1,
           parentId: "70R:0-435961-437515-34098",
           coversWholeDocument: false,
+          modificationsAfterSignature: 1,
         },
       ]);
 
@@ -2198,6 +2200,81 @@ describe("api", function () {
       await loadingTask.destroy();
     });
 
+    it("gets signature metadata without fetching later revisions", async function () {
+      const baseData = await DefaultFileReaderFactory.fetch({
+        path: TEST_PDFS_PATH + "signed_verified.pdf",
+      });
+      const payload = new Uint8Array(2 * 65536).fill(0x41);
+      const objectHeader = stringToBytes(
+        `\n9 0 obj\n<< /Length ${payload.length} >>\nstream\n`
+      );
+      const objectFooter = stringToBytes("\nendstream\nendobj\n");
+      const objectOffset = baseData.length + 1;
+      const xrefOffset =
+        baseData.length +
+        objectHeader.length +
+        payload.length +
+        objectFooter.length;
+      const xref = stringToBytes(
+        "xref\n" +
+          "9 1\n" +
+          `${objectOffset.toString().padStart(10, "0")} 00000 n \n` +
+          "trailer\n" +
+          "<< /Size 10 /Root 1 0 R /Prev 10007 >>\n" +
+          "startxref\n" +
+          `${xrefOffset}\n` +
+          "%%EOF\n"
+      );
+      const data = new Uint8Array(
+        baseData.length +
+          objectHeader.length +
+          payload.length +
+          objectFooter.length +
+          xref.length
+      );
+      let position = 0;
+      for (const part of [
+        baseData,
+        objectHeader,
+        payload,
+        objectFooter,
+        xref,
+      ]) {
+        data.set(part, position);
+        position += part.length;
+      }
+
+      const initialData = new Uint8Array(data.subarray(0, 65536));
+      const transport = new PDFDataRangeTransport(data.length, initialData);
+      let fetches = 0;
+      transport.requestDataRange = (begin, end) => {
+        fetches++;
+        waitSome(() => {
+          transport.onDataRange(
+            begin,
+            new Uint8Array(data.subarray(begin, end))
+          );
+        });
+      };
+
+      const loadingTask = getDocument({
+        range: transport,
+        rangeChunkSize: 65536,
+        disableAutoFetch: true,
+        disableStream: true,
+      });
+      const pdfDoc = await loadingTask.promise;
+      const fetchesAfterLoading = fetches;
+      const signatures = await pdfDoc.getSignatures();
+
+      expect(signatures.length).toEqual(1);
+      expect(signatures[0].coversWholeDocument).toEqual(false);
+      expect(signatures[0].modificationsAfterSignature).toEqual(1);
+      expect(fetches).toEqual(fetchesAfterLoading);
+
+      await loadingTask.destroy();
+    });
+
     it("gets non-existent calculationOrder", async function () {
       const calculationOrder = await pdfDocument.getCalculationOrderIds();
       expect(calculationOrder).toEqual(null);

test/unit/document_spec.js

@@ -17,6 +17,7 @@ import { createIdFactory, XRefMock } from "./test_utils.js";
 import { Dict, Name, Ref } from "../../src/core/primitives.js";
 import { PDFDocument } from "../../src/core/document.js";
 import { StringStream } from "../../src/core/stream.js";
+import { XRef } from "../../src/core/xref.js";
 
 describe("document", function () {
   describe("Page", function () {
@@ -44,6 +45,35 @@ describe("document", function () {
     });
   });
 
+  describe("XRef", function () {
+    it("compares xref sections with absolute file offsets", function () {
+      const stream = new StringStream(" ".repeat(200));
+      stream.pos = 10;
+      stream.moveStart();
+
+      const xref = new XRef(stream, {});
+      xref._xrefSectionOffsets.add(100);
+
+      expect(xref.countUpdatesAfter(105)).toEqual(1);
+      expect(xref.countUpdatesAfter(111)).toEqual(0);
+    });
+
+    it("returns an unknown update count for an incomplete xref chain", function () {
+      const stream = new StringStream(
+        "xref\n" +
+          "0 1\n" +
+          "0000000000 65535 f \n" +
+          "trailer\n" +
+          "<< /Size 1 /Prev 999 >>\n"
+      );
+      const xref = new XRef(stream, {});
+      xref.setStartXRef(0);
+
+      expect(xref.readXRef()).toBeInstanceOf(Dict);
+      expect(xref.countUpdatesAfter(0)).toBeNull();
+    });
+  });
+
   describe("PDFDocument", function () {
     // Padded to 1024 bytes so signature ByteRange tests using offsets
     // like `[0, 100, 200, 800]` stay within `stream.end` (the new
@@ -51,7 +81,11 @@ describe("document", function () {
     // the file length).
     const stream = new StringStream("Dummy_PDF_data".padEnd(1024, " "));
 
-    function getDocument(acroForm, xref = new XRefMock()) {
+    function getDocument(
+      acroForm,
+      xref = new XRefMock(),
+      documentStream = stream
+    ) {
       const catalog = { acroForm };
       const pdfManager = {
         get docId() {
@@ -74,7 +108,7 @@ describe("document", function () {
           return { isOffscreenCanvasSupported: false };
         },
       };
-      const pdfDocument = new PDFDocument(pdfManager, stream);
+      const pdfDocument = new PDFDocument(pdfManager, documentStream);
       pdfDocument.xref = xref;
       pdfDocument.catalog = catalog;
 
@@ -283,6 +317,54 @@ describe("document", function () {
         expect(bytes.data.length).toEqual(2);
       });
 
+      it("reports whole-document coverage when only whitespace trails the signed range", async function () {
+        const acroForm = new Dict();
+        acroForm.set("SigFlags", 3);
+
+        const sigRef = Ref.get(40, 0);
+        const fieldRef = Ref.get(41, 0);
+        const sigDict = makeSigDict({ byteRange: [0, 20, 30, 20] });
+        const fieldDict = makeSigField({ T: "sig_full", sigRef });
+
+        const xref = new XRefMock([
+          { ref: sigRef, data: sigDict },
+          { ref: fieldRef, data: fieldDict },
+        ]);
+        acroForm.set("Fields", [fieldRef]);
+
+        const documentStream = new StringStream(
+          "A".repeat(50) + " ".repeat(150)
+        );
+        const pdfDocument = getDocument(acroForm, xref, documentStream);
+        const signatures = await pdfDocument.signatures;
+        expect(signatures.length).toEqual(1);
+        expect(signatures[0].coversWholeDocument).toBeTrue();
+      });
+
+      it("clears whole-document coverage when non-whitespace trails the signed range", async function () {
+        const acroForm = new Dict();
+        acroForm.set("SigFlags", 3);
+
+        const sigRef = Ref.get(42, 0);
+        const fieldRef = Ref.get(43, 0);
+        const sigDict = makeSigDict({ byteRange: [0, 20, 30, 20] });
+        const fieldDict = makeSigField({ T: "sig_partial", sigRef });
+
+        const xref = new XRefMock([
+          { ref: sigRef, data: sigDict },
+          { ref: fieldRef, data: fieldDict },
+        ]);
+        acroForm.set("Fields", [fieldRef]);
+
+        const documentStream = new StringStream(
+          "A".repeat(50) + "MODIFIED" + " ".repeat(142)
+        );
+        const pdfDocument = getDocument(acroForm, xref, documentStream);
+        const signatures = await pdfDocument.signatures;
+        expect(signatures.length).toEqual(1);
+        expect(signatures[0].coversWholeDocument).toBeFalse();
+      });
+
       it("walks Kids recursively to find nested signature fields", async function () {
         const acroForm = new Dict();
         acroForm.set("SigFlags", 3);

web/digital_signature_properties_manager.js

@@ -270,6 +270,7 @@ class SignaturePropertiesManager {
         message: null,
         certificate: null,
         documentModifiedAfterSigning: !sig.coversWholeDocument,
+        modificationsAfterSignature: sig.modificationsAfterSignature,
       });
     }
     this.#render();
@@ -647,6 +648,7 @@ class SignaturePropertiesManager {
         message: ex?.message ?? null,
         certificate: null,
         documentModifiedAfterSigning: !signature.coversWholeDocument,
+        modificationsAfterSignature: signature.modificationsAfterSignature,
       };
     }
     this.#pendingVerify.delete(signature.id);

web/firefoxcom.js

@@ -593,6 +593,7 @@ class SignatureVerifier {
         message: signature.subFilter,
         certificate: null,
         documentModifiedAfterSigning: !signature.coversWholeDocument,
+        modificationsAfterSignature: signature.modificationsAfterSignature,
       };
     }
 
@@ -610,6 +611,7 @@ class SignatureVerifier {
         message: ex?.message ?? null,
         certificate: null,
         documentModifiedAfterSigning: !signature.coversWholeDocument,
+        modificationsAfterSignature: signature.modificationsAfterSignature,
       };
     }
     if (!response || response.error) {
@@ -619,6 +621,7 @@ class SignatureVerifier {
         message: null,
         certificate: null,
         documentModifiedAfterSigning: !signature.coversWholeDocument,
+        modificationsAfterSignature: signature.modificationsAfterSignature,
       };
     }
 
@@ -632,6 +635,7 @@ class SignatureVerifier {
         message: null,
         certificate: null,
         documentModifiedAfterSigning: !signature.coversWholeDocument,
+        modificationsAfterSignature: signature.modificationsAfterSignature,
       };
     }
     const { status, errorCode } = mapVerificationStatus(
@@ -644,6 +648,7 @@ class SignatureVerifier {
       message: entry.message ?? null,
       certificate: entry.certificate ?? null,
       documentModifiedAfterSigning: !signature.coversWholeDocument,
+      modificationsAfterSignature: signature.modificationsAfterSignature,
     };
   }
 

web/genericcom.js

@@ -184,6 +184,7 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
           message: signature.subFilter,
           certificate: null,
           documentModifiedAfterSigning: !signature.coversWholeDocument,
+          modificationsAfterSignature: signature.modificationsAfterSignature,
         };
       }
 
@@ -193,6 +194,7 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
         message: null,
         certificate: null,
         documentModifiedAfterSigning: !signature.coversWholeDocument,
+        modificationsAfterSignature: signature.modificationsAfterSignature,
       };
     }