Fix DSS-3946

[enzoBrum]

relevant issue: DSS-3946

This PR modifies the SignatureDictionary check to make it ensures the entries /Rect and /AP weren't modified after signature creation and creates two unit tests demonstrating that the attacks mentioned on the issue don't work anymore.

[enzoBrum]

Originally i had written an implementation recursively checking the signature field, but decided to backtrack because behavior between PdfBox and OpenPDF diferred. There were some documents used on unit tests for the dss-pades module in which OpenPDF added the keys (/DA, /DR, /Fields) to the final revision signature field but not to the field in the revision it was filled. These keys were seemingly inherited from somewhere else given they didn't show up when physically looking at the raw signature fields.

One example of such a file was the one used by DSS1899Test. The same behaviour did not occur with PdfBox. This made me reconsider the recursive approach and decide to just explicitly check the relevant keys, just to be on the safe side. Do you think this is reasonable? Or is it better to just recurse the full signature field while ignoring the offending keys (and non-relevant ones like /Kids, /P and /Parent)?

Furthermore, I also had to change the public API of PdfSigDictWrapper such that the Signature Fields could be verified in there. Is that acceptable? I can refactor to execute such verification within the SignatureField object itself if not.

[bsanchezb]

Hi Enzo,

Thank you for the Pull Request! I did some modifications that you may find in #195. Please verify whether it fixes the issue.

Please note that the PR includes other commits from the develop branch we made in the time being. Please ignore those commits. The relevant commit for this PR is 168effc .

Best regards,
Aleksandr

[bsanchezb]

Hm, found something in the ISO 32000-2:

12.5.2 Annotation dictionaries

{After the "Table 166 — Entries common to all annotation dictionaries"}

A PDF reader shall render the appearance dictionary without regard to any other keys and values in
the annotation dictionary and shall ignore the values of the C, IC, Border, BS, BE, BM, CA, ca, H, DA, Q,
DS, LE, LL, LLE, and Sy keys.

Perhaps evaluating against all entries is indeed too much. Probably will try to use a "blacklist" of properties to be evaluated only, but not all of them. You can see the last commit with the changes.