Skip to content
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

[CI test, dontmerge] PR #1579 with secp256k1_memclear using volatile_memset #1622

Closed

Conversation

theStack
Copy link
Contributor

As suggested in #1579 (comment).

real-or-random and others added 10 commits October 22, 2024 18:24
This code is not supposed to handle secret data.
We rely on memset() and an __asm__ memory barrier where it's available or
on SecureZeroMemory() on Windows. The fallback implementation uses a
volatile function pointer to memset which the compiler is not clever
enough to optimize.
There are two uses of the secp256k1_fe_clear() function that are now separated
into these two functions in order to reflect the intent:

1) initializing the memory prior to being used -> converted to fe_set_int( . , 0 )
2) zeroing the memory after being used such that no sensitive data remains. ->
    remains as fe_clear()

In the latter case, 'magnitude' and 'normalized' need to be overwritten when
VERIFY is enabled.

Co-Authored-By: isle2983 <isle2983@yahoo.com>
Co-Authored-By: isle2983 <isle2983@yahoo.com>
Co-Authored-By: Pieter Wuille <pieter.wuille@gmail.com>
All of the invocations of secp256k1_memclear() operate on stack
memory and happen after the function is done with the memory object.
This commit replaces existing memset() invocations and also adds
secp256k1_memclear() to code locations where clearing was missing;
there is no guarantee that this commit covers all code locations
where clearing is necessary.

Co-Authored-By: isle2983 <isle2983@yahoo.com>
This gives the caller more control about whether the state should
be cleaned (= should be considered secret). Moreover, it gives the
caller the possibility to clean a hash struct without finalizing it.
Quoting sipa (see bitcoin-core#1479 (comment)):
"When performing an EC multiplication A = aG for secret a, the resulting
 _affine_ coordinates of A are presumed to not leak information about a (ECDLP),
  but the same is not necessarily true for the Jacobian coordinates that come
  out of our multiplication algorithm."

For the ECDH point multiplication result, the result in Jacobi coordinates should be
cleared not only to avoid leaking the scalar, but even more so as it's a representation
of the resulting shared secret.
@real-or-random
Copy link
Contributor

Okay, awesome CI seems to like this. Let me close this PR, I think this is a sufficient test for this #if branch, even if further small changes will be made to #1579.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants