Skip to content

Avoid password prompt on attachment-only encryption with no attachments #20140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid password prompt on attachment-only encryption with no attachments #20140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 38 additions & 16 deletions src/core/xref.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
} from "./core_utils.js";
import { BaseStream } from "./base_stream.js";
import { CipherTransformFactory } from "./crypto.js";
import { NameTree } from "./name_number_tree.js";

class XRef {
#firstXRefStmPos = null;
Expand Down Expand Up @@ -118,22 +119,6 @@ class XRef {
}
warn(`XRef.parse - Invalid "Encrypt" reference: "${ex}".`);
}
if (encrypt instanceof Dict) {
const ids = trailerDict.get("ID");
const fileId = ids?.length ? ids[0] : "";
// The 'Encrypt' dictionary itself should not be encrypted, and by
// setting `suppressEncryption` we can prevent an infinite loop inside
// of `XRef_fetchUncompressed` if the dictionary contains indirect
// objects (fixes issue7665.pdf).
encrypt.suppressEncryption = true;
this.encrypt = new CipherTransformFactory(
encrypt,
fileId,
this.pdfManager.password
);
}

// Get the root dictionary (catalog) object, and do some basic validation.
let root;
try {
root = trailerDict.get("Root");
Expand All @@ -143,6 +128,43 @@ class XRef {
}
warn(`XRef.parse - Invalid "Root" reference: "${ex}".`);
}

if (encrypt instanceof Dict) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell all this stuff is useless and only the AuthEvent entry should be checked:
https://christianhaider.de/dokuwiki/lib/exe/fetch.php?media=pdf:pdf32000_2008.pdf#page=76&zoom=auto,-60,624

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, It looks like AuthEvent checks for when to check for encryption but doesn't really tell us what parts are encrypted, which is what we need to check if attachments are the only thing that's encrypted

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the AuthEvent value we should only ask for the password when the user wants to access to the attachments (EFOpen) or when the doc is open (DocOpen).
So if the value is EFOpen and there are no attachments, then the user won't be prompted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time digging into this today, and based on what the spec says, I agree we can simplify things here. For PDFs with encrypted attachments, there appear to be three relevant cases:

  1. The PDF has /AuthEvent /EFOpen, and includes attachments: (Issue [Bug]: File Attachments do not open when they are encrypted #20139)
  2. It has /AuthEvent /EFOpen but no attachments, so we still need to check if any are actually present.
  3. There's No /AuthEvent, but it uses /StmF /Identity, /StrF /Identity, and /EFF /StdCF, this is a very specific case and not something we see in practice, I tried to generate a PDF that hits this edge-case, but I ended up with a PDF where the entries were /StmF /Identity, /StrF /Identity, and /EFF /StdCF and there was no /AuthEvent, but the content of the attachments was not actually encrypted.

So now, I’ve removed the extra checks for /StmF, /StrF, and /EFF. We're only checking for /AuthEvent and whether the file actually has attachments.

// Check if only the file attachments are encrypted.
if (
encrypt.get("CF")?.get("StdCF")?.get("AuthEvent")?.name === "EFOpen"
) {
let hasEncryptedAttachments = false;
if (root instanceof Dict) {
const names = root.get("Names");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you're doing that.
If there's no attachment then the user cannot click on them and the encryption stuff will never be used.
And if there are attachments then we've to only decrypt them when the user request to read them.
There is a bug (we talked about with @timvandermeij) about having to only load (and decrypt) the attachments when the user requests them, because right now we get them all the time.

if (names instanceof Dict && names.has("EmbeddedFiles")) {
const nameTree = new NameTree(names.getRaw("EmbeddedFiles"), this);
const attachments = nameTree.getAll();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about how much performance matters in this case, but we could have a nameTree.isEmpty() method to avoid having to extract all the entries just to check if any exists.

if (attachments.size > 0) {
hasEncryptedAttachments = true;
}
}
}
if (!hasEncryptedAttachments) {
// If there are no encrypted attachments, encrypt dictionary is
// not needed.
encrypt = null;
}
} else {
const ids = trailerDict.get("ID");
const fileId = ids?.length ? ids[0] : "";
// The 'Encrypt' dictionary itself should not be encrypted, and by
// setting `suppressEncryption` we can prevent an infinite loop inside
// of `XRef_fetchUncompressed` if the dictionary contains indirect
// objects (fixes issue7665.pdf).
encrypt.suppressEncryption = true;
this.encrypt = new CipherTransformFactory(
encrypt,
fileId,
this.pdfManager.password
);
}
}
if (root instanceof Dict) {
try {
const pages = root.get("Pages");
Expand Down
1 change: 1 addition & 0 deletions test/pdfs/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@
!bug1020226.pdf
!issue9534_reduced.pdf
!attachment.pdf
!issue20049.pdf
!basicapi.pdf
!issue15590.pdf
!issue15594_reduced.pdf
Expand Down
Binary file added test/pdfs/issue20049.pdf
View file
Open in desktop
Binary file not shown.
7 changes: 7 additions & 0 deletions test/test_manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4930,6 +4930,13 @@
"rounds": 1,
"type": "eq"
},
{
"id": "issue20049",
"file": "pdfs/issue20049.pdf",
"md5": "1cdfde56be6b070e0c18aafc487d92ff",
"rounds": 1,
"type": "eq"
},
{
"id": "issue8117",
"file": "pdfs/issue8117.pdf",
Expand Down
15 changes: 15 additions & 0 deletions test/unit/api_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,21 @@ describe("api", function () {

await loadingTask.destroy();
});

it("should not prompt for password if only attachments are encrypted and there are none", async function () {
const loadingTask = getDocument(buildGetDocumentParams("issue20049.pdf"));
expect(loadingTask instanceof PDFDocumentLoadingTask).toEqual(true);

loadingTask.onPassword = function (callback, reason) {
if (reason === PasswordResponses.NEED_PASSWORD) {
expect(false).toEqual(true);
throw new Error("Should not prompt for password.");
}
};

const pdfDocument = await loadingTask.promise;
expect(pdfDocument.numPages).toBeGreaterThan(0);
});
});

describe("PDFWorker", function () {
Expand Down
Loading