Prevent dead-store elimination when clearing secrets in examples

Harshil-Jani commented at 8:38 am on February 17, 2023: contributor

Signed-off-by: Harshil Jani harshiljani2002@gmail.com

Harshil-Jani cross-referenced this on Feb 17, 2023 from issue Prevent optimization in algorithms by Harshil-Jani

Harshil-Jani marked this as a draft on Feb 17, 2023

real-or-random renamed this:
~~prevent optimization in algorithms~~
Prevent dead-store elimination when clearing secrets in examples
on Feb 17, 2023

real-or-random commented at 9:42 am on February 17, 2023: contributor

Thanks for the PR!

It’s probably a good idea to add this to the examples, but I think an approach that has a separate function like in https://github.com/bitcoin-core/secp256k1/pull/636/commits/494965880b47c3f2b752391b0c757614d6cb9f75 is cleaner. secure_erase sounds like a good function name, for example.

Harshil-Jani commented at 4:39 pm on February 17, 2023: contributor

It’s probably a good idea to add this to the examples, but I think an approach that has a separate function like in https://github.com/bitcoin-core/secp256k1/commit/494965880b47c3f2b752391b0c757614d6cb9f75 is cleaner. secure_erase sounds like a good function name, for example.

Thanks @real-or-random for this !! Will look at this soon.

Please help me a little. I want to get it in phantom wallet. One more thing I want to know is my address in my phantom wallet where can I get it?

Sorry ?? I guess, This would be a wrong place for the question.

bitcoin-core deleted a comment on Feb 19, 2023

Harshil-Jani force-pushed on Feb 22, 2023

Harshil-Jani marked this as ready for review on Feb 22, 2023

Harshil-Jani commented at 9:40 pm on February 22, 2023: contributor

@real-or-random Let me know your feedback on this. Also, I was wondering is there any header where we can club this ? or how about a seperate header for secure_erase instead of writing it for each file.

real-or-random commented at 9:38 am on February 27, 2023: contributor

Yes, looks better now. Now if we include such a function, I still feel we should recommend state-of-the art practices, which is what I implemented in https://github.com/bitcoin-core/secp256k1/commit/494965880b47c3f2b752391b0c757614d6cb9f75. Note that essentially the same code was added to Bitcoin Core, see https://github.com/bitcoin-core/secp256k1/commit/494965880b47c3f2b752391b0c757614d6cb9f75. Can you switch to that function body?

Also, I was wondering is there any header where we can club this ? or how about a seperate header for secure_erase instead of writing it for each file.

I think we could put it in random.h and rename this to util.h or examples_util.h to better distinguish it from our own util.h.

Harshil-Jani force-pushed on Feb 27, 2023

Harshil-Jani commented at 5:47 am on February 28, 2023: contributor

@real-or-random I tried renaming the file. But, I think that would need to change few other things from secp256k1.h which could be part of another patch.

real-or-random commented at 10:01 am on February 28, 2023: contributor

@real-or-random I tried renaming the file. But, I think that would need to change few other things from secp256k1.h which could be part of another patch.

Why do you think so? This should be unrelated?

Harshil-Jani commented at 7:08 am on March 1, 2023: contributor

@real-or-random I tried renaming the file. But, I think that would need to change few other things from secp256k1.h which could be part of another patch.

Why do you think so? This should be unrelated?

My bad. I just renamed it and SECP256K1_INLINE instantly throwed an error of explicit type is missing ('int' assumed) (And I thought how would renaming a header affect the same file altough I knew we need to change it’s name in other files as well)so I assumed we need to fix this from secp.h But on further inspection I got to know that after renaming the header wherever it was used, It can find the inline and things would fix up.

Harshil-Jani force-pushed on Mar 1, 2023

real-or-random commented at 12:08 pm on March 1, 2023: contributor

Awesome. Can you also squash the first three commits together?

Harshil-Jani force-pushed on Mar 1, 2023

Harshil-Jani commented at 7:58 pm on March 1, 2023: contributor

Awesome. Can you also squash the first three commits together?

Done !! Thanks for reviewing.

in examples/schnorr.c:145 in df4693e031 outdated

139@@ -140,9 +140,8 @@ int main(void) {
140      * example through "out of bounds" array access (see Heartbleed), Or the OS
141      * swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
142      *
143-     * TODO: Prevent these writes from being optimized out, as any good compiler
144+     * Here we are preventing these writes from being optimized out, as any good compiler
145      * will remove any writes that aren't used. */
146-    memset(seckey, 0, sizeof(seckey));
147-
148+    secure_erase(seckey,sizeof(seckey));

real-or-random commented at 9:48 am on March 2, 2023:

0    secure_erase(seckey, sizeof(seckey));

And the same for the other calls to secure_erase.

Harshil-Jani commented at 10:09 am on March 2, 2023:

That was really the last comment I have.

Oh that’s totally fine. Even I am sceptical about the coding standards for established projects :) Done. Thanks for suggesting!!

real-or-random commented at 9:48 am on March 2, 2023: contributor

ACK with the comment addressed

That was really the last comment I have.

prevent optimization in algorithms

Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>

Add secure_erase function to clear secrets

Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>

Update the function with good practices

Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>

Renaming random.h to examples_util.h

Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>

5660c13755

Harshil-Jani force-pushed on Mar 2, 2023

real-or-random approved

real-or-random commented at 10:11 am on March 2, 2023: contributor

utACK 5660c137552c657da5265691dea0fb10faae6a76

sipa commented at 10:06 pm on March 2, 2023: contributor

utACK 5660c137552c657da5265691dea0fb10faae6a76

real-or-random merged this on Mar 2, 2023

real-or-random closed this on Mar 2, 2023

jonasnick cross-referenced this on Mar 7, 2023 from issue Update Changelog by jonasnick

dhruv referenced this in commit a5df79db12 on Mar 7, 2023

dhruv referenced this in commit 77b510d84c on Mar 7, 2023

sipa referenced this in commit 763079a3f1 on Mar 8, 2023

div72 referenced this in commit 945b094575 on Mar 14, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 20, 2023 from issue Upstream PRs 1160, 1193, 1169, 1190, 1192, 1194, 1196, 1195, 1170, 1172, 1200, 1199, 1203, 1201, 1206, 1078, 1209, 979, 1212, 1218, 1217, 1221, 1222 by jonasnick

Prevent dead-store elimination when clearing secrets in examples #1212