`BerSequence` and `DerSequence` derive macros produce code that can panic

[plusvic]

The code generated by both BerSequence and DerSequence for the TryFrom<Any> trait uses the finish function for error convertion. The issue with finish is that it can panic when the parser's result is Err(Err::Incomplete(_)). This is problematic when parsing ASN.1 structures that are corrupted.

I've found this issue while trying to use the x509-parser crate for parsing X509 certificates in Windows PE files. The files are sometimes corrupt, and make the program panic.

Consider the AlgorithmIdentifier structure:

#[derive(Clone, Debug, PartialEq, DerSequence)]
#[error(X509Error)]
pub struct AlgorithmIdentifier<'a> {
    #[map_err(|_| X509Error::InvalidAlgorithmIdentifier)]
    pub algorithm: Oid<'a>,
    #[optional]
    pub parameters: Option<Any<'a>>,
}

The code generated by DerSequence for the try_from function looks like:

fn try_from(any: Any<'ber>) -> asn1_rs::Result<Self, X509Error> {
    use asn1_rs::nom::*;
    any.tag().assert_eq(Self::TAG)?;
    let i = any.data;
    let (i, algorithm) = FromBer::from_ber(i).finish().map_err(|_| X509Error::InvalidAlgorithmIdentifier)?;
    let (i, parameters) = FromBer::from_ber(i).finish()?;
    let _ = i;
    Ok(Self {
        algorithm,
        parameters,
    })
}

Notice the use of finish after parsing both the algorithm and parameters fields. The documentation for finish says:

warning: if the result is Err(Err::Incomplete(_)), this method will panic.

“complete” parsers: It will not be an issue, Incomplete is never used “streaming” parsers: Incomplete will be returned if there’s not enough data for the parser to decide, and you should gather more data before parsing again. Once the parser returns either Ok(), Err(Err::Error()) or Err(Err::Failure(_)), you can get out of the parsing loop and call finish() on the parser’s result

So, it looks like "streaming" parsing is being used, but I'm not sure if that's the case, or why.

[chifflier]

This is indeed a serious problem (unwanted panic in a library), thanks for reporting it I am investigating for a solution

[plusvic]

Thank you for the fix! Any plans to publish a release that includes this fix?

[chifflier]

The fix has been published in asn1-rs-derive 0.5.1 A simple cargo update on this crate should be enough (there is no breaking change).

[plusvic]

Great! Thank you!