Problem at use secp256k1 0.22.1 with wasm

[josediegorobles]

Describe the bug
I have the error that i describe later with last release of rust-bitcoin ( no problem with previous versions).

Build environment

OS+version: 20.04.01
Rust/Cargo version: 1.61.0
Rust/Cargo target: wasm32-unknown-unknown

That's the error:

Uncaught (in promise) RuntimeError: unreachable
at __rust_start_panic (67d29ce8a8f7d806078e.module.wasm:0xc6d5d8)
at rust_panic (67d29ce8a8f7d806078e.module.wasm:0xc55f4c)
at std::panicking::rust_panic_with_hook::he2a025723e105e28 (67d29ce8a8f7d806078e.module.wasm:0x960d29)
at std::panicking::begin_panic_handler::{{closure}}::hd9f8c213ec91b9d5 (67d29ce8a8f7d806078e.module.wasm:0xa6ac56)
at std::sys_common::backtrace::__rust_end_short_backtrace::h6efd730283875809 (67d29ce8a8f7d806078e.module.wasm:0xc6c185)
at rust_begin_unwind (67d29ce8a8f7d806078e.module.wasm:0xc16d18)
at core::panicking::panic_fmt::hb02133958c1e7d35 (67d29ce8a8f7d806078e.module.wasm:0xc24531)
at rand::rngs:thread:THREAD_RNG_KEY::__init::{{closure}}::ha2ad45ff6630e75f (67d29ce8a8f7d806078e.module.wasm:0xa55aef)
at core::result::Result<T,E>::unwrap_or_else::hdc727ae752f97c76 (67d29ce8a8f7d806078e.module.wasm:0x73eae4)
at rand::rngs:thread:THREAD_RNG_KEY::__init::hfb75fbbba2c214fb (67d29ce8a8f7d806078e.module.wasm:0x80efe3)

If anyone wants to repro, check out this commit:
diba-io/bitmask-core@76e83c9

[apoelstra]

Hmm, I worry that this grounds out in the rand crate.

Your backtrace doesn't obviously go through our library ... are you able to re-run with RUST_BACKTRACE=1 or something to get more information?

[josediegorobles]

Yeah most probably is in that crate.
It doesn't change anything with RUST_BACKTRACE=1 but i know by debugging that the fail is here:
let secp = Secp256k1::new();

Thanks.

[Kixunil]

I believe you have to enable debug symbols or just avoid --release if debugging speed/size is acceptable.

[apoelstra]

I seem to remember something related to rerandomization and static contexts and wasm...

[josediegorobles]

I'm using wasm-pack build

[Kixunil]

I suspect that wasm (Rust support of wasm?) doesn't support thread locals, but couldn't find anything reasonable on the topic yet.

[apoelstra]

Okay, in #385 we started doing rerandomization on context creation ... this linked to #346 (comment). I need to read further but I think this old discussion is relevant.

[apoelstra]

Okay it looks like this was "resolved" in #407 where we added the global-context-less-secure feature, along with a huge comment in our Cargo.toml saying that you need this for WASM.

@josediegorobles can you try enabling that feature and see if it fixes your problem?

The issue here is that the rand crate panics when trying to obtain randomness, which is insane and antisocial but I guess we decided to live with it.

[josediegorobles]

Ok. The issue is that all is working great with previous versions (bitcoin 0.27 - sec256k1 0.20.2).
I will try global-context-less-secure, thanks

[Kixunil]

which is insane and antisocial

What would you like it to do? It doesn't seem there's an obvious better solution.

[apoelstra]

@Kixunil it could return a result. Or they could provide an alternate method that would just return bad randomness.

I think for now we should disable this line of code when compiling for wasm.

[josediegorobles]

@apoelstra how can i pass the feature to secp256k1 if i'm using it throught the rust-bitcoin crate?

[Kixunil]

Maybe Result would be too annoying downstream? Bad randomness sounds like a significant footgun.

How about requiring either global-context-less-secure for wasm or setting custom RNG?

[Kixunil]

@josediegorobles just add it explicitly as another dependency and activate the future. It will activate globally.

[josediegorobles]

@Kixunil thanks. I was trying that, but i wanted to know if there are a more direct alternative.

[josediegorobles]

I added

secp256k1 = { version = "0.22.1", features = ["global-context-less-secure"] }

last line on cargo.toml

It's not working but i'm cleaning and compiling again

[Kixunil]

You must use the exact same version bitcoin uses. cargo tree will tell you.

[josediegorobles]

Yeah, is the version it's using rust-bitcoin

[josediegorobles]

You can see my Cargo.toml here: https://github.com/diba-io/bitmask-core/blob/update-bdk-19/Cargo.toml

[apoelstra]

I'm a bit confused ... the new behavior showed up in rust-secp 0.23, but rust-bitcoin is using 0.22, so this shouldn't be an issue?

[josediegorobles]

Ok.
I can see in Cargo.lock, the branch that is working is
[[package]]
name = "secp256k1"
version = "0.20.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "97d03ceae636d0fed5bae6a7f4f664354c5f4fcedf6eef053fef17e49f837d0a"
dependencies = [
"secp256k1-sys",
"serde",
]

And the branch is not working say:
[[package]]
name = "secp256k1"
version = "0.22.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "26947345339603ae8395f68e2f3d85a6b0a8ddfe6315818e80b8504415099db0"
dependencies = [
"rand 0.6.5",
"secp256k1-sys",
"serde",
]

[Kixunil]

Check with cargo tree what's pulling in the old version.

[josediegorobles]

Bitcoin works with 0.22.1 so if i add secp256k1 with that version and the feature it works, but gives same error. If I use 0.20 version or 0.23 it fails.
With cargo tree i can see that secp256k1 0.22.1 is there, directly in first level, and as second level from bitcoin.

[josediegorobles]

So why is the problem with 0.22.1 and how i can get rid of that?

[Kixunil]

I don't understand the difference between works but gives error and fails.

[apoelstra]

@josediegorobles ok, I was just confused about versions. The issue was actually introduced in rust-secp 0.21. In the latest release of rust-bitcoin we jumped form 0.20 to 0.22, which is why you're seeing the problem crop up now.

Bitcoin works with 0.22.1 so if i add secp256k1 with that version and the feature it works, but gives same error. If I use 0.20 version or 0.23 it fails.

Can you elaborate on this?

[josediegorobles]

Ok, @apoelstra I'm adding directly secp256k1 0.22.1 with the feature you said and it does exactly the same. You can see in the Cargo.toml that i link previously, or in the Cargo.lock of same branch, I have to change something more? I have to try to use bitcoin 0.28 with secp256k1 0.23 or something like that?
Thanks

[apoelstra]

@josediegorobles oh! I see the issue. We have disabled randomness for the global context with global-context-less-secure but you are using a local variable as your context.

On our end, we should fix this to disable randomization for local contexts as well, but on your end you shouldn't be calling Secp256k1::new. Just use the global-context methods, e.g. SecretKey::sign_ecdsa.

[josediegorobles]

Ok, thanks!
So i use the feature global-context-less-secure and then SecretKey::sign_ecdsa?

[apoelstra]

@josediegorobles exactly.

Meanwhile we will fix our library so that you don't need to think about these things :)

[josediegorobles]

@apoelstra do you have an issue for tracking this? I'm using bdk and apparently they are with this problem so if I solve my part is going to be the same. So, do you think this issue is going to be solved soon or is very problematic? Maybe I can help with something with some guide.

[apoelstra]

This is the issue for tracking it :). And no, it's not hard at all. We just need to

• Disable the rerandomization code when compiling for wasm. (Alternately we could require wasm users enable the global-context-less-secure feature, but as you have noticed, this is a PITA when you don't have a direct rust-secp dependency, and also you don't always want the global context to be compiled.)
• Extend the "disable randomization" logic to cover Secp256k1::new as well as the global context.

I will try to do it later today.

[josediegorobles]

That's great, I will keep update about that

[Kixunil]

@apoelstra isn't it dangerous to silently decrease security just because it's a specific platform?

[apoelstra]

@Kixunil yes, but it's also dangerous to stick unexpected panics into library functions. If they could not return a Result, even though they've made enough other breaking changes that even non-rust-bitcoin downstream projects have complained about their instability, then surely they could provide an alternate method that returned a Result?

[Kixunil]

We could make it not compile unless the consumer acknowledges lower security or provides custom RNG. Also thankfully these panics are in init functions which is not that bad.

[apoelstra]

I don't think it's reasonable to ask a non-cryptographer user to have an opinion about the context rerandomization behavior ... or even a cryptographer user who is unfamiliar with libsecp. For our part IMO we should just disable rerandomization on wasm.

[Kixunil]

Why is disabling rerandomization better than forcing the user to provide RNG?

[apoelstra]

@Kixunil because if the user can't get a rng from the closest thing that exists to a standard rng package, they won't be able to get a rng from anywhere.

[Kixunil]

Isn't the problem with wasm that there's no standardized API for RNG but at least in browsers you can bind JS functions which do have the crypto API?

[josediegorobles]

So appart from the solution of this issue, what steps could be necessary to make secure the use of secp256k1 on wasm?
Btw, when do you think is going to be a solution for this issue, @apoelstra ?
Thanks

[apoelstra]

@josediegorobles none of this is required to make secp256k1 secure. There are no additional steps required on your part.

Probably we will not be able to solve the issue until @Kixunil returns after the weekend and we come to an agreement on how much of an ergonomic and coginitive load we ought to impose on our users.

[josediegorobles]

Ok, thanks for the reply.

[josediegorobles]

so @Kixunil and @apoelstra we are going to have this soon?

[apoelstra]

I will open a PR to fix it today or tomorrow..