rusticata / der-parser

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

BerObject struct changes #26

Closed sharksforarms closed 4 years ago

sharksforarms commented 4 years ago

Currently looking at: https://redmine.openinfosecfoundation.org/issues/2977

I've run into an issue where to be able to implement the detect-asn1 checks, some fields cannot be accessed with the current API, such as the length len of the BerObjectHeader.

The parse_ber function currently returns a BerObject

#[derive(Debug, Clone)]
pub struct BerObject<'a> {
    pub class: BerClass,
    pub structured: u8,
    pub tag: BerTag,

    pub content: BerObjectContent<'a>,
    pub raw_tag: Option<&'a [u8]>,
}

Note, the similarities with BerObjectHeader

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct BerObjectHeader<'a> {
    pub class: BerClass,
    pub structured: u8,
    pub tag: BerTag,
    pub len: u64,

    pub raw_tag: Option<&'a [u8]>,
}

And the following which is called in parse_ber after both the header/content are parsed:

impl<'a> BerObject<'a> {
    pub fn from_header_and_content<'hdr>(
        hdr: BerObjectHeader<'hdr>,
        c: BerObjectContent<'hdr>,
    ) -> BerObject<'hdr> {
        BerObject {
            class: hdr.class,
            structured: hdr.structured,
            tag: hdr.tag,
            content: c,
            raw_tag: hdr.raw_tag,
        }
    }

(Note, the len from hdr is not copied and can no longer be accessed, the .to_header() method on the BerObject explicitly sets this value to 0)

What if a BerObject looked like this?

/// Representation of a DER-encoded (X.690) object
#[derive(Debug, Clone)]
pub struct BerObject<'a> {
    pub header: BerObjectHeader<'a>,
    pub content: BerObjectContent<'a>,
}

Let me know what you think and I can start working on it!

chifflier commented 4 years ago

Agreed, this causes a lot of code duplication, and I can't remember the original reason for that. 4.0 may be a good opportunity to factorize the code

chifflier commented 4 years ago

I'll look into that

chifflier commented 4 years ago

I found no good reason for fields duplication, so this is now implemented. Thanks!

That said, if you need that for the suricata PR, you'll have to wait for der-parser 4.0 release [*], which is a major (and API-breaking) upgrade. I'm still running some checks, documenting upgrade, and looking at the resulting API (for ex in x509-parser) before the final release.

[*] especially since 3.0 is used in other dependencies in suricata, like kerberos, snmp, x509 etc. They all need to be upgraded before 4.0 can be used!