From b7888f258e812dbc91b4d3ac2f8b03101f0b228b Mon Sep 17 00:00:00 2001 From: Simon Krajewski Date: Wed, 11 Mar 2026 07:33:44 +0100 Subject: [PATCH 1/2] fix ISO10126 and PKCS7 unpad --- src/haxe/crypto/padding/ISO10126.hx | 2 + src/haxe/crypto/padding/PKCS7.hx | 26 +++++--- tests/src/unit/crypto/PaddingTest.hx | 90 ++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/haxe/crypto/padding/ISO10126.hx b/src/haxe/crypto/padding/ISO10126.hx index 08aedcc..5fc36d8 100644 --- a/src/haxe/crypto/padding/ISO10126.hx +++ b/src/haxe/crypto/padding/ISO10126.hx @@ -18,6 +18,8 @@ class ISO10126 { public static function unpad(encrypt:Bytes):Bytes { var padding:Int = encrypt.get(encrypt.length - 1); + if (padding < 1 || padding > encrypt.length) + throw "Invalid ISO10126 padding"; return encrypt.sub(0, encrypt.length - padding); } } diff --git a/src/haxe/crypto/padding/PKCS7.hx b/src/haxe/crypto/padding/PKCS7.hx index 1576ffe..00256bf 100644 --- a/src/haxe/crypto/padding/PKCS7.hx +++ b/src/haxe/crypto/padding/PKCS7.hx @@ -20,14 +20,26 @@ class PKCS7 { } public static function unpad(encrypt:Bytes):Bytes { - var padding:Int = encrypt.get(encrypt.length - 1); - if (padding > encrypt.length) - throw "Cannot remove " + padding + " bytes, because message is " + encrypt.length + " bytes"; - var block = encrypt.length - padding; - for (i in block...encrypt.length) { - if (encrypt.get(i) != padding) - throw "Invalid padding value. Got " + encrypt.get(i) + ", expected " + padding + " at position " + i; + var len = encrypt.length; + var padding:Int = encrypt.get(len - 1); + // Constant-time validation to prevent padding oracle timing attacks. + // We accumulate errors without early exit so the execution path does + // not reveal whether the padding value is in or out of range, and so + // the loop does not leak how many padding bytes were correct. + var bad:Int = 0; + // Arithmetic-right-shift trick: for a 32-bit signed int x, + // (x >> 31) is -1 (all bits set) when x < 0, and 0 otherwise. + bad |= (padding - 1) >> 31; // non-zero if padding < 1 + bad |= (len - padding) >> 31; // non-zero if padding > len + // Clamp block to a valid index (bad is already set for this case). + var block = len - padding; + if (block < 0) block = 0; + // Check every padding byte without early exit. + for (i in block...len) { + bad |= encrypt.get(i) ^ padding; } + if (bad != 0) + throw "Invalid PKCS7 padding"; return encrypt.sub(0, block); } } diff --git a/tests/src/unit/crypto/PaddingTest.hx b/tests/src/unit/crypto/PaddingTest.hx index 2e01060..0984c6e 100644 --- a/tests/src/unit/crypto/PaddingTest.hx +++ b/tests/src/unit/crypto/PaddingTest.hx @@ -103,4 +103,94 @@ class PaddingTest extends Test { eq(plainText[i], padTbc.toHex().toUpperCase()); } } + + // ----------------------------------------------------------------------- + // Security regression tests + // ----------------------------------------------------------------------- + + /** + * PKCS7.unpad must reject all invalid inputs with a single uniform + * exception, regardless of *why* validation fails. Using different + * code paths for "out of range" vs "wrong byte value" leaks the + * padding length to an attacker (padding oracle / timing side-channel). + */ + public function test_pkcs7_unpad_security():Void { + trace("PKCS7 security: all invalid inputs must throw ..."); + + // padding = 0 is never produced by pad() and must be rejected + exc(() -> PKCS7.unpad(Bytes.ofHex("0000000000000000"))); + + // padding byte value (0xFF = 255) exceeds the message length (1 byte) + exc(() -> PKCS7.unpad(Bytes.ofHex("FF"))); + + // padding byte value exceeds the message length (4 bytes, value 8) + exc(() -> PKCS7.unpad(Bytes.ofHex("04040408"))); + + // one corrupted byte in the middle of an otherwise valid padding region + // "0001040404040404": last byte = 4, bytes at positions 4-7 are 00,04,04,04 + exc(() -> PKCS7.unpad(Bytes.ofHex("0001040404040404"))); + + // only the first padding byte is wrong — catches a non-constant-time loop + // "0808080808080801": last byte = 1, byte at position 7 = 0x01 — that's fine, + // but the declared padding value is 1, so only position 7 is checked and it + // equals 1 → actually valid. Use a case where the first mismatch is early: + // "0102030404040404": last byte = 4, bytes at positions 4-7: 04,04,04,04 → valid + // "0102030400040404": last byte = 4, byte at position 4 = 0x00 ≠ 4 → invalid + exc(() -> PKCS7.unpad(Bytes.ofHex("0102030400040404"))); + + trace("PKCS7 security: valid inputs must still be accepted ..."); + + // full block of padding (blockSize = 8, padding = 8) + var result = PKCS7.unpad(Bytes.ofHex("0808080808080808")); + eq(0, result.length); + + // single padding byte + eq("01", PKCS7.unpad(Bytes.ofHex("0101")).toHex().toUpperCase()); + + // round-trip: pad then unpad must yield the original plaintext + for (i in 0...plainText.length) { + var original = Bytes.ofHex(plainText[i]); + var padded = PKCS7.pad(original, BLOCK_SIZE); + var restored = PKCS7.unpad(padded); + eq(plainText[i], restored.toHex().toUpperCase()); + } + } + + /** + * ISO10126.unpad must validate the padding length field before using it. + * Without this check an attacker can supply a crafted last byte that + * causes an out-of-bounds access, crashing the program. + * + * ISO 10126 pad() always writes a length-byte ≥ 1, so 0 is never a + * legitimate value and must also be rejected. + */ + public function test_iso10126_unpad_security():Void { + trace("ISO10126 security: padding = 0 must be rejected ..."); + // Last byte 0x00: never produced by pad(), must be rejected + exc(() -> ISO10126.unpad(Bytes.ofHex("0100"))); + + trace("ISO10126 security: padding > length must not crash ..."); + // 1-byte input, last byte = 0xFF (255) — would read sub(0, -254) + exc(() -> ISO10126.unpad(Bytes.ofHex("FF"))); + + // 4-byte input, last byte = 0x10 (16 > 4) — padding larger than message + exc(() -> ISO10126.unpad(Bytes.ofHex("AABBCC10"))); + + trace("ISO10126 security: valid inputs must still be accepted ..."); + + // Entire buffer is padding: 5 bytes, last byte = 5, result is empty + var result = ISO10126.unpad(Bytes.ofHex("AABBCCDD05")); + eq(0, result.length); + + // Single padding byte: last byte = 1 + eq("01", ISO10126.unpad(Bytes.ofHex("0101")).toHex().toUpperCase()); + + // Round-trip: pad then unpad must yield the original plaintext + for (i in 0...plainText.length) { + var original = Bytes.ofHex(plainText[i]); + var padded = ISO10126.pad(original, BLOCK_SIZE); + var restored = ISO10126.unpad(padded); + eq(plainText[i], restored.toHex().toUpperCase()); + } + } } From e5834ada76bc307f3fb58e7e2c2a994e88fe9263 Mon Sep 17 00:00:00 2001 From: Simon Krajewski Date: Wed, 11 Mar 2026 11:00:37 +0100 Subject: [PATCH 2/2] address review feedback --- src/haxe/crypto/padding/PKCS7.hx | 8 ++++---- tests/compile-each.hxml | 2 +- tests/src/unit/Test.hx | 2 +- tests/src/unit/crypto/PaddingTest.hx | 15 ++++----------- 4 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/haxe/crypto/padding/PKCS7.hx b/src/haxe/crypto/padding/PKCS7.hx index 00256bf..b85731e 100644 --- a/src/haxe/crypto/padding/PKCS7.hx +++ b/src/haxe/crypto/padding/PKCS7.hx @@ -27,10 +27,10 @@ class PKCS7 { // not reveal whether the padding value is in or out of range, and so // the loop does not leak how many padding bytes were correct. var bad:Int = 0; - // Arithmetic-right-shift trick: for a 32-bit signed int x, - // (x >> 31) is -1 (all bits set) when x < 0, and 0 otherwise. - bad |= (padding - 1) >> 31; // non-zero if padding < 1 - bad |= (len - padding) >> 31; // non-zero if padding > len + // Use explicit boolean-to-int to avoid platform-specific bit-width + // assumptions (e.g. Python arbitrary-precision ints, JS float64). + bad |= (padding < 1) ? 1 : 0; // non-zero if padding < 1 + bad |= (padding > len) ? 1 : 0; // non-zero if padding > len // Clamp block to a valid index (bad is already set for this case). var block = len - padding; if (block < 0) block = 0; diff --git a/tests/compile-each.hxml b/tests/compile-each.hxml index f2f0270..a14c854 100644 --- a/tests/compile-each.hxml +++ b/tests/compile-each.hxml @@ -3,6 +3,6 @@ -p src -p ../src --dce full --lib utest:git:https://github.com/haxe-utest/utest#559b24c9a36533281ba7a2eed8aab83ed6b872b4 +-lib utest -D analyzer-optimize -D analyzer-user-var-fusion \ No newline at end of file diff --git a/tests/src/unit/Test.hx b/tests/src/unit/Test.hx index 23df636..cc19362 100644 --- a/tests/src/unit/Test.hx +++ b/tests/src/unit/Test.hx @@ -37,7 +37,7 @@ class Test implements utest.ITest { } function exc(f:() -> Void, ?pos:haxe.PosInfos) { - Assert.raises(f, pos); + Assert.raises(f, null, null, null, pos); } function unspec(f:() -> Void, ?pos) { diff --git a/tests/src/unit/crypto/PaddingTest.hx b/tests/src/unit/crypto/PaddingTest.hx index 0984c6e..55dd06a 100644 --- a/tests/src/unit/crypto/PaddingTest.hx +++ b/tests/src/unit/crypto/PaddingTest.hx @@ -126,17 +126,10 @@ class PaddingTest extends Test { // padding byte value exceeds the message length (4 bytes, value 8) exc(() -> PKCS7.unpad(Bytes.ofHex("04040408"))); - // one corrupted byte in the middle of an otherwise valid padding region - // "0001040404040404": last byte = 4, bytes at positions 4-7 are 00,04,04,04 - exc(() -> PKCS7.unpad(Bytes.ofHex("0001040404040404"))); - - // only the first padding byte is wrong — catches a non-constant-time loop - // "0808080808080801": last byte = 1, byte at position 7 = 0x01 — that's fine, - // but the declared padding value is 1, so only position 7 is checked and it - // equals 1 → actually valid. Use a case where the first mismatch is early: - // "0102030404040404": last byte = 4, bytes at positions 4-7: 04,04,04,04 → valid - // "0102030400040404": last byte = 4, byte at position 4 = 0x00 ≠ 4 → invalid - exc(() -> PKCS7.unpad(Bytes.ofHex("0102030400040404"))); + // one corrupted byte at the start of an otherwise valid padding region: + // "AAAAAA00040404": last byte = 4, padding positions 3-6 = 00,04,04,04; + // position 3 is 0x00 ≠ 4, so the whole padding is invalid. + exc(() -> PKCS7.unpad(Bytes.ofHex("AAAAAA00040404"))); trace("PKCS7 security: valid inputs must still be accepted ...");