Clear sensitive memory without getting optimized out (revival of #636)

theStack commented at 9:10 pm on August 6, 2024: contributor

This PR picks up #636 (which in turn picked up #448, so this is take number three) and is essentially a rebase on master.

Some changes to the original PR:

the clearing function now has the secp256k1_ prefix again, since the related helper _memczero got it as well (see PR #835 / commit e89278f211a526062745c391d48a7baf782b4b2b)
the original commit b17a7df8145a6a86d49c354c7e7b59a432ea5346 (“Make _set_fe_int( . , 0 ) set magnitude to 0”) is not needed anymore, since it was already applied in PR #943 (commit d49011f54c2b31807158bdf06364f331558cccc7)
clearing of stack memory with secp256k1_memclear is now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered)

So far I haven’t looked at any disassembly and possible performance implications yet (there were some concerns expressed in #636 (comment)), happy to go deeper there if this gets Concept ACKed.

The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. https://github.com/google/boringssl/blob/5af122c3dfc163b5d1859f1f450756e8e320a142/crypto/mem.c#L335 and https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/string.h#L348 / https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/compiler.h#L102

Fixes #185.

theStack force-pushed on Aug 6, 2024

real-or-random added the label side-channel on Aug 17, 2024

real-or-random commented at 2:09 pm on August 17, 2024: contributor

Concept ACK (obviously)

Thanks for reviving this, I never had the time/motivation to come back to this PR, but it’s important.

We should call SECP256K1_CHECKMEM_UNDEFINE (https://github.com/bitcoin-core/secp256k1/blob/b307614401790850b48fb3ba878247290857a975/src/checkmem.h#L27) in secp256k1_memclear after clearing the memory. Reading cleared memory would clearly be a bug.

theStack force-pushed on Aug 20, 2024

theStack commented at 10:23 am on August 20, 2024: contributor

We should call SECP256K1_CHECKMEM_UNDEFINE (

https://github.com/bitcoin-core/secp256k1/blob/b307614401790850b48fb3ba878247290857a975/src/checkmem.h#L27

) in secp256k1_memclear after clearing the memory. Reading cleared memory would clearly be a bug.

Thanks, added that, and rebased on master.

real-or-random commented at 12:10 pm on October 8, 2024: contributor

@theStack Can you rebase this on top of musig which has introduced a few more code locations that need clearing?

Personally, I’d love to have this in the next release.

theStack force-pushed on Oct 8, 2024

theStack commented at 10:51 pm on October 8, 2024: contributor

@theStack Can you rebase this on top of musig which has introduced a few more code locations that need clearing?

Sure, done. With only five lines changed in the musig module, this needed less effort than expected, hope I didn’t miss any instances (many of them are handled indirectly via the ..._{fe,scalar}_clear functions though).

 0$ git range-diff ac0e41...0b01d2
 15:  6fcbae9 ! 15:  02ee811 Use secp256k1_memclear() to clear stack memory instead of memset()
 2    @@ src/modules/ellswift/main_impl.h: int secp256k1_ellswift_xdh(const secp256k1_con
 3          secp256k1_scalar_clear(&s);
 4      
 5     
 6    + ## src/modules/musig/session_impl.h ##
 7    +@@ src/modules/musig/session_impl.h: static void secp256k1_nonce_function_musig(secp256k1_scalar *k, const unsigned c
 8    +         secp256k1_scalar_set_b32(&k[i], buf, NULL);
 9    + 
10    +         /* Attempt to erase secret data */
11    +-        memset(buf, 0, sizeof(buf));
12    +-        memset(&sha_tmp, 0, sizeof(sha_tmp));
13    ++        secp256k1_memclear(buf, sizeof(buf));
14    ++        secp256k1_memclear(&sha_tmp, sizeof(sha_tmp));
15    +     }
16    +-    memset(rand, 0, sizeof(rand));
17    +-    memset(&sha, 0, sizeof(sha));
18    ++    secp256k1_memclear(rand, sizeof(rand));
19    ++    secp256k1_memclear(&sha, sizeof(sha));
20    + }
21    + 
22    + int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, secp256k1_musig_pubnonce *pubnonce, const unsigned char *input_nonce, const unsigned char *seckey, const secp256k1_pubkey *pubkey, const unsigned char *msg32, const secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *extra_input32) {
23    +@@ src/modules/musig/session_impl.h: int secp256k1_musig_nonce_gen_counter(const secp256k1_context* ctx, secp256k1_mu
24    +     if (!secp256k1_musig_nonce_gen_internal(ctx, secnonce, pubnonce, buf, seckey, &pubkey, msg32, keyagg_cache, extra_input32)) {
25    +         return 0;
26    +     }
27    +-    memset(seckey, 0, sizeof(seckey));
28    ++    secp256k1_memclear(seckey, sizeof(seckey));
29    +     return 1;
30    + }
31    + 
32    +

real-or-random commented at 10:59 pm on October 8, 2024: contributor

This one comes to my mind, too: https://github.com/bitcoin-core/secp256k1/blob/a88aa9350633c2d2472bace5c290aa291c7f12c9/src/util.h#L253-L254

theStack force-pushed on Oct 8, 2024

theStack commented at 11:16 pm on October 8, 2024: contributor

This one comes to my mind, too:

https://github.com/bitcoin-core/secp256k1/blob/a88aa9350633c2d2472bace5c290aa291c7f12c9/src/util.h#L253-L254

Ah good catch, missed that (only grepped for memset calls). Tackled now as well.

in src/util.h:19 in 4c9542040e outdated

14 #include <stdlib.h>
15 #include <stdint.h>
16 #include <stdio.h>
17 #include <limits.h>
18+#if defined(_MSC_VER)
19+// For SecureZeroMemory

real-or-random commented at 12:05 pm on October 10, 2024:

nit: //-style comments not allowed in C89. Just for consistency, it appears that MSVC doesn’t care…

theStack commented at 4:15 pm on October 11, 2024:

Fixed.

in src/secp256k1.c:503 in 05a46b5920 outdated

499        secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
500    }
501    secp256k1_rfc6979_hmac_sha256_finalize(&rng);
502+
503+   secp256k1_memclear(keydata, sizeof(keydata));
504+   secp256k1_rfc6979_hmac_sha256_clear(&rng);

real-or-random commented at 12:23 pm on October 10, 2024:

We should add similar calls in other code locations where we derive secrets via hash functions, e.g., in the schnorrsig and musig modules (and ellswift? now sure). The previous PR predates even the schnorrsig module, see the commit message.

theStack commented at 4:25 pm on October 11, 2024:

We should add similar calls in other code locations where we derive secrets via hash functions, e.g., in the schnorrsig and musig modules (and ellswift? now sure).

As far as I could see by greping for _finalize, all places where we derive secrets via hashing had corresponding hash object clearing calls already (probably I did that already in the first version of this PR, though I honestly don’t remember). What I found though was that it was unnecessarily called in the schnorrsig challenge function, so I removed it again, as there are no secrets involved. For the musig module, the _memclear calls were replaced by _sha256_clear calls.

One place where I added a hash clearing call was in the public API function secp256k1_tagged_sha256. We don’t use that internally, but it seems to make sense to clear the hash object, considering that we don’t know if the user passes in secret data or not.

The previous PR predates even the schnorrsig module, see the commit message.

Right, removed that part of the commit message.

real-or-random commented at 12:29 pm on October 10, 2024: contributor

If you want motivated, you could look at git grep secp256k1_ge_set_gej.*(. @sipa’s recent comment in the MuSig PR really caught my attention. When I worked on the previous PR, I really didn’t consider the possibility that a gej could leak secret data.

I wonder if it makes sense to have the _finalize functions in the hash module clear the state automatically. And then have “unsafe” funtions like _finalize_keep if the callers knows that data is public or if the caller wants to reuse the midstate. It sounds like a neat idea, but I’m not really convinced because it special-cases the hash module somewhat: we’ll need to manually clear everything else including scalars, etc… So we have to be careful with this anyway when writing and reviewing code, and perhaps having yet another safety mechanism that works only for the hash module. (This could perhaps be a nice follow-up if we add the same mechanism to more modules, e.g., ge_set_gej could clear the gejs, unless you use a _keep variant.)

real-or-random added the label needs-changelog on Oct 10, 2024

theStack commented at 5:38 pm on October 10, 2024: contributor

If you want motivated, you could look at git grep secp256k1_ge_set_gej.*(. @sipa’s recent comment in the MuSig PR really caught my attention. When I worked on the previous PR, I really didn’t consider the possibility that a gej could leak secret data.

I found the following functions containing _gej instances resulting from point multiplication (secp256k1_ecmult_{gen,const}) with secret scalars:

ECDH module: secp256k1_ecdh (res, resulting from the ECDH point multiplication; I think this should be cleared not only to avoid leaking the scalar, but even more so as it’s a representation of the resulting shared secret)
MuSig2 module: secp256k1_musig_nonce_gen_internal (public nonce nonce_ptj in the for loop)
Schnorr module: secp256k1_schnorrsig_sign_internal (nonce commitment rj)
main library: secp256k1_ec_pubkey_create_helper (public point pj)

Taking care of those could be probably go into an own PR, as its trivial to fix and review and hence has a significantly higher chance to land in the next release than this one? (and having a version where the compiler still might optimize it out seems still much better than not doing it at all)

Interestingly, the ECDSA signing function does clear out the nonce commitment for both the jacobi and affine points (though the latter wouldn’t be needed according to #1479 (review)).

I wonder if it makes sense to have the _finalize functions in the hash module clear the state automatically. And then have “unsafe” funtions like _finalize_keep if the callers knows that data is public or if the caller wants to reuse the midstate. It sounds like a neat idea, but I’m not really convinced because it special-cases the hash module somewhat: we’ll need to manually clear everything else including scalars, etc… So we have to be careful with this anyway when writing and reviewing code, and perhaps having yet another safety mechanism that works only for the hash module. (This could perhaps be a nice follow-up if we add the same mechanism to more modules, e.g., ge_set_gej could clear the gejs, unless you use a _keep variant.)

Sounds like a good idea to me for a follow-up.

real-or-random commented at 8:55 pm on October 10, 2024: contributor

Taking care of those could be probably go into an own PR, as its trivial to fix and review and hence has a significantly higher chance to land in the next release than this one? (and having a version where the compiler still might optimize it out seems still much better than not doing it at all)

My guess is that what we do currently is useless on any modern compiler. I admit that I haven’t looked at the compiler output, but I’d rather spend the time on resolving the problem.

I don’t think that fixing these additional cases here will make it much more difficult to review the PR. And to be honest, while this is a great for defense in depth, we haven’t deeply cared about this so far. It’s not the end of the world if we need to wait a few more months. So I think it’s good to add these cases to this PR here.

theStack force-pushed on Oct 11, 2024

theStack commented at 4:29 pm on October 11, 2024: contributor

My guess is that what we do currently is useless on any modern compiler. I admit that I haven’t looked at the compiler output, but I’d rather spend the time on resolving the problem.

I don’t think that fixing these additional cases here will make it much more difficult to review the PR. And to be honest, while this is a great for defense in depth, we haven’t deeply cared about this so far. It’s not the end of the world if we need to wait a few more months. So I think it’s good to add these cases to this PR here.

Thanks for the feedback! Makes sense, added an extra commit at the end for the gej clearing after point multiplications, it’s only four lines of code anyway. Also went over the necessary hash clearing places and made a few small changes (see #1579 (review)).

sipa commented at 5:50 pm on October 21, 2024: contributor

utACK c9210784f7300ad8209094b98e865e96e9efe2e6. I have not reviewed this for exhaustiveness (as in, are there more places where clearing is useful/necessary), but the code changes look good.

in src/util.h:254 in 767ac09297 outdated

248+    __asm__ __volatile__("" : : "r"(ptr) : "memory");
249+#else
250+    void *(*volatile const volatile_memset)(void *, int, size_t) = memset;
251+    volatile_memset(ptr, 0, len);
252+#endif
253+    SECP256K1_CHECKMEM_UNDEFINE(ptr, len);

theStack commented at 0:56 am on October 22, 2024:

Hm, it seems that in production code, we usually don’t call any of the _CHECKMEM_{DEFINE,UNDEFINE} macros. There is one reachable code-path in secp256k1_declassify, but it would only hit if the context was created with the SECP256K1_CONTEXT_DECLASSIFY flag, where the API documentation explicitly says “Do not use”. Should that _CHECKMEM_UNDEFINE call here be conditional by a preprocessor #if(def) or something alike, so it only applies to tests and can’t slow down (and bloat) release builds?

I noticed that while looking at the disassembly of the .so file and wondering why there was so much extra code after the memory clearing, until I realized this must be valgrind’s VALGRIND_MAKE_MEM_UNDEFINED. I’m building now explicitly with -DSECP256K1_VALGRIND=OFF -DSECP256K1_BUILD_CTIME_TESTS=OFF to avoid that.

real-or-random commented at 11:06 am on October 22, 2024:

Hm, great point! The reason why secp256k1_declassify uses a run-time flag (instead of compile-time flag) is that we want to run the constant-time tests on the real binary.

I don’t know what the performance impact of these additional instructions is, but I doubt that having a compile-time flag is a concern in this case. That means that we could just wrap the SECP256K1_CHECKMEM_UNDEFINE in an #ifdef VERIFY. @sipa What do you think?

sipa commented at 2:00 pm on October 22, 2024:

I think the reasoning is that we want the release binaries to be as close as possible to what we actually test in the ctime test, but disable the effect at runtime using SECP256K1_CONTEXT_DECLASSIFY to avoid a performance impact.

It wouldn’t surprise me that that is overkill; the conditional to determine whether or not to declassify operations at runtime may have a higher cost than actually executing the nop instructions that declassification actually correspond to when not running instrumented by valgrind.

real-or-random commented at 2:49 pm on October 22, 2024:

Oh, sorry, I think we’re talking past each other. My suggestion, for which I’d like to have your opinion on, is to wrap SECP256K1_CHECKMEM_UNDEFINE in an #ifdef VERIFY block here in this particular case of clearing memory, i.e., inside secp256k1_memclear (and not in secp256k1_declassify. Do you think that’s a reasonable thing to do?

But regarding what you said:

I think the reasoning is that we want the release binaries to be as close as possible to what we actually test in the ctime test, but disable the effect at runtime using SECP256K1_CONTEXT_DECLASSIFY to avoid a performance impact.

Yes, I agree. Perhaps an additional reason is to avoid any shenanigans that the valgrind syscalls may have. They should be noops, but it’s certainly a bit safer not to run them at all.

It wouldn’t surprise me that that is overkill; the conditional to determine whether or not to declassify operations at runtime may have a higher cost than actually executing the nop instructions that declassification actually correspond to when not running instrumented by valgrind.

I can imagine that the overhead of the noops is negligible, but at least checking the conditional should be negligible as well because we use EXCEPT to help the compiler predict the branch: https://github.com/bitcoin-core/secp256k1/blob/68b55209f1ba3e6c0417789598f5f75649e9c14c/src/secp256k1.c#L236-L238

theStack commented at 4:15 pm on October 22, 2024:

What bothered me in general (independent on how lightweight the code generated by the valgrind macros might be) is that when users build with the default CMake settings, they could currently end up with different release binaries, depending on whether valgrind is installed or not. Unrelated to this PR, but maybe the SECP256K1_VALGRIND build setting should only be auto-detected in the “dev-mode” preset, and OFF by default? (I only looked at the CMake build, don’t know if the valgrind setting is also auto-detected on autotools).

real-or-random commented at 8:21 pm on October 22, 2024:

I see. That’s a valid point, which has been brought up before, see #813 (comment). (This thread is also very helpful to understand why we have run-time flag etc.) The conclusion was that the benefits outweigh the drawback that the build outputs depends on the availability of valgrind, but yeah, there’s no perfect solution here. And yes, it’s also auto-detected in autotools.

sipa commented at 4:30 pm on October 25, 2024:

I see, so the idea is to have

0...
1#ifndef VERIFY
2     SECP256K1_CHECKMEM_UNDEFINE(ptr, len);
3#endif
4}

in secp256k1_memclear()?

That would mean tests_noverify wouldn’t detect violations of use-after-clear, nor would applications linking against the production library when running in valgrind. The upside would be that the production library definitely does not have its behavior affected by being compiled with valgrind present.

real-or-random commented at 4:35 pm on October 25, 2024:

Indeed.

That would mean tests_noverify wouldn’t detect violations of use-after-clear

Fwiw, I had this downside in mind, and I don’t think it’s a big deal because we still have the normal tests.

nor would applications linking against the production library when running in valgrind.

Okay, I had not considered this one… I still think that’s acceptable, but yeah, it’s a good point…

sipa commented at 4:40 pm on October 25, 2024:

ACK. I guess it makes sense that if the primary goal is making sure no runtime impact for production code, this should be guarded by a compile-time check, as the runtime check isn’t available here (being just for ctime testing).

in src/util.h:242 in 767ac09297 outdated

237+     *
238+     * Quoting Adam Langley <agl@google.com> in commit ad1907fe73334d6c696c8539646c21b11178f20f
239+     * in BoringSSL (ISC License):
240+     *    As best as we can tell, this is sufficient to break any optimisations that
241+     *    might try to eliminate "superfluous" memsets.
242+     * This method used in memzero_explicit() the Linux kernel, too. Its advantage is that it is

real-or-random commented at 2:57 pm on October 22, 2024:

s/used/is used/

Apparently that had been fixed in Core: https://github.com/bitcoin/bitcoin/blob/28ce159bc327e6dfec34077ff2e379b23a95db65/src/support/cleanse.cpp#L24

theStack commented at 4:44 pm on October 22, 2024:

Thanks, fixed.

theStack commented at 3:13 pm on October 22, 2024: contributor

For a first check on whether this PR fulfills its promise, I looked at the generated assembler code of secp256k1_ec_seckey_verify, as this function is particularly short and should clear out the secret scalar at the end. The following commands were used to generate the disassembled output:

0$ cmake --build build && objdump --no-addresses --no-show-raw-insn --disassemble=secp256k1_ec_seckey_verify ./build/lib/libsecp256k1.so > seckey_verify_disasm.txt

running on Debian 12 stable (Bookworm) and gcc 12.2.0 on a x86-64 machine

 0--- master.txt  2024-10-22 16:09:38.985554751 +0200
 1+++ pr1579.txt  2024-10-22 16:12:37.324112394 +0200
 2@@ -11,31 +11,38 @@
 3 Disassembly of section .text:
 4 
 5 <secp256k1_ec_seckey_verify>:
 6-       sub    $0x38,%rsp
 7+       push   %rbx
 8+       sub    $0x30,%rsp
 9        test   %rsi,%rsi
10-       je     <secp256k1_ec_seckey_verify+0x48>
11+       je     <secp256k1_ec_seckey_verify+0x58>
12+       lea    0x10(%rsp),%rbx
13        lea    0xc(%rsp),%rdx
14-       lea    0x10(%rsp),%rdi
15+       pxor   %xmm0,%xmm0
16+       mov    %rbx,%rdi
17        call   <secp256k1_scalar_set_b32>
18        mov    0xc(%rsp),%ecx
19        mov    0x10(%rsp),%rax
20        or     0x18(%rsp),%rax
21        or     0x20(%rsp),%rax
22+       movaps %xmm0,0x10(%rsp)
23        or     0x28(%rsp),%rax
24+       movaps %xmm0,0x20(%rsp)
25        setne  %dl
26        xor    %eax,%eax
27        test   %ecx,%ecx
28        sete   %al
29-       add    $0x38,%rsp
30        and    %edx,%eax
31+       add    $0x30,%rsp
32+       pop    %rbx
33        ret
34        nopl   0x0(%rax)
35        mov    %rdi,%rax
36        mov    0xb0(%rdi),%rsi
37-       lea    0x63b9(%rip),%rdi        # <_fini+0x1196>
38+       lea    0x6199(%rip),%rdi        # <_fini+0xf46>
39        call   *0xa8(%rax)
40+       add    $0x30,%rsp
41        xor    %eax,%eax
42-       add    $0x38,%rsp
43+       pop    %rbx
44        ret

in src/util.h:234 in 767ac09297 outdated

226@@ -221,6 +227,32 @@ static SECP256K1_INLINE void secp256k1_memczero(void *s, size_t len, int flag) {
227     }
228 }
229 
230+/* Cleanses memory to prevent leaking sensitive info. Won't be optimized out. */
231+static SECP256K1_INLINE void secp256k1_memclear(void *ptr, size_t len) {
232+#if defined(_MSC_VER)
233+    /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */
234+    SecureZeroMemory(ptr, len);

real-or-random commented at 3:34 pm on October 22, 2024:

I looked at the same code in Core again, and found this PR, and commented there: https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429613882

theStack commented at 4:46 pm on October 22, 2024:

Interesting. I left it as-is for now, happy to adapt once there is clarity that the variant in Bitcoin Core is preferred or needed.

real-or-random commented at 2:08 pm on October 25, 2024:

Everything else being equal, keeping the code in sync with Core is nice, but as I said there, I have a slight preference to stick to the current #if defined(_MSC_VER) because we have no OS-specific macros anywhere else in the library (ignoring the examples). See also #1625.

in src/util.h:251 in c9210784f7 outdated

246+     */
247+    memset(ptr, 0, len);
248+    __asm__ __volatile__("" : : "r"(ptr) : "memory");
249+#else
250+    void *(*volatile const volatile_memset)(void *, int, size_t) = memset;
251+    volatile_memset(ptr, 0, len);

real-or-random commented at 3:50 pm on October 22, 2024:

I’ve tested this locally, but it would be nice to see a full CI run where this branch is forced (e.g., via #if 0 / #elif 0)

theStack commented at 4:45 pm on October 22, 2024:

Opened #1622 for that.

real-or-random commented at 3:51 pm on October 22, 2024: contributor

ACK mod my review comments

Don't clear secrets in pippenger implementation

This code is not supposed to handle secret data.

e7d384488e

theStack force-pushed on Oct 22, 2024

theStack commented at 4:46 pm on October 22, 2024: contributor

Fixed #1579 (review) and rebased on master for easier comparison of branches (since #1553, the output location of the .so file changed from ./build/src to ./build/lib; not being aware of that, I was unintentionally comparing the library file generated from master with itself for quite some time, wondering why the PR doesn’t change anything 🤦‍♂️ ).

theStack commented at 3:55 pm on October 25, 2024: contributor

Regarding the open question of whether or not to include the _CHECKMEM_UNDEFINE macro in secp256k1_memclear in non-verify binaries or not, I compared the valgrind-builds on the pr branch with the macro commented out vs. with the macro included. This is what the assembler diff looks like (created for the same function and using the same methodology as in #1579 (comment)):

 0--- pr1579_without_checkmem_undefine.txt	2024-10-25 17:34:50.639344958 +0200
 1+++ pr1579_with_checkmem_undefine.txt	2024-10-25 17:33:35.632626088 +0200
 2@@ -11,38 +11,59 @@
 3 Disassembly of section .text:
 4 
 5 <secp256k1_ec_seckey_verify>:
 6+	push   %rbp
 7 	push   %rbx
 8-	sub    $0x30,%rsp
 9+	sub    $0x68,%rsp
10 	test   %rsi,%rsi
11-	je     <secp256k1_ec_seckey_verify+0x58>
12+	je     <secp256k1_ec_seckey_verify+0xb8>
13+	lea    0x30(%rsp),%rbp
14 	lea    0x10(%rsp),%rbx
15-	lea    0xc(%rsp),%rdx
16 	pxor   %xmm0,%xmm0
17+	mov    %rbp,%rdx
18 	mov    %rbx,%rdi
19 	call   <secp256k1_scalar_set_b32>
20-	mov    0xc(%rsp),%ecx
21-	mov    0x10(%rsp),%rax
22-	or     0x18(%rsp),%rax
23-	or     0x20(%rsp),%rax
24+	mov    0x10(%rsp),%rdx
25+	or     0x18(%rsp),%rdx
26 	movaps %xmm0,0x10(%rsp)
27-	or     0x28(%rsp),%rax
28+	or     0x20(%rsp),%rdx
29+	or     0x28(%rsp),%rdx
30 	movaps %xmm0,0x20(%rsp)
31-	setne  %dl
32-	xor    %eax,%eax
33-	test   %ecx,%ecx
34-	sete   %al
35-	and    %edx,%eax
36-	add    $0x30,%rsp
37+	mov    0x30(%rsp),%edx
38+	setne  %al
39+	xor    %ecx,%ecx
40+	test   %edx,%edx
41+	sete   %cl
42+	and    %eax,%ecx
43+	movq   $0x4d430001,0x30(%rsp)
44+	xor    %edx,%edx
45+	mov    %rbp,%rax
46+	mov    %rbx,0x38(%rsp)
47+	movq   $0x20,0x40(%rsp)
48+	movq   $0x0,0x48(%rsp)
49+	movq   $0x0,0x50(%rsp)
50+	movq   $0x0,0x58(%rsp)
51+	rol    $0x3,%rdi
52+	rol    $0xd,%rdi
53+	rol    $0x3d,%rdi
54+	rol    $0x33,%rdi
55+	xchg   %rbx,%rbx
56+	mov    %rdx,0x8(%rsp)
57+	mov    0x8(%rsp),%rax
58+	add    $0x68,%rsp
59+	mov    %ecx,%eax
60 	pop    %rbx
61+	pop    %rbp
62 	ret
63-	nopl   0x0(%rax)
64+	nopl   0x0(%rax,%rax,1)
65 	mov    %rdi,%rax
66 	mov    0xb0(%rdi),%rsi
67-	lea    0x5d09(%rip),%rdi        # <_fini+0x766>
68+	lea    0x7b99(%rip),%rdi        # <_fini+0x14e6>
69 	call   *0xa8(%rax)
70-	add    $0x30,%rsp
71-	xor    %eax,%eax
72+	add    $0x68,%rsp
73+	xor    %ecx,%ecx
74+	mov    %ecx,%eax
75 	pop    %rbx
76+	pop    %rbp
77 	ret
78 
79 Disassembly of section .fini:

Seems like at least in this function, it causes at least 15 extra (no-op like) instructions to be emitted. Considering that previously, these valgrind placeholders were not reachable (unless explicitly specified in the context object) and these extra instructions are added at multiple places spread all over the code, it might be better to not include them in the release binaries? OTOH, “ctime-tests should run with the same binary as in the release” then doesn’t hold anymore. So really not sure what’s better 🤔

real-or-random commented at 4:28 pm on October 25, 2024: contributor

OTOH, “ctime-tests should run with the same binary as in the release” then doesn’t hold anymore.

I don’t think so. If we wrap the new _CHECKMEM_UNDEFINE within _memclear in #ifdef VERIFY, I don’t see how this affects the ctimetests at all. We’d still include the _CHECKMEM_UNDEFINE in declassify in the release builds. And the ctimetests executable will still link against the (release) binary of the library. (Note that the ctimetests test executable uses the public API, it’s really just an application that consumes the library by linking against it, as opposed to the other test binaries, which essentially #include all the library code.)

Add secp256k1_memclear() for clearing 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.

1c08126222

Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear()

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>

d79a6ccd43

Separate between clearing memory and setting to zero in tests

Co-Authored-By: isle2983 <isle2983@yahoo.com>
Co-Authored-By: Pieter Wuille <pieter.wuille@gmail.com>

e3497bbf00

Use secp256k1_memclear() to clear stack memory instead of memset()

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>

9bb368d146

Implement various _clear() functions with secp256k1_memclear() 97c57f42ba

Don't rely on memset to set signed integers to 0 99cc9fd6d0

Introduce separate _clear functions for hash module

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.

349e6ab916

Clear _gej instances after point multiplication to avoid potential leaks

Quoting sipa (see https://github.com/bitcoin-core/secp256k1/pull/1479#discussion_r1790079414):
"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.

765ef53335

theStack force-pushed on Oct 25, 2024

theStack commented at 4:57 pm on October 25, 2024: contributor

OTOH, “ctime-tests should run with the same binary as in the release” then doesn’t hold anymore.

I don’t think so. If we wrap the new _CHECKMEM_UNDEFINE within _memclear in #ifdef VERIFY, I don’t see how this affects the ctimetests at all. We’d still include the _CHECKMEM_UNDEFINE in declassify in the release builds. And the ctimetests executable will still link against the (release) binary of the library. (Note that the ctimetests test executable uses the public API, it’s really just an application that consumes the library by linking against it, as opposed to the other test binaries, which essentially #include all the library code.)

Oh right, for some reason I assumed that the ctimetests binary is also compiled with VERIFY defined, which is of course nonsensical. Added the “#ifdef VERIFY” block around _CHECKMEM_UNDEFINE now, as it looks we have consensus on doing that (https://github.com/bitcoin-core/secp256k1/pull/1579#discussion_r1817025668).

real-or-random approved

real-or-random commented at 4:38 pm on November 1, 2024: contributor

ACK 765ef53335a3e0fafdafe1e757f6fe0789f2797f

sipa commented at 3:15 pm on November 4, 2024: contributor

reACK 765ef53335a3e0fafdafe1e757f6fe0789f2797f

real-or-random merged this on Nov 4, 2024

real-or-random closed this on Nov 4, 2024

theStack deleted the branch on Nov 4, 2024

achow101 referenced this in commit 2d46a89386 on Nov 4, 2024

vmta referenced this in commit b5ae194ce5 on Nov 6, 2024

vmta referenced this in commit 8999068421 on Nov 6, 2024

Eunovo referenced this in commit 55a2f7a840 on Nov 12, 2024

Vin757 commented at 3:57 am on November 21, 2024: none

W

Clear sensitive memory without getting optimized out (revival of #636) #1579