rusticata / der-parser

BER/DER parser written in pure Rust. Fast, zero-copy, safe.
Apache License 2.0
84 stars 29 forks source link

Fix conversion of Integer to fixedints #58

Closed Ichbinjoe closed 1 year ago

Ichbinjoe commented 2 years ago

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:

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

chifflier commented 2 years ago

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:

Could you check that this code implements the same checks as in your code?