Fix ISO10126 and PKCS7 unpad

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);
 	}
 }

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);
 	}
 }

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());
+		}
+	}
 }