From a0abf8a06f8fc1bdad9a5cd32b8c475d533dd0c6 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Sun, 13 Sep 2026 22:56:20 -0300 Subject: [PATCH 1/2] fix: prevent /P permissions overflow from corrupting decryption key PdfNumber#intValue() narrowed the underlying double with a plain (int) cast. Some PDF producers write the /Encrypt dictionary's /P (permissions) entry using its unsigned 32-bit decimal representation instead of the equivalent signed value (e.g. 4294965956 instead of -1340) - both encode the identical 4-byte value, and this is spec-legal. Java's narrowing double-to-int conversion saturates rather than wraps for an out-of-range value, so such a /P value was misread as Integer.MAX_VALUE instead of -1340. That corrupted the encryption key the standard security handler derives, so its own recomputed /U check failed for every password - including a correct, empty one - and PdfReader reported BadPasswordException for a document that was never actually password-protected. Route the cast through long first, which reproduces the expected low-order-32-bit truncation instead of clamping. Applied identically to both the legacy (com.lowagie) and modern (org.openpdf) copies of PdfNumber, since this branch ships both. Add PdfNumberTest covering the truncation directly, and DecryptUnsignedPermissionsTest, a PdfReader-level regression test using a crafted PDF whose /P is written this way with an empty user password, to both modules. Co-Authored-By: Claude Sonnet 5 --- .../java/com/lowagie/text/pdf/PdfNumber.java | 11 +++- .../com/lowagie/text/pdf/PdfNumberTest.java | 31 ++++++++++ .../DecryptUnsignedPermissionsTest.java | 57 ++++++++++++++++++ .../empty-user-password-unsigned-p-value.pdf | Bin 0 -> 615 bytes .../java/org/openpdf/text/pdf/PdfNumber.java | 11 +++- .../org/openpdf/text/pdf/PdfNumberTest.java | 31 ++++++++++ .../DecryptUnsignedPermissionsTest.java | 57 ++++++++++++++++++ .../empty-user-password-unsigned-p-value.pdf | Bin 0 -> 615 bytes 8 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/PdfNumberTest.java create mode 100644 openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java create mode 100644 openpdf-core-legacy/src/test/resources/permissions/empty-user-password-unsigned-p-value.pdf create mode 100644 openpdf-core-modern/src/test/java/org/openpdf/text/pdf/PdfNumberTest.java create mode 100644 openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java create mode 100644 openpdf-core-modern/src/test/resources/permissions/empty-user-password-unsigned-p-value.pdf diff --git a/openpdf-core-legacy/src/main/java/com/lowagie/text/pdf/PdfNumber.java b/openpdf-core-legacy/src/main/java/com/lowagie/text/pdf/PdfNumber.java index 8a785484e..0d52a0d2f 100644 --- a/openpdf-core-legacy/src/main/java/com/lowagie/text/pdf/PdfNumber.java +++ b/openpdf-core-legacy/src/main/java/com/lowagie/text/pdf/PdfNumber.java @@ -137,11 +137,20 @@ public PdfNumber(float value) { /** * Returns the primitive int value of this object. + *

+ * Some PDF producers write 32-bit values that are meant to be interpreted as a two's-complement + * signed integer (for example the {@code /P} entry of an encryption dictionary) using their + * equivalent unsigned decimal representation instead of the negative signed value, e.g. + * {@code 4294965956} instead of {@code -1340}. A plain {@code (int) value} narrowing cast on the + * underlying double would clamp such an out-of-range value to + * {@link Integer#MAX_VALUE} rather than wrapping it, silently corrupting values like that. Going + * through long first reproduces the C-style truncation to the low-order 32 bits that + * PDF producers/consumers expect. * * @return The value as int */ public int intValue() { - return (int) value; + return (int) (long) value; } /** diff --git a/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/PdfNumberTest.java b/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/PdfNumberTest.java new file mode 100644 index 000000000..ae145c62f --- /dev/null +++ b/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/PdfNumberTest.java @@ -0,0 +1,31 @@ +package com.lowagie.text.pdf; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.Test; + +class PdfNumberTest { + + /** + * Some PDF producers write the encryption dictionary's {@code /P} (permissions) entry using its + * equivalent unsigned 32-bit decimal representation instead of the negative signed value, e.g. + * {@code 4294965956} instead of {@code -1340}. {@link PdfNumber#intValue()} must truncate such a + * value to its low-order 32 bits rather than clamping it to {@link Integer#MAX_VALUE}, otherwise + * the permission bits used to derive the decryption key no longer match the value the producer + * used, and a document with an empty user password is wrongly reported as requiring a password. + */ + @Test + void intValueTruncatesOutOfRangeUnsignedRepresentationInsteadOfClamping() { + assertEquals(-1340, new PdfNumber("4294965956").intValue()); + assertEquals(-1, new PdfNumber("4294967295").intValue()); + assertEquals(Integer.MIN_VALUE, new PdfNumber(String.valueOf(1L << 31)).intValue()); + } + + @Test + void intValueRoundTripsValuesWithinIntRange() { + assertEquals(-1340, new PdfNumber("-1340").intValue()); + assertEquals(0, new PdfNumber("0").intValue()); + assertEquals(Integer.MAX_VALUE, new PdfNumber(String.valueOf(Integer.MAX_VALUE)).intValue()); + assertEquals(Integer.MIN_VALUE, new PdfNumber(String.valueOf(Integer.MIN_VALUE)).intValue()); + } +} diff --git a/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java b/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java new file mode 100644 index 000000000..5cfba1366 --- /dev/null +++ b/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java @@ -0,0 +1,57 @@ +package com.lowagie.text.pdf.encryption; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.lowagie.text.pdf.PdfReader; +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.Field; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** + * Regression test for a document that has no user password but is nevertheless reported by + * {@link PdfReader} as requiring one, because its {@code /Encrypt} dictionary writes the {@code /P} + * (permissions) entry using the unsigned 32-bit decimal representation of a negative value (e.g. + * {@code 4294965956} instead of {@code -1340}) - a convention some PDF producers use. + *

+ * {@link com.lowagie.text.pdf.PdfNumber#intValue()} used a plain {@code (int)} narrowing cast on the + * underlying double, which clamps such an out-of-range value to + * {@link Integer#MAX_VALUE} instead of truncating it to its low-order 32 bits. The corrupted + * permission value then fed into the standard security handler's key derivation, producing a user key + * that did not match {@code /U}, so {@link PdfReader} threw a {@link com.lowagie.text.exceptions.BadPasswordException} + * even though the correct (empty) password was supplied. + */ +class DecryptUnsignedPermissionsTest { + + static Field ownerPasswordUsedField; + + static boolean isOwnerPasswordUsed(PdfReader pdfReader) { + try { + return ownerPasswordUsedField.getBoolean(pdfReader); + } catch (IllegalArgumentException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + @BeforeAll + static void setUpBeforeClass() throws Exception { + ownerPasswordUsedField = PdfReader.class.getDeclaredField("ownerPasswordUsed"); + ownerPasswordUsedField.setAccessible(true); + } + + @Test + void opensWithEmptyPasswordDespiteUnsignedPermissionsValue() throws IOException { + try (InputStream resource = getClass() + .getResourceAsStream("/permissions/empty-user-password-unsigned-p-value.pdf")) { + PdfReader pdfReader = new PdfReader(resource); + assertTrue(pdfReader.isEncrypted(), "PdfReader fails to report test file to be encrypted."); + assertFalse(isOwnerPasswordUsed(pdfReader), "PdfReader fails to report limited permissions."); + assertEquals(1, pdfReader.getNumberOfPages(), + "PdfReader fails to report the correct number of pages"); + pdfReader.close(); + } + } +} diff --git a/openpdf-core-legacy/src/test/resources/permissions/empty-user-password-unsigned-p-value.pdf b/openpdf-core-legacy/src/test/resources/permissions/empty-user-password-unsigned-p-value.pdf new file mode 100644 index 0000000000000000000000000000000000000000..d528d8b2cde6819094b8d57ad8d805bd24145baa GIT binary patch literal 615 zcmah`%Wm6147}?rc=0ja7xx7SIy8xaqAg-LX)i_&``|h%3@MORAnDh4WuQusOL^mP zhTw2EyPNHm_=JrB4rBb)w(Q~cnb>uh!)ZLST{zMiWF79ij)q(*&Cm@@lD z!*PBBAsxGeHl-x9Lsxk5!4?-%Fg`^}w%EiCI+L7LMJmCgM-SJ7#0g)uR!j_m-Y zM{nv0hV4PIAJ7U(i3ekiw?1)H6iw2}p>aGBuZOx=kTfA#?E(seoY&rzQUVv;b?hg| z)yvC7rKuhpz7`m*m4*Uun1{<0hBr*tPc8Kh6TF7@5i|8$|FC%q{fVZU>3#o)K(qZg z)?Aj5*zJ&}*XJ3Q=kD2;n?oy&@@O?tl$5Ox7b=1NCDK)g_Jcnt%_*BL< Ko6YUrw)qQ-l%xUx literal 0 HcmV?d00001 diff --git a/openpdf-core-modern/src/main/java/org/openpdf/text/pdf/PdfNumber.java b/openpdf-core-modern/src/main/java/org/openpdf/text/pdf/PdfNumber.java index b87784f2a..03b446af6 100644 --- a/openpdf-core-modern/src/main/java/org/openpdf/text/pdf/PdfNumber.java +++ b/openpdf-core-modern/src/main/java/org/openpdf/text/pdf/PdfNumber.java @@ -135,11 +135,20 @@ public PdfNumber(float value) { /** * Returns the primitive int value of this object. + *

+ * Some PDF producers write 32-bit values that are meant to be interpreted as a two's-complement + * signed integer (for example the {@code /P} entry of an encryption dictionary) using their + * equivalent unsigned decimal representation instead of the negative signed value, e.g. + * {@code 4294965956} instead of {@code -1340}. A plain {@code (int) value} narrowing cast on the + * underlying double would clamp such an out-of-range value to + * {@link Integer#MAX_VALUE} rather than wrapping it, silently corrupting values like that. Going + * through long first reproduces the C-style truncation to the low-order 32 bits that + * PDF producers/consumers expect. * * @return The value as int */ public int intValue() { - return (int) value; + return (int) (long) value; } /** diff --git a/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/PdfNumberTest.java b/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/PdfNumberTest.java new file mode 100644 index 000000000..edc9e9f38 --- /dev/null +++ b/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/PdfNumberTest.java @@ -0,0 +1,31 @@ +package org.openpdf.text.pdf; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.Test; + +class PdfNumberTest { + + /** + * Some PDF producers write the encryption dictionary's {@code /P} (permissions) entry using its + * equivalent unsigned 32-bit decimal representation instead of the negative signed value, e.g. + * {@code 4294965956} instead of {@code -1340}. {@link PdfNumber#intValue()} must truncate such a + * value to its low-order 32 bits rather than clamping it to {@link Integer#MAX_VALUE}, otherwise + * the permission bits used to derive the decryption key no longer match the value the producer + * used, and a document with an empty user password is wrongly reported as requiring a password. + */ + @Test + void intValueTruncatesOutOfRangeUnsignedRepresentationInsteadOfClamping() { + assertEquals(-1340, new PdfNumber("4294965956").intValue()); + assertEquals(-1, new PdfNumber("4294967295").intValue()); + assertEquals(Integer.MIN_VALUE, new PdfNumber(String.valueOf(1L << 31)).intValue()); + } + + @Test + void intValueRoundTripsValuesWithinIntRange() { + assertEquals(-1340, new PdfNumber("-1340").intValue()); + assertEquals(0, new PdfNumber("0").intValue()); + assertEquals(Integer.MAX_VALUE, new PdfNumber(String.valueOf(Integer.MAX_VALUE)).intValue()); + assertEquals(Integer.MIN_VALUE, new PdfNumber(String.valueOf(Integer.MIN_VALUE)).intValue()); + } +} diff --git a/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java b/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java new file mode 100644 index 000000000..bc4d65c8c --- /dev/null +++ b/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java @@ -0,0 +1,57 @@ +package org.openpdf.text.pdf.encryption; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.openpdf.text.pdf.PdfReader; +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.Field; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** + * Regression test for a document that has no user password but is nevertheless reported by + * {@link PdfReader} as requiring one, because its {@code /Encrypt} dictionary writes the {@code /P} + * (permissions) entry using the unsigned 32-bit decimal representation of a negative value (e.g. + * {@code 4294965956} instead of {@code -1340}) - a convention some PDF producers use. + *

+ * {@link org.openpdf.text.pdf.PdfNumber#intValue()} used a plain {@code (int)} narrowing cast on the + * underlying double, which clamps such an out-of-range value to + * {@link Integer#MAX_VALUE} instead of truncating it to its low-order 32 bits. The corrupted + * permission value then fed into the standard security handler's key derivation, producing a user key + * that did not match {@code /U}, so {@link PdfReader} threw a {@link org.openpdf.text.exceptions.BadPasswordException} + * even though the correct (empty) password was supplied. + */ +class DecryptUnsignedPermissionsTest { + + static Field ownerPasswordUsedField; + + static boolean isOwnerPasswordUsed(PdfReader pdfReader) { + try { + return ownerPasswordUsedField.getBoolean(pdfReader); + } catch (IllegalArgumentException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + @BeforeAll + static void setUpBeforeClass() throws Exception { + ownerPasswordUsedField = PdfReader.class.getDeclaredField("ownerPasswordUsed"); + ownerPasswordUsedField.setAccessible(true); + } + + @Test + void opensWithEmptyPasswordDespiteUnsignedPermissionsValue() throws IOException { + try (InputStream resource = getClass() + .getResourceAsStream("/permissions/empty-user-password-unsigned-p-value.pdf")) { + PdfReader pdfReader = new PdfReader(resource); + assertTrue(pdfReader.isEncrypted(), "PdfReader fails to report test file to be encrypted."); + assertFalse(isOwnerPasswordUsed(pdfReader), "PdfReader fails to report limited permissions."); + assertEquals(1, pdfReader.getNumberOfPages(), + "PdfReader fails to report the correct number of pages"); + pdfReader.close(); + } + } +} diff --git a/openpdf-core-modern/src/test/resources/permissions/empty-user-password-unsigned-p-value.pdf b/openpdf-core-modern/src/test/resources/permissions/empty-user-password-unsigned-p-value.pdf new file mode 100644 index 0000000000000000000000000000000000000000..d528d8b2cde6819094b8d57ad8d805bd24145baa GIT binary patch literal 615 zcmah`%Wm6147}?rc=0ja7xx7SIy8xaqAg-LX)i_&``|h%3@MORAnDh4WuQusOL^mP zhTw2EyPNHm_=JrB4rBb)w(Q~cnb>uh!)ZLST{zMiWF79ij)q(*&Cm@@lD z!*PBBAsxGeHl-x9Lsxk5!4?-%Fg`^}w%EiCI+L7LMJmCgM-SJ7#0g)uR!j_m-Y zM{nv0hV4PIAJ7U(i3ekiw?1)H6iw2}p>aGBuZOx=kTfA#?E(seoY&rzQUVv;b?hg| z)yvC7rKuhpz7`m*m4*Uun1{<0hBr*tPc8Kh6TF7@5i|8$|FC%q{fVZU>3#o)K(qZg z)?Aj5*zJ&}*XJ3Q=kD2;n?oy&@@O?tl$5Ox7b=1NCDK)g_Jcnt%_*BL< Ko6YUrw)qQ-l%xUx literal 0 HcmV?d00001 From 94f3a8fd4771a6d7fbff76637dae0f19b3239442 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Mon, 14 Sep 2026 19:10:58 -0300 Subject: [PATCH 2/2] test: use PdfReader.isOwnerPasswordUsed() instead of reflection DecryptUnsignedPermissionsTest reached PdfReader's private ownerPasswordUsed field via reflection (setAccessible), which Codacy flagged as a high-severity visibility-alteration issue, plus a medium issue for wrapping the reflection exceptions in a raw RuntimeException. PdfReader already exposes a public isOwnerPasswordUsed() getter that returns the same field, so the reflection is unnecessary. Calling it directly returns the identical value and removes both findings. Co-Authored-By: Claude Sonnet 5 --- .../DecryptUnsignedPermissionsTest.java | 20 +------------------ .../DecryptUnsignedPermissionsTest.java | 20 +------------------ 2 files changed, 2 insertions(+), 38 deletions(-) diff --git a/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java b/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java index 5cfba1366..909b970e1 100644 --- a/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java +++ b/openpdf-core-legacy/src/test/java/com/lowagie/text/pdf/encryption/DecryptUnsignedPermissionsTest.java @@ -7,8 +7,6 @@ import com.lowagie.text.pdf.PdfReader; import java.io.IOException; import java.io.InputStream; -import java.lang.reflect.Field; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; /** @@ -26,29 +24,13 @@ */ class DecryptUnsignedPermissionsTest { - static Field ownerPasswordUsedField; - - static boolean isOwnerPasswordUsed(PdfReader pdfReader) { - try { - return ownerPasswordUsedField.getBoolean(pdfReader); - } catch (IllegalArgumentException | IllegalAccessException e) { - throw new RuntimeException(e); - } - } - - @BeforeAll - static void setUpBeforeClass() throws Exception { - ownerPasswordUsedField = PdfReader.class.getDeclaredField("ownerPasswordUsed"); - ownerPasswordUsedField.setAccessible(true); - } - @Test void opensWithEmptyPasswordDespiteUnsignedPermissionsValue() throws IOException { try (InputStream resource = getClass() .getResourceAsStream("/permissions/empty-user-password-unsigned-p-value.pdf")) { PdfReader pdfReader = new PdfReader(resource); assertTrue(pdfReader.isEncrypted(), "PdfReader fails to report test file to be encrypted."); - assertFalse(isOwnerPasswordUsed(pdfReader), "PdfReader fails to report limited permissions."); + assertFalse(pdfReader.isOwnerPasswordUsed(), "PdfReader fails to report limited permissions."); assertEquals(1, pdfReader.getNumberOfPages(), "PdfReader fails to report the correct number of pages"); pdfReader.close(); diff --git a/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java b/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java index bc4d65c8c..a7fa3186b 100644 --- a/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java +++ b/openpdf-core-modern/src/test/java/org/openpdf/text/pdf/encryption/DecryptUnsignedPermissionsTest.java @@ -7,8 +7,6 @@ import org.openpdf.text.pdf.PdfReader; import java.io.IOException; import java.io.InputStream; -import java.lang.reflect.Field; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; /** @@ -26,29 +24,13 @@ */ class DecryptUnsignedPermissionsTest { - static Field ownerPasswordUsedField; - - static boolean isOwnerPasswordUsed(PdfReader pdfReader) { - try { - return ownerPasswordUsedField.getBoolean(pdfReader); - } catch (IllegalArgumentException | IllegalAccessException e) { - throw new RuntimeException(e); - } - } - - @BeforeAll - static void setUpBeforeClass() throws Exception { - ownerPasswordUsedField = PdfReader.class.getDeclaredField("ownerPasswordUsed"); - ownerPasswordUsedField.setAccessible(true); - } - @Test void opensWithEmptyPasswordDespiteUnsignedPermissionsValue() throws IOException { try (InputStream resource = getClass() .getResourceAsStream("/permissions/empty-user-password-unsigned-p-value.pdf")) { PdfReader pdfReader = new PdfReader(resource); assertTrue(pdfReader.isEncrypted(), "PdfReader fails to report test file to be encrypted."); - assertFalse(isOwnerPasswordUsed(pdfReader), "PdfReader fails to report limited permissions."); + assertFalse(pdfReader.isOwnerPasswordUsed(), "PdfReader fails to report limited permissions."); assertEquals(1, pdfReader.getNumberOfPages(), "PdfReader fails to report the correct number of pages"); pdfReader.close();