duesee / imap-codec

Rock-solid and complete codec for IMAP
Apache License 2.0
35 stars 13 forks source link

misuse: Deserialization into incorrect types #502

Closed HenningHolmDE closed 2 weeks ago

HenningHolmDE commented 2 weeks ago

Currently, types created through deserialize are not validated. Thus, it is possible to create incorrect values, e.g.:

#!/usr/bin/env rust-script
//! Try to deserialize invalid input into a `core::Text` type
//!
//! ```cargo
//! [dependencies]
//! imap-types = { git = "https://github.com/duesee/imap-codec.git", features = ["serde"] }
//! serde_json = "1.0"
//! ```

use imap_types::core::Text;

fn main() {
    let valid_input = r#""Hello, world!""#;
    let invalid_input = r#""Hello,\rworld!""#;

    let text = serde_json::from_str::<Text>(valid_input)
        .expect("valid input should deserialize successfully");
    assert_eq!(text.as_ref(), "Hello, world!");

    let err = serde_json::from_str::<Text>(invalid_input)
        .expect_err("invalid input should not deserialize successfully");
    assert_eq!(
        err.to_string(),
        r"Validation failed: Invalid byte b'\x0d' at index 6"
    );

    println!("Success.")
}
$ rust-script deserialize_invalid_text.ers    
thread 'main' panicked at ...\deserialize_invalid_text.ers:21:10:
invalid input should not deserialize successfully: Text("Hello,\rworld!")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

At least for core::Text, it is possible to fix the issue using #[serde(try_from = "String")].

I will try to provide a corresponding PR in the next days.

duesee commented 2 weeks ago

Thank you very much, Henning! As a short explainer: The serde support happened "by accident" because we needed it quickly for another project :-) I never saw it as part of the misuse resistancy contract. But: This is inconsequent (and not documented).

Now, with the Python bindings, we have a good reason to finally rectify that. Consequently, we should extend the fuzz targets to capture serdes (de)serialization, too. (Let's think about how to do it later.)

Good news: I believe we only need a few tweaks here and there. Deserialization is guaranteed to be misuse resistant for tightly modelled types, i.e., types that enforce all invariants by definition.

This is not true for "string types", such as Atom, etc. Thus, I expect most of the validated deserialization will need to happen in the core module. It should really only be a handful of types.

(Maybe CommandContinuationRequest will be tricky. But we should get quite far.)