Added Drop to SharedSecret and SecretKey

[elichai]

• I added the Drop trait to SecretKey and SharedSecret which calls clear(),
• I replaced the clear function with write_volatile to prevent the compiler from optimizing it out.

[sorpaas]

Why is Into<Scalar> removed?

[elichai]

Because this creates an error.
You can't move out the inside of a type that implements Drop

[sorpaas]

Maybe this will work:

let ret = self.0.clone();
ret

I understand that this may be a little bit more inefficient but it preserves the API, as someone might need it.

[elichai]

Hmm ok, although that will leak the scalar and then the Drop won't run on it

[elichai]

Done

[elichai]

BTW, one thing about this is that if you pass one of these things by value to somewhere they will be moved in memory and create another copy of the data that won't be removed.
the solution to this is either put the array in a Box or Pin or just always pass by reference.

e.g:
With box: https://play.rust-lang.org/?gist=fade1db77af5f902f8403a97c9341b44
without box: https://play.rust-lang.org/?gist=9425accba3793bbc777fa767e265b7e8

Edit:
There's no Box without std, I'm still looking for a different solution for my code too but for now it just means always passing by reference :/

[sorpaas]

@elichai Do you think this may be better suited for libraries using libsecp256k1? If you're under no_std, you probably run things in wasm or something, in which I presume whether clearing the memory or not doesn't matter. If you're not on no_std, then like you said, there's Pin and Box (which can easily create wrapper on SecretKey to make sure nothing is leaked).

FYI, on paritytech/parity-common#80 @cheme uses the clear_on_drop library to solve this issue.

[elichai]

Hey, You might be right and it is more suited for the users to put SharedSecret and SecretKey in Boxs.
and by looking at paritytech/parity-common#80 I couldn't find how he addressed the passing by value problem

[cheme]

@elichai I think @sorpaas was referring to the possibility to use clear_on_drop instead of write_volatile (I do not remember what was the pros and cons), I agree with you that it does not fix the passing by value issue, using 'pin' seems definitely like an interesting option.

[elichai]

@elichai I think @sorpaas was referring to the possibility to use clear_on_drop instead of write_volatile (I do not remember what was the pros and cons), I agree with you that it does not fix the passing by value issue, using 'pin' seems definitely like an interesting option.

oh yeah, I personally don't like clear_on_drop because it uses gymnastics to make the compiler not optimize this away instead of using the builitin functions,
so in my opinion this isn't future proof (as llvms optimizing properties can change over time)

About the Pin, it's nice because it's accessible from core and you don't need std, but I still haven't managed to make it work as I want and the docs around it are lacking

[elichai]

@sorpass so what do you think?

[sorpaas]

Sorry!