-
Notifications
You must be signed in to change notification settings - Fork 276
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
Context randomization tracking issue #388
Comments
SGTM, so
ACK
Maybe lets see what upstream does on this before we move?
One general issue here is that users who build with no-std support will want to randomize regularly, but then if a downstream user turns on rand somehow, we'll end up randomizing twice. Maybe that's okay, but it feels quite wasteful. |
I think this is going to end up requiring Perhaps I'll push up a PR that does parts 1 and 2 since they are simple changes. Then we can nut out the rest along with the EDIT: First two parts done in #385 |
Also relevant is the original issue that brought this up: #225 |
That's not true. All contexts except the Note that this means that also the creation of "verification" contexts is expensive now. This is a regression introduced by the recent changes to make tables static (sorry!) but the actual problem is that we perform the entire (re-)randomization routine on context creation on constant input data (because
There's the idea to add a convenience function that randomizes on creation: bitcoin-core/secp256k1#780. If you're interested, it would be nice to PR to upstream and then we can call it here. (I admit that this is strictly more work that simply fixing it here.) I somehow feel upstream can't provide the necessary bandwidth currently to match the bandwidth of changes here. edit: I saw this issue very late because I unsubscribed from the repo. Please just @mention me when there are discussions relevant to upstream. I'm sure this is where I can help best. |
Fair, it is the case that it already doesn't work. The only thing they separate features accomplish today is lack of rand dependency, which it sounds like we'll keep. |
Is this what the re-randomization buys us? (quote from the readme of bitcoin-core/secp256k1) |
Thanks for your input @real-or-random! I'm happy to PR to upstream to try to clean this up -- though I'd like to hold off until bitcoin-core/secp256k1#1058 is in so that I won't have a bunch of rebasing work to do. Regarding upstream not keeping up with the pace of development here, I think the opposite is true :) but in any case I'm happy to propose changes upstream to simplify my life here. Specifically, I will contribute upstream to
Then here (in rust-secp) I'd like to add a sign-then-rerandomize function. After discussion on IRC I'm not sure that I'd be able to get this into upstream, since they already have too many variants of signing functions :) |
Mad, I'll step back from this a bit then. I had a look at secp256k1 and its a bit above me without spending a whole bunch of time to come up to speed (I don't write C++). |
Yes!
No worries, it's just C. Just kidding.
Sounds great. Anyway, sorry for interrupting, I certainly don't want to stop any efforts here. |
I didn't get what 'Just kidding' meant here so I had to check and it is C huh. Well now I do feel like a goose, yesterday the code looked like such gobledy-gook that I assumed it was C++, turns out I've just been staring at Rust for too long. C89 too if I'm not mistaken, again. |
@tcharding I think Tim's "just kidding" was about the implication that C would be simpler than C++, so no need to be worried about it. (This is true, in some senses, but that does not mean that C is a sane or friendly language!) |
C was my first true love, that's why I was so surprised that I mistook it for C++. Friendly languages are boring :) |
8339ca5 Add documentation guiding users towards randomization (Tobin Harding) cf1496b Add documentation about rand-std feature (Tobin Harding) 1693d51 Randomize context on creation (Tobin Harding) a0465ea Remove feature global-context-less-secure (Tobin Harding) Pull request description: 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](#388 (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. ACKs for top commit: apoelstra: ACK 8339ca5 Tree-SHA512: e74fe9a6eaf8ac40e4e06997602006eb8ca95216b5bc6dca3f5f96b5b4d3bf8610d851d8f1ef5c199ab7fbe85b34d162f2ee0073647f45105a486d20d8c0722a
Sounds really bad TBH. Would like to figure out a better way. One thing I don't understand: is rerandomizing the context on each signing truly required for security or is it just a way to procrastinate expensive rerandomization? If the latter does it matter that each context is rerandomized or one randomized context per application is fine? |
No. It helps blinding which improves resistance against certain side-channel attacks. So it's good to use and it does not hurt but signing will be secure without it.
I don't understand this sentence, sorry.
Well, blinding works best if you don't reuse it. That's why it's a good idea to rerandomize the blinding after signing operation. If you have multiple signing contexts and they use the same blinding, this also means you'll reuse the same blinding values. |
I meant is re-randomization required to achieve the desired resistance?
Your explanation at the bottom actually answers everything. Thinking about it
Does it mean it requires |
@Kixunil naively yes, we would need Having said this, the rerandomize-after-sign logic can be done without |
As I understand it non-std can still impl its own RNG and we could support setting it. I suspect re-randomizing in |
You mean, having a global resettable RNG? That feels kinda scary and overengineered to me. |
No, just optionally pass RNG when creating secp context. |
This wouldn't be much easier than just calling |
We could use it to force |
Yeah, it's not really that important. We should encourage it though -- TBH I have never used I think all we should do is add a doccomment on |
If |
Yeah. That's what I'd like. But we should open a PR and have a separate discussion about that specific API change. |
Another related issue: #346 |
|
I'm trying to figure out if it's a good idea to rely on signing/pubkey-deriving APIs to be available after this change. I can not imagine how they can be available on Am I missing something? |
Sorry I can't follow this sentence. Which change are you referring to? The change proposed by Andrew in the initial comment here? And for whom is it not a good idea to rely on who making APIs available? |
Oh, while writing the reply I noticed I previously missed As an example, this library currently provides function These things can not happen all at the same time:
Because:
I was asking whether removing the function (at least from Side note: I prefer thread local to mutexes on |
Ok yes, this makes sense. So my thinking is:
|
Consolidating discussion from #385 #386 #387. Tobin is proposing to re-randomize contexts, not just the global one, when they are created -- though naturally only if the
rand
feature is enabled. Our current situation is kinda weird -- for users of global contexts, we require they "opt out" of rerandomization by choosing theglobal-context-less-secure
feature vs theglobal-context
flag, while ordinary users must "opt in" by creating a context then explicitly callingrandomize()
on it.A bit of context (hah):
Here is my proposal:
global-context-less-secure
feature;global-context
enables the global context, and we rerandomize it on first use ifrand
is also enabled.rand
is enabledsign()
andsign_schnorr
to rerandomize the context after each signing operation. We add asign_no_rerandomize
method to opt out of this&mut
pointer to the context object, which really sucks.rerandomize()
methods in place but document that users have basically no reason to call them manually.Alternate proposals:
sign
methods alone but addsign_randomize
ones which also do the re-randomizationContext
objects so that re-randomization can be done with normal non-mutable references. (May be OK with "fast re-randomization", almost certainly not with full re-randomization, which would basically cause parallel signing operations to be force-serialized.)cc @TheBlueMatt @tcharding
See also bitcoin-core/secp256k1#881 (which is not the "one-bit rerandomization" idea, but is similar and has a similar effect).
See also bitcoin-core/secp256k1#1058 which will affect upstream's blinding logic.
The text was updated successfully, but these errors were encountered: