Fix ISO10126 and PKCS7 unpad#32
Conversation
| // 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 |
There was a problem hiding this comment.
This is fine but assumes 32-bit integers. At least Python and JS are failing some of the tests because the shift here does not produce the expected bit pattern. haxe.Int32 would probably fix this?
| // 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"))); |
There was a problem hiding this comment.
Here it is straight up lying about the test case: the bytes should be (for example) AAAAAA00040404 (so the 4 padding bytes are actually 00 04 04 04 as it wands).
| // 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"))); |
There was a problem hiding this comment.
LLMism of generating an invalid test case, realising it's fine, then trying again. This is also the same test case in the end as the previous one, remove.
|
Oh hey, glad to see you're still around and thanks for the review! |
|
With the recent changes, everything seems correct to me. |
3 participants
(Full disclosure: I know nothing about this and simply asked Claude.)
We received a report about potential security problems in these functions. The ISO10126 one looks like an obvious fix, so that should be good. The other one is more involved and this should definitely not be merged before it has been reviewed by a human being.