Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

KeyPair FromStr and Deserialize impls crash #491

Closed
elsirion opened this issue Oct 21, 2022 · 2 comments · Fixed by #492
Closed

KeyPair FromStr and Deserialize impls crash #491

elsirion opened this issue Oct 21, 2022 · 2 comments · Fixed by #492

Comments

@elsirion
Copy link
Contributor

When trying to deserialize a KeyPair the thread crashes with

[libsecp256k1] illegal argument. rustsecp256k1_v0_6_1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)

this can be reproduced using the following test without special feature configurations by running cargo test test_broken_keypair_serde. I originally tested using serde, but could simplify it to FromStr.

    #[test]
    fn test_broken_keypair_serde() {
        let ctx = crate::Secp256k1::new();
        let keypair = KeyPair::new(&ctx, &mut thread_rng());
        let msg = keypair.secret_key().secret_bytes().to_hex();
        dbg!(&msg);
        let parsed_key: KeyPair = msg.parse().unwrap();
        assert_eq!(parsed_key, keypair);
    }

I'm not familiar enough with the low-level C API of libsecp to propose a fix keeping the unsafe code but would propose to use the global context instead and feature gate these impls accordingly.

@apoelstra
Copy link
Member

Yikes. Yeah, these are basically completely broken without the global-context feature. We will need to backport this probably several versions back.

I think we should use the global context if available and otherwise have fallback code which constructs a signing context. It will be slow but probably better than not working at all, I think.

elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 22, 2022
elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 22, 2022
elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 22, 2022
elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 22, 2022
@apoelstra
Copy link
Member

Wow, on further investigation it looks like I merged #313, which introduced this code, even though it had no tests and evidently never worked under any circumstances. All the discussion on that PR was about some unrelated terrible idea. What a process failure.

elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 23, 2022
elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 23, 2022
elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 24, 2022
elsirion added a commit to fedimint/rust-secp256k1 that referenced this issue Oct 24, 2022
apoelstra added a commit that referenced this issue Oct 25, 2022
…yPair`

1f327b4 Bump version number to v0.24.1 (elsirion)
53c1354 Fix broken `serde::Deserialize` and `FromStr` impl of `keyPair` (elsirion)

Pull request description:

  Fixes #491

ACKs for top commit:
  apoelstra:
    ACK 1f327b4

Tree-SHA512: 1af54667b7a1b310035fa35bd2aeb508e432d8c7f153ae1b9850431ba77dcc3e2194c1cda45a1ed5218d955d9284ba6512cf8ab6dafc673f23ccdad7c601b1b6
huitseeker added a commit to huitseeker/fastcrypto that referenced this issue Nov 16, 2022
huitseeker added a commit to MystenLabs/fastcrypto that referenced this issue Nov 16, 2022
* chore: upgrade to Rust 1.65

* chore: upgrade secp256k1 to 0.24.1

Fix: rust-bitcoin/rust-secp256k1#491

* chore: run clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants