informalsystems / tendermint-rs

Client libraries for Tendermint/CometBFT in Rust!
Apache License 2.0
587 stars 213 forks source link

Error while parsing Osmosis block 13590870 with secp256k1 key #1419

Closed penso closed 1 month ago

penso commented 1 month ago

What went wrong?

I get a cryptographic error in this cosmos-rust line coming from this tendermint-rs line when parsing what seems to be a valid transaction. This is the mintscan transaction properly indexed.

Steps to reproduce

This is a code sample showing it fails.

#  Cargo.toml
cosmrs = { git = "https://github.com/cosmos/cosmos-rust.git", features = [
  "cosmwasm",
  "default",
  "dev",
] }
sha256 = "1.0.3"
base64 = "0.21"
reqwest = { version = "0.11", features = ["json", "trust-dns", "gzip", "brotli", "deflate"] }
#[test]
pub fn decode_error() {
    let block_url = "https://rpc.archive.osmosis.zone/block?height=13590870";

    let response = reqwest::blocking::get(block_url).unwrap();

    let data: serde_json::Value = response.json().unwrap();

    let Some(txs) = data
        .get("result")
        .and_then(|c| c.get("block"))
        .and_then(|c| c.get("data"))
        .and_then(|c| c.get("txs"))
        .and_then(|c| c.as_array())
    else {
        return;
    };

    for (idx, tx) in txs.into_iter().enumerate() {
        let tx = tx.as_str().unwrap();

        #[allow(deprecated)]
        let bytes = base64::decode(tx).unwrap();

        let res = cosmrs::Tx::from_bytes(&bytes);
        let hash = sha256::digest(bytes.as_slice()).to_uppercase();

        match res {
            Ok(_) => {
                println!("[{}] [{hash}] ok", idx);
            }
            Err(e) => {
                println!("[{}] [{hash}] error: {:?}", idx, e);
            }
        }
    }
}

Output:

[0] [D99D15DAE73ACEAFF7EB113AFC5C03F2BE98CD657702DB5737056146FCD1BAEA] ok
[1] [D6566C8458A2B4039DB7DB667D477BAE4E696EDF45BA0AB6433F4C6AD3505927] ok
[2] [DD711F90DEB9E8E4F8FDF4A880A9BD7872A53CF910809E633ABB4F1C785CC7CD] ok
[3] [86F12289502E4C7A0C9D268203E604BF568D359DA8D4E5B21EB068E00FDBA6E5] ok
[4] [06F0D433B20C47489239F930715D5FB0786527DCD330BA944F84CCC4999CE947] ok
[5] [5A887B028C421119CDA2323DA66E580B99ADC22626579C572B95210B42823465] ok
[6] [97D93F73D8418BFC9740A50C300A5E58E3D4BE98CA0F283206A7857FEF6EF144] ok
[7] [093C1B648BF2FF1ABCBF912D829C694E3CF163F4698284F64AE3914318CCF2A9] ok
[8] [14A62A59EB83089AAEB34249E3E8124BE66F7A32E65CAAC174FABFC73657C174] ok
[9] [AC1D08A30FAA0C12BED3C95FE37E25814764A26B160D91887841C14B7DF6529F] error: cryptographic error

Location:
    /Users/penso/.cargo/git/checkouts/cosmos-rust-5eb3768b2a88fb5e/c0fa7b4/cosmrs/src/crypto/public_key.rs:137:42

[10] [95580D29926DF1E194EC81CE0D9000FD4380DCFB3AC26DD5905D724E3097C056] ok
[11] [67EA436BD157F1731FE6B5E5D993E68AF84E21F8D081ABD4DF499D1AD5533726] ok
[12] [E2C8D6C44CF6C101A9B6B0ACB89BB9AB3EC7F4A88A092160C18D51BF0F5DA859] ok
[13] [477C001348D5847B4D2EC308E87B9252FC5B96F131798F384BED8BC2F830A2DA] ok

Definition of "done"

We should be able to parse this public key without error

tony-iqlusion commented 1 month ago

Seems similar to #1417, but in this case the public key is invalid:

AF390E8EB13DC2C89F91D09EB5BEF64367BD3BD0C3446C270A6277335228E7DF87

It's 33-bytes like we'd expect: SEC1 tag || secp256k1 x-coordinate, but where a valid SEC1 tag is: 0x00, 0x02, 0x03, 0x04, this key for whatever reason has 0xAF, which is not a valid SEC1 tag.

tony-iqlusion commented 1 month ago

Really this is a CosmRS (and Osmosis) issue as opposed to a tendermint-rs one. CosmRS tries to eagerly parse the public key, and it seems like we just won't be able to rely on chains not to put out garbage public keys. Those garbage public keys likely represent some sort of bug in Osmosis where it failed to validate the key in the first place, but once they wind up in the chain data there's really no way of fixing them.

penso commented 1 month ago
  1. Agreed cosmos-rust shouldn't try to validate public keys, there is too much garbage out there. Maybe it should be a specific call like public_key.is_valid() to prevent such issues but looking at the code you can't create an invalid PublicKey so it should be done differently, like holding the raw bytes for the keys and only call Secp256k1::from_sec1_bytes when is_valid() or parse() is called.
  2. Is there a deeper issue if the Osmosis chain accepted invalid public keys as part of a successful transaction? Any security issue here? I guess those show up on mintscan because mintscan is using code not parsing/validating public keys.
tony-iqlusion commented 1 month ago

Is there a deeper issue if the Osmosis chain accepted invalid public keys as part of a successful transaction?

Possibly. You might open an Osmosis issue about this block and see if they can figure out what happened.

penso commented 1 month ago

Closing this, will followup if any news.

tony-iqlusion commented 1 month ago

@penso as a stopgap, you can parse these transactions as e.g. cosmos_sdk_proto::cosmos::tx::v1beta1::Tx rather than cosmrs::Tx

ValarDragon commented 1 month ago

This is likely coming from the SDK not validating this first byte here: https://github.com/cosmos/cosmos-sdk/blob/main/crypto/keys/secp256k1/secp256k1.go#L203-L211

tony-iqlusion commented 1 month ago

I opened a Cosmos SDK bug: https://github.com/cosmos/cosmos-sdk/issues/20406