Skip to content
New issue

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

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

Already on GitHub? # to your account

Error while parsing Osmosis block 13590870 with secp256k1 key #1419

Closed
penso opened this issue May 9, 2024 · 8 comments
Closed

Error while parsing Osmosis block 13590870 with secp256k1 key #1419

penso opened this issue May 9, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@penso
Copy link
Contributor

penso commented May 9, 2024

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

@penso penso added the bug Something isn't working label May 9, 2024
@tony-iqlusion
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

penso commented May 9, 2024

  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
Copy link
Collaborator

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
Copy link
Contributor Author

penso commented May 9, 2024

Closing this, will followup if any news.

@penso penso closed this as completed May 9, 2024
@tony-iqlusion
Copy link
Collaborator

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

@ValarDragon
Copy link

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
Copy link
Collaborator

I opened a Cosmos SDK bug: cosmos/cosmos-sdk#20406

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants