-
Notifications
You must be signed in to change notification settings - Fork 110
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
Fix: zeroize Point and BigInt on drop, and other zeroize changes #154
Conversation
Is everything okay with your Travis CI? 🙃 |
@Sky-Nik we ran out of credits on Travis CI 🙂 I'll run tests locally |
(suggested by @survvived and @elichai)
Note: actual contents of gmp bigint are zeroized on drop within gmp, so it does not make sense to zeroize them by hand.
Note: I had to tweak the implementation of zeroize a bit once again, to get past borrow checker. I believe it still does the same thing, and it still passes my tests, but feel free to double-check.
Potentially closes ZenGo-X/multi-party-ecdsa#148 and #150. |
@survived one more thing: earlier we were concerned with Vec reallocations, but then we reviewed the code and realized that the current version of curv is mostly iterator-based. Dima and I are not huge Rust experts, so we are not sure if such iterators leave intermediate values in memory or if they construct the resulting object in place. Could you please clarify this? |
What do you mean? Could you give an example?
Actually, I'm not sure whether we should address reallocation concern in num-bigint backend. It slows down the performance which might be not desirable. Maybe we should address reallocations zeroizing in another backend? |
One random example might be |
I don't think that in this particular case vector gets reallocated. Since it's constructed from Iterator, it can use Though relocation might happen somewhere else in the library. The entire library needs to be carefully reviewed in order to make sure that it doesn't happen. |
Moreover, (but I might be mistaken) every time we pass Scalar by value as an argument to some function, it gets copied and old version persist on stack without zeroing out. I mean: let scalar = Scalar::random();
some_function(scalar); Here scalar is supposed to be moved to let scalar = Scalar::random();
some_function(scalar.clone()); In this case, scalar is not moved and is dropped properly. So there are a lot of things we need to keep in mind in order to design zeroing properly. |
|
Note: as pointed out by @survived, passing Scalars by value will create an extra copy on the stack. Several methods and functions were updated to take a reference instead.
As for scalars getting implicitly copied - in some functions, they are passed by reference and I think it is, if possible, the way to go, right? Any code that calls such function won't be able to accidentally create a copy. If scalars are passed by value, all callers should carefully clone scalars and so we transfer our problems onto the client code.
|
@Sky-Nik,
Indeed, we didn't take into account that aspect while were designing the library. Good opportunity to improve it. I don't think that bunch of
Yes, accepting secret by reference is the best way to avoid accident moving. (secrets like
My intuition says that this code leaves a copy on the stack (unless compiler optimizes out extra stack variables; but we always can write more complex function that cannot be optimized): fn keypair() -> Keypair {
let sk = Scalar::random();
let pk = Point::generator() * &sk;
Keypair{ sk, pk }
} And it needs to be rewritten: fn keypair() -> Keypair {
let sk = Scalar::random();
let pk = Point::generator() * &sk;
Keypair{ sk: sl.clone(), pk }
} (but clippy will be complaining that
I believe that artefacts on the stack is not a really bad thing, as they much likely are going to be rewritten by next function call. But we ideally should minimize these leaks. I feel like it's more important to vanish secrets on heap, but it's doubtable |
@survived Your code still returns |
Note: as pointed out by @DmytroTym and @survived, moving Scalars into structs will create an extra copy on the stack. Several struct constructions were updated to clone instead.
@survived I added two commits that try to pass Scalars by reference and clone them when constructing structs, but I must say that I agree with elichai. It sounds extremely difficult to wipe out the stack completely, as values are copied around all the time, including the underlying gmp implementation. The performance regression I was talking about earlier actually refers to the "Box everything" solution suggested by elichai. |
Doesn't it writes directly to caller stack? It writes to own stack first and then just copies the value to caller stack? Anyway, zeroing stack values very tricky, right |
It seems so :( |
Can't say anything about that crate, I haven't used it. But looks like it erases stack, yeah, but you need to choose accurate amount of pages to be cleared. |
@survived would it be OK if we:
|
@survived to clarify:
|
Impl Zeroize for Point<E>
;Zeroizing<Point<E>>
for points that need to be zeroized;Zeroize
for BigInts.