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

Allow Transcript::new labels to come from non-rust code without leakage, UB, etc. #44

Closed
wants to merge 1 commit into from

Conversation

burdges
Copy link

@burdges burdges commented Jul 2, 2019

Merlin requires 'static protocol labels, which normally originate in Rust code for various reasons, ala session type. Yet, the label passwd to Transcript::new could reasonably originate outside Rust code when protocols are designed to be composed.

paritytech/schnorrkel-js#12 (comment)

Merlin requires `'static` protocol labels, which normally originate
in Rust code for various reasons, ala session type.  Yet, the label
passwd to new could reasonably originate outside Rust code, due to
protocols being designed to be composed.

paritytech/schnorrkel-js#12 (comment)
@WildCryptoFox
Copy link

WildCryptoFox commented Jul 2, 2019

Alternatives:

  1. Use static labels and commit to the dynamic labels as the message. Possibly as Transcript::append_label(&mut self, label: &str).
t.append_message(b"externally defined label", external_label);
  1. Use Box::leak to promote a String to a &'static str by leaking the value. Edit: To minimize leaks use a single String containing all labels, then refer to their respective slices.
let _: &'static str = Box::leak(String::from("hello world").into_boxed_str());

@burdges burdges changed the title Allow new labels to come from non-rust code without leakage, UB, etc. Allow Transcript::new labels to come from non-rust code without leakage, UB, etc. Jul 2, 2019
@burdges burdges changed the title Allow Transcript::new labels to come from non-rust code without leakage, UB, etc. Allow Transcript::new labels to come from non-rust code without leakage, UB, etc. Jul 2, 2019
@burdges
Copy link
Author

burdges commented Jul 2, 2019

Internally, merlin does 1 for Transcript::new, so always using Transcript::new(b"") and then append_label is indeed hacky, but should work.

I now think 2 cannot work in practice because someone will invoke the leaking code repeatedly.

@WildCryptoFox
Copy link

(2) shouldn't be an issue if the user can't create new labels. See my previous message as edited. Leak a single string containing all labels and refer to its slices.

@hdevalence
Copy link
Contributor

Internally, merlin does 1 for Transcript::new, so always using Transcript::new(b"") and then append_label is indeed hacky, but should work.

Note: because Merlin includes metadata in the transcript, passing an empty label into the transcript and then later adding a label is not the same as initializing a transcript with that label, by design.

The Merlin API is designed so that the parts which should be fixed statically (protocol framing and names) are &'static, and the parts which can vary at runtime (message data) are not. The protocol label is &'static because it is not supposed to vary at runtime.

You should not create a transcript with an empty protocol label. The protocol label does domain separation for your construction. If you want to do additional domain separation, you can append further messages after creating the transcript, and this works fine without having to change any lifetimes.

@burdges
Copy link
Author

burdges commented Jul 2, 2019

There is no meaningful 'static label available for Transcript::new when people bind to dynamic languages with even minimal flexibility from the dynamic language's side, but..

Yes, a static mut LABELS: Option<&'static [u8]> with a function that updates it only once actually works, cute. We cannot solve the problem that way however since modules exist in dynamic languages too.

As I noted elsewhere, a static mut LABLES: HashSet<&'static [u8]> actually works correctly, but requires alloc, so also unacceptable.

@rphmeier
Copy link

rphmeier commented Jul 2, 2019

Use Box::leak to promote a String to a &'static str by leaking the value.

Given that this exists in safe Rust, what exactly is the API meant to defend against?
correct usage of an API is of course valuable but you cannot expect to foresee all use-cases.

The protocol label is &'static because it is not supposed to vary at runtime.

Your definition of "runtime" works for pure-rust but is fairly short-sighted w.r.t. dynamic languages. The protocol label may not vary after some point in time T but is not known at compile-time. If a goal of Merlin is to avoid users hand-rolling their own (potentially broken) domain separation then it seems counter-intuitive to raise the barrier to entry for certain valid use-cases.

@wilfred-centrality
Copy link

The Merlin API is designed so that the parts which should be fixed statically (protocol framing and names) are &'static, and the parts which can vary at runtime (message data) are not. The protocol label is &'static because it is not supposed to vary at runtime.

Is this strictly necessary? Pun not intended

burdges added a commit to w3f/bls that referenced this pull request Jul 3, 2019
We ran into trouble using merlin with dynamic langauges:
paritytech/schnorrkel-js#12
dalek-cryptography/merlin#44
We only benefit minimally if at all from using merlin here anyways,
so just remove it entirely.

Also merlin is not likely to adapt anything else we noticed as
convenient:
dalek-cryptography/merlin#37
@burdges
Copy link
Author

burdges commented Jul 3, 2019

We'll use Transcript::new(b"") plus append_message maybe

@burdges burdges closed this Jul 3, 2019
burdges added a commit to w3f/schnorrkel that referenced this pull request Jul 3, 2019
We ran into trouble using merlin with dynamic langauges:
paritytech/schnorrkel-js#12
dalek-cryptography/merlin#44
This is the easiest solution.
@alinush
Copy link

alinush commented Aug 25, 2022

For what it's worth, I am also finding this design choice regrettable and not usable even within Rust itself.

I am trying to export the proof verifier from the bulletproofs library inside our smart contract language, Move, which is implemented in Rust.

By requiring Transcript::new to take a &'static [u8] domain separation tag (DST), I am forced to hard-code the same DST for anybody calling our smart contract Bulletproof verifier. However, most Bulletproof provers out there will most-certainly use a different DST. Yet, because of the &'static [u8] limitation, I cannot take their DST as user input... :'-(

I cannot use Box::leak either because I'd have to then rely on the unsafe Box::from_raw to free the memory.

Will probably have to fork and change this.

PS: I cannot "use Transcript::new(b"") plus append_message" as suggested by @burdges because, again, most Bulletproof provers already out there probably don't do this; some of them actually call Transcript::new with their DST.

@burdges
Copy link
Author

burdges commented Aug 26, 2022

I've always just told people to unsafe cast to static but I'm always calling merlin directly.. you maybe more nervous if you propagate that static through however many subcrates are in bulletrpoofs..

If you maintain a fork then maybe you'll find some other warts more visible from how academics think about transcripts and it could find more adoption than merlin.

@alinush
Copy link

alinush commented Aug 27, 2022

Hey Jeff,

If by cast to static you mean the Box-based approach I outlined above, then that would almost work, but not for us: we are not yet ready to use unsafe in our repo 😔

@burdges
Copy link
Author

burdges commented Aug 27, 2022

You do not need allocation. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=36046949c84b41372cc658cd4bb36f2c although as statements would be better.

If you're using a dependency that missuses the memory safety boundary then don't be so surprised by using unsafe.

It sounds like those bulletproofs provers are miss-using one of the good parts merlin interface though, which makes this unsafety worse.

@alinush
Copy link

alinush commented Aug 27, 2022

Solution without Box is useful to know! Thank you! 🙇 (Maybe I can just cheat and import a crate that merely implements your f, lol.)

For future reference, in case link breaks:

fn f(x: &[u8]) {
    g(unsafe { core::mem::transmute::<&[u8], &'static [u8]>(x) })
}

Not sure I follow your last claim: there's no misuse in each user $i$ computing, say, a range proof $\pi_i$ using DST $dst_i$ as:

    // Compute a range proof for 'value' committed with commitment key `pg` and `blinder`
    let mut t = Transcript::new(dst_i);
    let pi_i =
        RangeProof::prove_single(&bg, &pg, &mut t, value, &blinder, MAX_RANGE_BITS);

The problem, on our side, will be that we will need to provide a public-facing API that can verify any range proof $\pi$ with any DST $dst_i$ ... which we can't do (without unsafe) because Transcript::new must take a &'static [u8].

@burdges
Copy link
Author

burdges commented Aug 27, 2022

Yes. I meant it's good RangeProof takes t: &mut merlin::Transcript. It's bad when people put at &'static [u8] for merlin in public interfaces in Rust.

As non-Rust languages exists, it's also bad or worse that merlin tries to prevent you from passing labels this with this silliness, but these mistakes do not cancel out.

It'd all be far simpler to just say throughout the merlin documentation that domain separation depends upon your label choices so labels should come from your own code and not be abstracted.

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 this pull request may close these issues.

6 participants