der-parser icon indicating copy to clipboard operation
der-parser copied to clipboard

Fix conversion of Integer to fixedints

Open Ichbinjoe opened this issue 3 years ago • 1 comments

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).

Ichbinjoe avatar Jan 03 '22 02:01 Ichbinjoe

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?

chifflier avatar Jan 31 '22 09:01 chifflier