Repository: J-F-Liu/lopdf (language: rust)

Issue:
Upstream bug to pdf-extract -> PdfError(Decryption(UnsupportedEncryption)) for format PDF-1.7 but not for PDF-1.6
https://github.com/jrmuizel/pdf-extract/issues/98

Above is an issue I raised on pdf-extract's issues section. It only occurs when the format is "format PDF-1.7" but not "format PDF-1.6". In that issue is an attached PDF that shows the behaviour mentioned. Is this an issue with lopdf? I am unsure if I am doing this right sorry!


Discussion / hints:
Just to give you an update. I have been looking into this issue since I am fixing some of the encryption code of lopdf right now anyway. [AQA-8461-SP-2016.PDF](https://github.com/user-attachments/files/18260026/AQA-8461-SP-2016.PDF) is actually encrypted with an empty password and seems to mostly use the AES-128 CBC encryption with PKCS#7 padding. However, support for that has only been added since lopdf 0.35 (and pdf-extract is currently using lopdf 0.34), so this is why the file doesn't work.

However, even when using commit [eaedf57](https://github.com/J-F-Liu/lopdf/commit/eaedf571b9075e9b5172b0f28289047c61e25c71), decrypting the file currently still causes a panic for one of the encrypted objects. My current hypothesis is that one of the encrypted objects is using a crypt filter specifying that a different algorithm should be used.

Once #378 has been merged, I will open a pull request to at least fix that panic, which will still fail to decrypt the document with an `UnpadError` (for which I will remove the unwrap too) because it doesn't seem to be using AES-128 CBC with PKCS#7 padding. I will also see if I can figure out if supporting crypt filters resolves this issue, but maybe also AES-256 CBC with PKCS#7 support is required.
Thank you very much for the update :)
OK, I looked a bit more into this and all stream objects that are encrypted do use AES-128 CBC (no other crypt filters). The problem seems to be that the PDF contains multiple trailers where the trailers themselves are stream objects that are just Zlib compressed but not encrypted at all (so they shouldn't be decrypted either). Ignoring stream objects containing the "Root" key in their dictionary when decrypting seems to fix the issue (**edit**: the correct fix for this is to ignore cross-reference streams, since those shouldn't be encrypted according to the specification).

The code also needs a few sanity checks:
 * Make sure that the encrypted data can be split correctly at 16 bytes into the IV and the ciphertext (without causing a panic).
 * Make sure the ciphertext length is a multiple of 16 for AES-128 CBC.
 * If there is no data, just return an empty vector of bytes.

However, this only ensures that the `decrypt()` doesn't fail. The "Pages" key in the catalog seems to refer to an object ID that doesn't exist for some reason.
The remaining problem is that `Document::decrypt()` gets called after `Reader::read()`, but `Reader::read()` is responsible for parsing the objects from the object streams. However, the document provided contains object streams that are encrypted, including the stream that contains the object to which `Pages` refers.

I moved the decryption logic from `Document::decrypt()` to `Reader::read()` in a local version and that works to the point that I can see all the pages now.

This can be fixed in several ways, but I am not sure which one is more appropriate:

1. Simply extend all the `Document::load()` functions to accept an optional password and do the decryption in `Reader::read()` and remove `Document::decrypt()`.
2. Keep `Document::decrypt()`, but copy the object stream parsing logic to `Document::decrypt()` and avoid parsing object streams if the document is encrypted to defer that to the `Document::decrypt()` function.
3. Since `Document::load_filtered()` allows users to pass their own object filter function. It can possibly be used to implement the decryption logic that way, but I suspect it requires parsing the document twice (the first time to just get the `Encrypt` dictionary, and a second time to actually parse the document).

I am currently leaning towards number two since that doesn't have any breaking API changes, and would also avoid cases where `load()` fails due some error related to encryption.
Thanks, yeah I like the sound of number two because there are no breaking changes.

Fix patch:
diff --git a/src/encryption.rs b/src/encryption.rs
index 0994af8..12052a4 100644
--- a/src/encryption.rs
+++ b/src/encryption.rs
@@ -212,6 +212,16 @@ pub fn decrypt_object<Key>(key: Key, obj_id: ObjectId, obj: &mut Object, aes: bo
 where
     Key: AsRef<[u8]>,
 {
+    // The cross-reference stream shall not be encrypted and strings appearing in the
+    // cross-reference stream dictionary shall not be encrypted.
+    let is_xref_stream = obj.as_stream()
+        .map(|stream| stream.dict.has_type(b"XRef"))
+        .unwrap_or(false);
+
+    if is_xref_stream {
+        return Ok(());
+    }
+
     let key = key.as_ref();
     let len = if aes { key.len() + 9 } else { key.len() + 5 };
     let mut builder = Vec::<u8>::with_capacity(len);


Test patch: