aptos-labs / petra-wallet

Public issue tracker for Petra Wallet on Aptos
https://petra.app
87 stars 54 forks source link

Petra signatures fail to pass s malleability checks. #32

Closed humb1t closed 2 months ago

humb1t commented 2 months ago

When I'm using Petra to sign a transaction, resulted signature do not bypass Aptos SDK check for s malleability: Test:

    let hex_decoded = hex::decode("ad0828843228048fee9ead35cf0decc8155342e967993851f70bfc0bb9e97719a094c6f956e5cd65872fa769e7dce26f6e637eb4ff70c3f6aafc5f06fbc300ad").unwrap();
    Signature::check_s_malleability(&hex_decoded).unwrap();

checks in SDK:

    pub fn check_s_malleability(bytes: &[u8]) -> std::result::Result<(), CryptoMaterialError> {
        if bytes.len() != ED25519_SIGNATURE_LENGTH {
            return Err(CryptoMaterialError::WrongLengthError);
        }
        if !Ed25519Signature::check_s_lt_l(&bytes[32..]) {
            return Err(CryptoMaterialError::CanonicalRepresentationError);
        }
        Ok(())
    }

    /// Check if S < L to capture invalid signatures.
    fn check_s_lt_l(s: &[u8]) -> bool {
        for i in (0..32).rev() {
            match s[i].cmp(&L[i]) {
                Ordering::Less => return true,
                Ordering::Greater => return false,
                _ => {},
            }
        }
        // As this stage S == L which implies a non canonical S.
        false
    }

Does Petra sign in low order?

humb1t commented 2 months ago

Tested using both - signTransaction and signMessage, none of these methods return signature that can be validated by Rust SDK

kent-white commented 2 months ago

Hey, @humb1t I think there are no malleability checks on other sdks so we haven't run into this before. We should have a solution for you by Monday.

Thanks for the patience.

humb1t commented 2 months ago

Great news, thank you @kent-white 💜

kent-white commented 2 months ago

@gregnazario is responding to you in slack, sounds like this is more complicated than I originally thought.

alinush commented 2 months ago

Hi @humb1t,

This is very surprising, since Petra uses https://github.com/aptos-labs/aptos-ts-sdk, which uses TweetNacl.js to Ed25519-sign, which should produce canonical signatures.

The only thing I could imagine is some sort of heisenbug that once in a blue moon produces a non-canonical signature.

Have you been able to reliably reproduce this issue since?

humb1t commented 2 months ago

Hey @alinush, it is happening to all signatures. I tried different examples of signatures. Generate in our application and the ones I found on GitHub.

#[test]
fn test_s_malleability_check() {
    let _public_key = PublicKey::from_encoded_string(
        "0x8ca2de88d68153655ec156226361ecb295ac1b31fc896b277cb11a9d90e389be",
    )
    .unwrap();
    // let hex_decoded = hex::decode("ad0828843228048fee9ead35cf0decc8155342e967993851f70bfc0bb9e97719a094c6f956e5cd65872fa769e7dce26f6e637eb4ff70c3f6aafc5f06fbc300ad").unwrap();
    // let hex_decoded = hex::decode("397be8ba1cdb31072922a733049b65fbe2855a02a532b0c18cad38fa5e53ba48445f4703fb65604681257a067bea4d847191ea63ec8608e4bd1a9ee63718210f").unwrap();
    // ^^^ FROM https://github.com/aptos-labs/aptos-core/issues/5696
    // let hex_decoded = hex::decode("34fae90d6f458709c9ee955ce311062112658084c925ae4efe5febae81e9a821b6d5b7be73e3a1487167c8db445dc4043349b571c27f35f66a6922eb53b51004").unwrap();
    // ^^^ FROM https://github.com/aptos-labs/aptos-core/issues/3581
    // let hex_decoded =
    //     hex::decode("88fbd33f54e1126269769780feb24480428179f552e2313fbe571b72e62a1ca1").unwrap();
    // ^^^ FROM https://github.com/aptos-labs/aptos-core/issues/840
    let hex_decoded = hex::decode("c116c8b89f1861402a7a2c82d607d389e5b4ab3957628467952543472da9a51c7ef534a2e2dc3d4053de2441b7c9206a7f73a5ab8156405fb766dd4e13dc100b").unwrap();
    Signature::check_s_malleability(&hex_decoded).unwrap();

and they all fail to pass this check with calledResult::unwrap()on anErrvalue: CanonicalRepresentationError

I'm using:

aptos-sdk = { git = "https://github.com/aptos-labs/aptos-core", branch = "mainnet" }
aptos-types = { git = "https://github.com/aptos-labs/aptos-core", branch = "mainnet" }
humb1t commented 2 months ago

@gregnazario I tried devnet branch and no luck either. Which branch/commit should I try?

humb1t commented 2 months ago

Was able to pass the check using forked devnet, thanks a lot for you help 💜