Randomize context on creation #385

tcharding · 2022-01-25T04:57:31Z

Currently it is easy for users to mis-use our API because they may not know that randomize() should be called after context creation for maximum defence against side channel attacks.

This PR entails the first two parts of the plan outlined in #388. The commit messages are a bit light of information as to why we are doing this so please see #388 for more context.

In light of @real-or-random's comment about verification contexts the randomization is done in gen_new i.e., for all contexts not just signing ones.

Also, I think we should add some docs about exactly what randomization buys the user and what it costs. I do not know exactly what this is, can someone please write a sentence or two that we can include in the docs to gen_new?

@TheBlueMatt please review patch 4.

Resolves: #225

Note: This is a total re-write of the original PR, most of the discussion below is stale. Of note, the additional API that takes a seed during construction is not implemented here.

TheBlueMatt · 2022-01-25T06:11:50Z

It is my understanding that there are no known sidechannel attacks in unrandomized contexts, but randomization provides a additional layer which may protect against unknown sidechannel leakages. That said, I agree it would be nice to nudge users towards randomization. Instead of encouraging a dependency, we should also provide a constructor that takes 32 bytes of random data and point users to that as well.

apoelstra · 2022-01-25T13:19:00Z

src/lib.rs

+//! random keys or to re-randomize a context object, compile with the `rand`
+//! feature. Unless you _really_ do not want the `rand` dependency consider
+//! using the `rand` feature, creating contexts without randomizing can leave
+//! you open to side channel attacks. To de/serialize objects with serde,


"Can leave you open to sidechannel attacks" is definitely too strong. I would invert this whole paragraph to say something like "If you are willing to use a rand dependency, we have enabled an additional defense-in-depth sidechannel protection for our context objects, which re-blinds certain operations on secret key data".

Use your suggested text, almost verbatim. The relevant section now reads

//! To minimize dependencies, some functions are feature-gated. To generate //! random keys or to re-randomize a context object, compile with the `rand` //! feature. If you are willing to use the `rand` dependency, we have enabled an //! additional defense-in-depth sidechannel protection for our context objects, //! which re-blinds certain operations on secret key data. To de/serialize //! objects with serde, compile with "serde". **Important**: `serde` encoding is //! **not** the same as consensus encoding!

apoelstra · 2022-01-25T13:19:49Z

concept ACK this change, though I'd like the documentation to be much softer.

tcharding · 2022-01-25T21:47:28Z

Instead of encouraging a dependency, we should also provide a constructor that takes 32 bytes of random data and point users to that as well.

If such an API existed users have to get the 32 bytes of random data from somewhere, right? In other words, would providing this API just be a way that users are not forced to use rand but can pick their own randomness crate? Is this something folks will really want? Or am I missing something?

EDIT: OOOh, if someone already has a dependency on a different version of rand than we use then this API would allow them to not introduce a second one.

TheBlueMatt · 2022-01-25T22:08:18Z

If such an API existed users have to get the 32 bytes of random data from somewhere, right? In other words, would providing this API just be a way that users are not forced to use rand but can pick their own randomness crate? Is this something folks will really want? Or am I missing something?

We use a similar API extensively in LDK, in part to support various language bindings - we expose the "get 32 bytes of random data" function in Javascript, which users can implement as they see fit based on their platform (ie usually calling window.crypto or the alternatives that exist in node.js). But we also support C++ where someone may be running on a hardware wallet (or other constrained environment). In general LDK refuses to take any system-specific dependencies, including rand which guesses the best way to get randomness based on the target tuple. This mostly works, but isn't great IMO. LDK of course already manually randomizes so its not like it'll make a difference for us, but just as a general principle I find it useful to always have a "just give me the random bytes" method instead of relying on a specific dependency to Do The Right Thing always.

tcharding · 2022-01-26T02:50:21Z

Cool, thanks.

tcharding · 2022-01-27T03:02:31Z

@TheBlueMatt I had a go at adding an API that includes functions that accept 32 bytes of seed data.

TheBlueMatt

Thanks for doing this!

TheBlueMatt · 2022-01-27T03:14:24Z

src/context.rs

+        /// No randomization is done, this context is perfectly safe but for additional
+        /// defense-in-depth side channel protection consider using [`new_seeded_randomize`].
+        pub fn new_no_randomize() -> Secp256k1<All> {
+            Secp256k1::gen_new_no_randomize()


Honestly I feel like new_no_randomize should randomize if the rand-std feature is on. If one crate depends on rust-secp256k1 but doesn't want to force the rand dependency, they may call this, but if another crate enables the rand dependency we might as well use it - does randomization take any material amount of time? (cc @apoelstra)

Nah, it takes roughly as long as a signing operation -- which is nontrivial but very little compared to the rest of the context creation. It's hard for me to imagine a user who'd care about it.

In the future though we are moving away from dynamic precomputation toward one where all the precomp is done at compile time ... in which case randomization would be the difference between "effectively instant, you can do it inside loops" and "takes dozens of microseconds, don't loop it".

We haven't decided yet how to approach this, since we definitely don't want to discourage re-randomization.

@TheBlueMatt honestly what I'd like to see is rerandomizing after every signature in the case that rand is enabled. Lemme chat on IRC about the performance implications of this. Typically people are not signing in tight loops and wouldn't mind a doubling in signing time.

Can't we just leave all the functions as-is but add randomization if rand-std is available?
That way downstream users can enable rand-std even transitively and improve side-channel resistance
and the performance price is not too high. (and this is going to be the main advantage of contexts soon as everything else becomes statically precomputed)

I like this idea. I opened #388 to discuss it.

TheBlueMatt · 2022-01-27T03:14:43Z

src/context.rs

+        #[cfg(feature = "rand-std")]
+        #[cfg_attr(docsrs, doc(cfg(feature = "rand-std")))]
+        pub fn verification_only_randomize() -> Secp256k1<VerifyOnly> {
+            Secp256k1::gen_new_randomize()


Does randomization for verification contexts do anything? cc @apoelstra

Nope, so this specific function should be dropped :) As a general rule, "verification" is synonymous with "not touching secret data, no sidechannel protection needed".

Tim (real-or-random) pointed out to me in a different thread that it's not a no-op, because the current code does not actually have a notion of a "verification-only context" anymore. But nor does it do anything useful, it just wastes time.

Indeed, it's a waste of time. Unless you then use the context then to sign, which would now be possible upstream but our type system here will prevent it. So yes, no need to randomize verification contexts here.

apoelstra

ACK 0805d31

Great first step toward simplifying our rerandomization API. Messes with cargo features so requires a major version bump, cannot merge until after #390

apoelstra · 2022-02-03T15:25:27Z

Oops, needs rebase :)

Instead of providing a mechanism for users to opt out of randomization we can just feature gate the call site i.e., opportunistically randomize the global context on creation if `rand-std` feature is enabled.

Randomize context on creation if `rand-std` feature is enabled.

We recently implemented opportunistic randomization of the context object if the the `rand-std` feature is enabled. Both for the global context and also for signing context constructors. Add documentation about `rand-std` feature in relation to the context object.

Now that we opportunistically randomize the context on creation if `rand-std` is enabled it would be nice to encourage users who do not wish to use `rand-std` to randomize the context. We already have an API to do this but it requires a separate call to do so. Instead of adding a bunch of additional constructors elect to add documentation to the current constructors guiding users towards randomization.

tcharding · 2022-02-03T21:37:11Z

Another instance of forgetting to fix the commit log when rebasing, patch 2 commit log (incl. commit brief message) had to be changed to remove reference to 'signing' since the patch randomizes for all contexts.

apoelstra

ACK 8339ca5

apoelstra · 2022-02-16T23:44:29Z

In light of #346 (comment) I think we need a followup to this before we can do a minor release

apoelstra · 2022-02-16T23:45:33Z

Also we can't remove a feature in a minor release, oops.

tcharding · 2022-02-18T08:37:09Z

Also we can't remove a feature in a minor release, oops.

I'm still a bit lost as to minor/major and when each is coming. Just for the record devs are right to PR changes anytime and its the folks merging (cough you) that are burdened with the task of keeping in mind what can go into which release, right?

apoelstra · 2022-02-19T19:09:12Z

@tcharding correct, people can PR whatever they want whenever they want, and I do my best to keep track of what can be merged and at what time. In rust-bitcoin we have a set of labels to help here, while rust-secp is lower volume and I handle it ad-hoc.

tcharding · 2022-02-20T12:57:50Z

Legend.

…l context (fixup for #407) c1bb316 Make global-context-less-secure actually enable the global context (Elichai Turkel) Pull request description: In #407 we restored the `global-context-less-secure` feature, but it didn't actually do anything because #385 changed all the cfg checks on the whole module to depend on `global-context`, so we need to enable `global-context` in order to make that module compile. so before all this, users could enable *just* `global-context-less-secure` without enabling the `global-context`, and after this PR it will behave the same. (this will not enable the randomization because of: https://github.com/rust-bitcoin/rust-secp256k1/blob/1cf2429b12ff06a04a5c882f2f5d918042629660/src/context.rs#L51) ACKs for top commit: apoelstra: ACK c1bb316 Tree-SHA512: edc7b4916b359a0696cc25f498bc52ad340f981ad6b01b83b68966d6179200bac6acb96f5480157e24c605b5552bdd7b6eb8770bc9a2c5734da3df11c021fb5b

apoelstra reviewed Jan 25, 2022

View reviewed changes

tcharding force-pushed the randomize-context branch from 980e30b to 8b4ec1b Compare January 25, 2022 21:48

tcharding force-pushed the randomize-context branch 2 times, most recently from 10e779f to 10128c8 Compare January 25, 2022 23:36

tcharding marked this pull request as draft January 25, 2022 23:44

tcharding force-pushed the randomize-context branch 11 times, most recently from c5e8f9e to 2cd66fa Compare January 27, 2022 03:00

TheBlueMatt reviewed Jan 27, 2022

View reviewed changes

apoelstra mentioned this pull request Jan 27, 2022

Context randomization tracking issue #388

Open

tcharding force-pushed the randomize-context branch 2 times, most recently from 37273e7 to f3511fa Compare February 1, 2022 05:34

tcharding marked this pull request as ready for review February 1, 2022 05:46

tcharding force-pushed the randomize-context branch 3 times, most recently from 8dea6d4 to 0805d31 Compare February 1, 2022 23:02

apoelstra previously approved these changes Feb 3, 2022

View reviewed changes

Remove feature global-context-less-secure

a0465ea

Instead of providing a mechanism for users to opt out of randomization we can just feature gate the call site i.e., opportunistically randomize the global context on creation if `rand-std` feature is enabled.

tcharding dismissed apoelstra’s stale review via 1da5160 February 3, 2022 21:34

tcharding force-pushed the randomize-context branch from 0805d31 to 1da5160 Compare February 3, 2022 21:34

tcharding added 3 commits February 4, 2022 08:36

Randomize context on creation

1693d51

Randomize context on creation if `rand-std` feature is enabled.

tcharding force-pushed the randomize-context branch from 1da5160 to 8339ca5 Compare February 3, 2022 21:36

tcharding requested review from real-or-random, apoelstra, elichai and TheBlueMatt February 3, 2022 21:38

apoelstra approved these changes Feb 4, 2022

View reviewed changes

apoelstra merged commit 86447ee into rust-bitcoin:master Feb 4, 2022

tcharding deleted the randomize-context branch February 9, 2022 06:18

tcharding mentioned this pull request Feb 17, 2022

Wasm, Contexts, and Entropy #346

Open

elichai mentioned this pull request Mar 28, 2022

Make global-context-less-secure actually enable the global context (fixup for #407) #431

Merged

apoelstra mentioned this pull request Jul 4, 2022

Problem at use secp256k1 0.22.1 with wasm #470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Randomize context on creation #385

Randomize context on creation #385

tcharding commented Jan 25, 2022 •

edited

Loading

TheBlueMatt commented Jan 25, 2022 •

edited

Loading

apoelstra Jan 25, 2022

tcharding Jan 25, 2022

apoelstra commented Jan 25, 2022

tcharding commented Jan 25, 2022 •

edited

Loading

TheBlueMatt commented Jan 25, 2022

tcharding commented Jan 26, 2022

tcharding commented Jan 27, 2022

TheBlueMatt left a comment

TheBlueMatt Jan 27, 2022

apoelstra Jan 27, 2022

apoelstra Jan 27, 2022

elichai Jan 27, 2022

apoelstra Jan 27, 2022

TheBlueMatt Jan 27, 2022

apoelstra Jan 27, 2022 •

edited

Loading

apoelstra Feb 3, 2022

real-or-random Feb 3, 2022

apoelstra left a comment

apoelstra commented Feb 3, 2022

tcharding commented Feb 3, 2022 •

edited

Loading

apoelstra left a comment

apoelstra commented Feb 16, 2022

apoelstra commented Feb 16, 2022

tcharding commented Feb 18, 2022

apoelstra commented Feb 19, 2022

tcharding commented Feb 20, 2022

Randomize context on creation #385

Randomize context on creation #385

Conversation

tcharding commented Jan 25, 2022 • edited Loading

TheBlueMatt commented Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 25, 2022

tcharding commented Jan 25, 2022 • edited Loading

TheBlueMatt commented Jan 25, 2022

tcharding commented Jan 26, 2022

tcharding commented Jan 27, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Feb 3, 2022

tcharding commented Feb 3, 2022 • edited Loading

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Feb 16, 2022

apoelstra commented Feb 16, 2022

tcharding commented Feb 18, 2022

apoelstra commented Feb 19, 2022

tcharding commented Feb 20, 2022

tcharding commented Jan 25, 2022 •

edited

Loading

TheBlueMatt commented Jan 25, 2022 •

edited

Loading

tcharding commented Jan 25, 2022 •

edited

Loading

apoelstra Jan 27, 2022 •

edited

Loading

tcharding commented Feb 3, 2022 •

edited

Loading