der-parser
der-parser copied to clipboard
Fix conversion of Integer to fixedints
The current implementations of as_i64, as_i32, as_u64, and as_u32 are all flawed (as well as the der validation of integers) when it comes to handing particularities around negative integers. Of note is specifically the following:
- DER Validation doesn't properly enforce 8.3.2.b of x.690 for negative numbers. For example, 0xff 0xff should fail, but will not as the code currently only checks for whether the first 9 bytes are 0, not whether they are also 1.
- as_i32 / as_i64 could incorrectly interpret a positive integer as a negative one. Ex: 0x00 0xff 0xff 0xff 0xff will be interpreted as -1 instead of IntegerTooLarge (first bit is not set, so output will be filled with 0x00, 0x00 gets stripped off by remove_zeros, all bytes get overwritten by 0xff, thus an i64 taking in the buffer with from_be_bytes will be -1).
- remove_zeros relies on TRO which is usually fine but suboptimial
This change will also make conversion more restrictive when 8.3.2 doesn't hold (even in BER cases) - if excess bytes are supplied, as_* may report IntegerTooLarge even if the Integer could be represented as a smaller integer (ex. 0x00 0x00 0x00 0x00 0x01 is accepted today for as_u32 but will now return IntegerTooLarge). This wouldn't be a regression in DER as such an encoding would (should?) fail validation, only for BER.
This commit also makes other changes as well - there is generally less branching, especially in the signed integer case (relying on shr for extending the twos compliment bits).
Hi,
Thanks for your contribution!
First of all sorry for the long delay in the reply. This crate was under heavy refactoring to merge/use parsing code from the new asn-rs crate. This is where the parsing code will be moved, and ultimately der-parser will only be a frontend (optional) to asn1-rs.
So, this PR cannot be merged anymore, since the integers are not parsed here anymore. However, I tried to merge and/or verify things during the process.
The relevant code in asn1-rs is in the following section:
- https://github.com/rusticata/asn1-rs/blob/master/src/asn1_types/integer.rs#L424-L440 for the DER verifications
- https://github.com/rusticata/asn1-rs/blob/master/src/asn1_types/integer.rs#L10-L40 for trimming leading bytes (only for BER)
Could you check that this code implements the same checks as in your code?