w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
309 stars 93 forks source link

Keypair::vrf_sign ignores (partially) passed RNG #70

Closed ghost closed 3 years ago

ghost commented 3 years ago

When attempting to using vrf sign in #[no_std] context, no rand features, and with a custom rng attached to my transcript (via attach_rng) I got a panic, since schnorrkel was attempting to use os-provided rng.

I think it might be due to the "extra" transcript being generated anew without basing it on the existing's transcript rng...

Backtrace ``` test tests::verify_vrf_sign ... thread 'main' panicked at 'Attempted to use functionality that requires system randomness!!', /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/lib.rs:250:55 stack backtrace: 0: rust_begin_unwind at /rustc/07e0e2ec268c140e607e1ac7f49f145612d0f597/library/std/src/panicking.rs:493:5 1: core::panicking::panic_fmt at /rustc/07e0e2ec268c140e607e1ac7f49f145612d0f597/library/core/src/panicking.rs:92:14 2: core::panicking::panic_str at /home/karrq/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:57:5 3: ::fill_bytes at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/lib.rs:250:55 4: merlin::transcript::TranscriptRngBuilder::finalize at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/merlin-3.0.0/src/transcript.rs:329:13 5: ::witness_bytes_rng at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/context.rs:160:21 6: schnorrkel::context::SigningTranscript::witness_bytes at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/context.rs:97:9 7: schnorrkel::context::SigningTranscript::witness_scalar at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/context.rs:90:9 8: schnorrkel::vrf::::dleq_proove at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/vrf.rs:637:21 9: schnorrkel::vrf::::vrf_sign_extra at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/vrf.rs:678:40 10: schnorrkel::vrf::::vrf_sign at /home/karrq/.cargo/registry/src/github.com-1ecc6299db9ec823/schnorrkel-0.10.1/src/vrf.rs:667:9 ```
burdges commented 3 years ago

Yes exactly, just use a method that exposes the extra transcript..

keypair.vrf_sign_extra( input_t, attach_rng(Transcript::new(b"VRF"),rng) )

I doubt input_t even needs attach_rng btw.

I've never added convenience methods with rng control because normal users should never change the rng. In theory, we could've a feature that disables all features that really require an rng, like multi-signatures, and then uses a deterministic rng.

ghost commented 3 years ago

Ok I'll try attaching the rng to input_t... Because as it is currently I can't attach the same rng to both (since I'd need a double mutable reference).

I was also thinking of changing the API a bit (if possible) by providing a "state machine vrf", so you'd provide only one of the transcript at a time or something like that...


After looking at the vrf_sign_extra code I see what you meant with input_t not needing an rng, since it's just used to create a hash and no rng needed functions are used for that.

ghost commented 3 years ago

Indeed after only attaching the rng to the extra transcript it doesn't panic anymore.

To note is also that the same procedure applies for verification, thus vrf_verify should be avoided when providing a custom rng and vrf_verify_extra should be used instead and E should have the custom rng attached.

Thanks for your help!

burdges commented 3 years ago

Actually just vrf_verifiy should be fine for verification, either way, unless I cut and pasted the extra transcript wrong.

I should rename extra to aux probably. It's just data the VRF signs without including inside the VRF input, but aux is a more normal term elsewhere in crypto, ala AEAD.

ghost commented 3 years ago

I agree on the vrf_verify: upon closer inspection no rng is used anywhere so even not attaching any rng would be fine

burdges commented 3 years ago

All this complexity is purely to provide a sensible default CSPRNG when required. It's as if the whole schnorrkel crate has a type parameter, which defaults to rand::thread_rng.