Prevent dead-store elimination when clearing secrets in examples #1212
pull Harshil-Jani wants to merge 1 commits into bitcoin-core:master from Harshil-Jani:no-optimization changing 5 files +42 −17-
Harshil-Jani commented at 8:38 am on February 17, 2023: contributorSigned-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
-
bitcoin-core deleted a comment on Feb 19, 2023
-
Harshil-Jani force-pushed on Feb 22, 2023
-
Harshil-Jani force-pushed on Feb 22, 2023
-
Harshil-Jani force-pushed on Feb 22, 2023
-
Harshil-Jani force-pushed on Feb 22, 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 toutil.h
orexamples_util.h
to better distinguish it from our ownutil.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 ofexplicit 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: contributorAwesome. 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: contributorACK 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>
Harshil-Jani force-pushed on Mar 2, 2023real-or-random approvedreal-or-random commented at 10:11 am on March 2, 2023: contributorutACK 5660c137552c657da5265691dea0fb10faae6a76sipa commented at 10:06 pm on March 2, 2023: contributorutACK 5660c137552c657da5265691dea0fb10faae6a76real-or-random merged this on Mar 2, 2023real-or-random closed this on Mar 2, 2023
jonasnick cross-referenced this on Mar 7, 2023 from issue Update Changelog by jonasnickdhruv referenced this in commit a5df79db12 on Mar 7, 2023dhruv referenced this in commit 77b510d84c on Mar 7, 2023sipa referenced this in commit 763079a3f1 on Mar 8, 2023div72 referenced this in commit 945b094575 on Mar 14, 2023vmta referenced this in commit e1120c94a1 on Jun 4, 2023vmta referenced this in commit 8f03457eed on Jul 1, 2023jonasnick 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
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC
More mirrored repositories can be found on mirror.b10c.me