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

Randomize contexts by default? #225

Closed
real-or-random opened this issue Jul 6, 2020 · 3 comments · Fixed by #385
Closed

Randomize contexts by default? #225

real-or-random opened this issue Jul 6, 2020 · 3 comments · Fixed by #385

Comments

@real-or-random
Copy link
Collaborator

Given that getting entropy is easy in Rust (thread_rng at least with std), we could optimistically call randomize() on every new context that we give to the user. I believe the reason why upstream doesn't do this is because it's just not clear how to obtain entropy in C.

An issue is that we'll need to disable this without std, which will be a subtle loss of security if you switch std off. Ignoring the current API, I think the proper way to do this is to let new create randomized contexts and make this only available with std, and have a verbose function new_no_randomize. But that's a pretty large API break. Hm.

If we don't want to do any of this, we should at least add randomize our examples.

Related to #224 and rust-random/rand#313.

@elichai
Copy link
Member

elichai commented Jul 6, 2020

well we could randomize in secp256k1::new if the rand feature is available.

this will be a change in direction though, because right now our only ever use of rand is this: https://docs.rs/secp256k1/0.17.2/secp256k1/struct.Secp256k1.html#method.generate_keypair
where the user provide the RNG, that way we don't need to decide which rng.

BUT because only a handful of people know the importance of randomizing the context in practice we can see a lot(most?) of our users not calling it just because they don't know/understand, so I agree that a better default is a great idea

@real-or-random
Copy link
Collaborator Author

BUT because only a handful of people know the importance of randomizing the context in practice we can see a lot(most?) of users not calling it just because they don't know/understand, so I agree that a better default is a great idea

Indeed, this is my main concern.

@real-or-random
Copy link
Collaborator Author

Taking a step back, I believe that even given the problems with C, upstream's API is not optimal here. A better API would be to ask the user to provide entropy already in _context_create, or pass NULL explicitly to opt out.

(This does not help us here but I wanted to share the thought.)

tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 25, 2022
If a context is created and not randomized then it is susceptible to
sidechannel attacks (quoting what I've read).

Currently it is easy for users to mis-use our API because they may not
know that `randomize()` should be called.

We can better assist users by making APIs that are hard to mis-use.

If the `rand` feature is enabled we can optimistically call `randomize`
when creating a context as we do for the global context.

Also, add documentation highlighting the benefit of using the `rand`
feature.

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 25, 2022
If a context is created and not randomized then it is susceptible to
sidechannel attacks (quoting what I've read).

Currently it is easy for users to mis-use our API because they may not
know that `randomize()` should be called.

We can better assist users by making APIs that are hard to mis-use.

If the `rand` feature is enabled we can optimistically call `randomize`
when creating a context as we do for the global context.

Also, add documentation highlighting the benefit of using the `rand`
feature.

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 25, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add an enum to the context constructor that forces the use to make a
decision on the additional side channel attack protection we provide.
This makes the API a little harder to use because users must now read
the docs but this cost offsets the likely-hood of users ignoring this
important feature.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 25, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add an enum to the context constructor that forces the use to make a
decision on the additional side channel attack protection we provide.
This makes the API a little harder to use because users must now read
the docs but this cost offsets the likely-hood of users ignoring this
important feature.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add an enum to the context constructor that forces the use to make a
decision on the additional side channel attack protection we provide.
This makes the API a little harder to use because users must now read
the docs but this cost offsets the likely-hood of users ignoring this
important feature.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add an enum to the context constructor that forces the use to make a
decision on the additional side channel attack protection we provide.
This makes the API a little harder to use because users must now read
the docs but this cost offsets the likely-hood of users ignoring this
important feature.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add an enum to the context constructor that forces the use to make a
decision on the additional side channel attack protection we provide.
This makes the API a little harder to use because users must now read
the docs but this cost offsets the likely-hood of users ignoring this
important feature.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add an enum to the context constructor that forces the use to make a
decision on the additional side channel attack protection we provide.
This makes the API a little harder to use because users must now read
the docs but this cost offsets the likely-hood of users ignoring this
important feature.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add functions that make explicit the randomization of the context during
construction.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add functions that make explicit the randomization of the context during
construction.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add functions that make explicit the randomization of the context during
construction.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add functions that make explicit the randomization of the context during
construction.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 26, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add functions that make explicit the randomization of the context during
construction.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
tcharding added a commit to tcharding/rust-secp256k1 that referenced this issue Jan 27, 2022
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.

We can better assist users by making APIs that are hard to mis-use.

Add functions that make explicit the randomization of the context during
construction.

This is quite an invasive change because every user of the secp256k1
library will have to update the context constructor call sites and read
what this enum does. Is this worth it?

Resolves: rust-bitcoin#225
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