Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GeneralizedTime encodes unaccepted value. #57

Closed
5225225 opened this issue Dec 23, 2021 · 2 comments · Fixed by #150
Closed

GeneralizedTime encodes unaccepted value. #57

5225225 opened this issue Dec 23, 2021 · 2 comments · Fixed by #150
Labels
area/types Related to rasn’s types for ASN.1 help wanted Extra attention is needed kind/bug Something isn't working

Comments

@5225225
Copy link

5225225 commented Dec 23, 2021

My fuzzer for this was

#![no_main]
use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
    if let Ok(value) = rasn::der::decode::<rasn::types::Open>(data) {
        assert_eq!(value, rasn::der::decode(&rasn::der::encode(&value).unwrap()).unwrap());
    }
});

Not sure if this a valid round fuzz test, feel free to close as invalid if not.

Reproducer is

fn main() {
    let data = [
        24, 19, 43, 53, 49, 54, 49, 53, 32, 32, 48, 53, 50, 52, 48, 57, 52, 48, 50, 48, 90,
    ];

    let value = rasn::der::decode::<rasn::types::Open>(&data).expect("passes");

    rasn::der::decode::<rasn::types::Open>(&rasn::der::encode(&value).unwrap()).expect("fails");
}

Fails with thread 'main' panicked at 'fails: NoValidChoice { name: "Open" }', src/main.rs:7:81

@XAMPPRocky
Copy link
Collaborator

Thank you for your issue! This seems maybe valid, it's definitely a difference at least, though I'm not sure why yet, and not entirely sure how to fix it.

The supplied data is a GeneralizedTime type encoded as +51615 0524094020Z, where once as it's decoded and re-encoded it's encoded +516150524094020Z (notice the missing space), which is then rejected by the decoder.

Here's the snippets of where this encoded/decoded.

https://github.com/XAMPPRocky/rasn/blob/441810cbaefddce2e2a6612acc73f2d5e1775707/src/ber/enc.rs#L340-L355

https://github.com/XAMPPRocky/rasn/blob/441810cbaefddce2e2a6612acc73f2d5e1775707/src/ber/de.rs#L290-L296

@XAMPPRocky XAMPPRocky changed the title Failure to round trip decode fuzzed data Difference in GeneralizedTime encodes unaccepted value. Dec 23, 2021
@XAMPPRocky XAMPPRocky changed the title Difference in GeneralizedTime encodes unaccepted value. GeneralizedTime encodes unaccepted value. Dec 23, 2021
@XAMPPRocky XAMPPRocky added the kind/bug Something isn't working label Dec 23, 2021
@XAMPPRocky XAMPPRocky added help wanted Extra attention is needed area/types Related to rasn’s types for ASN.1 labels May 8, 2022
@Nicceboy
Copy link
Contributor

Nicceboy commented Aug 24, 2023

I am trying to fix this, but it seems that the decoder is much more "loose", than the spec allows for GeneralizedTime.

If we go with the spec, it should be easy to handle all variations, but if we compare to other implementations (asn1tools), they handle more cases.
E.g. they accept both , comma and . dot, or missing seconds or minutes, or also +/- offsets in encoded values, while the BER should never produce such outputs.

Actually I think this applies only for CER and DER, but is missing from them. So I guess both are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/types Related to rasn’s types for ASN.1 help wanted Extra attention is needed kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants