Code accepts inconsistent serialised ChainId objects

[mina86]

Bug Summary

It’s possible to craft JSON or Borsh serialised object which deserialises into an inconsistent ChainId object which has invalid revision_number field.

Demonstration

use borsh::BorshDeserialize;
use ibc_core_host_types::identifiers::ChainId;

fn evil_borsh() -> ChainId {
    ChainId::try_from_slice(b"\x06\0\0\0foo-42\x45\0\0\0\0\0\0\0").unwrap()
}

fn evil_json() -> ChainId {
    serde_json::from_str(r#"{"id": "foo-42", "revision_number": 69}"#).unwrap()
}

fn show(id: ChainId) {
    println!("id: {}; revision: {}", id.as_str(), id.revision_number());
        // → id: foo-42; revision: 69
    let other = ChainId::new(id.as_str()).unwrap();
    println!("id: {}; revision: {}", other.as_str(), other.revision_number());
        // → id: foo-42; revision: 42
    // assert_eq!(id, other); // → panics
}

fn main() {
    show(evil_borsh());
    show(evil_json());
}

Solution

Either serialisation format should change or deserialisation should verify the object is valid.

IMO ideally ChainId would be serialised as a String without the revision_number and the revision would be filled in when deserialising. Alas, this would be an ABI breaking change.

ABI-stable alternative is for deserialise implementations to verify that the parsed object is correct.

(Side note: IMO revision_number shouldn’t even be a field in ChainId and instead it should be parsed when requested but that’s a separate matter).

Version

ibc-core-host-types = { version = "0.48.1", features = ["serde", "borsh"] }

[Farhad-Shabani]

Awesome info @mina86! Thanks for flagging this. We'll dive into it and figure out the best way to tackle the situation. I have a hunch that it might be the case for other identifiers too.

[mina86]

With other identifiers the problem is that deserialisation will accept arbitrary strings even if they contain invalid characters or are invalid length. It's also an issue but subtly different one.

On Fri, Dec 1, 2023, 06:10 Farhad Shabani @.***> wrote:

Awesome info @mina86 https://github.com/mina86! Thanks for flagging this. We'll dive into it and figure out the best way to tackle the situation. I have a hunch that it might be the case for other identifiers too.

— Reply to this email directly, view it on GitHub https://github.com/cosmos/ibc-rs/issues/996#issuecomment-1835470175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH474K5ZSZNTTLOWMO76TYHFREBAVCNFSM6AAAAABAADQ6MOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZVGQ3TAMJXGU . You are receiving this because you were mentioned.Message ID: @.***>