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 <#996 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH474K5ZSZNTTLOWMO76TYHFREBAVCNFSM6AAAAABAADQ6MOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZVGQ3TAMJXGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>