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

[theStack]

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 Don't use reserved identifiers memczero and benchmark_verify_t #835 / commit e89278f)
• the original commit b17a7df ("Make _set_fe_int( . , 0 ) set magnitude to 0") is not needed anymore, since it was already applied in PR VERIFY_CHECK precondition for secp256k1_fe_set_int. #943 (commit d49011f)
• 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.

[real-or-random]

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 (

secp256k1/src/checkmem.h

Line 27 in b307614

* - SECP256K1_CHECKMEM_UNDEFINE(p, len):

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

[theStack]

We should call SECP256K1_CHECKMEM_UNDEFINE (

secp256k1/src/checkmem.h

Line 27 in b307614

* - SECP256K1_CHECKMEM_UNDEFINE(p, len):

) 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]

@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]

@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).

Relevant excerpt of the range-diff (uncolored here)

$ git range-diff ac0e41...0b01d2
5:  6fcbae9 ! 15:  02ee811 Use secp256k1_memclear() to clear stack memory instead of memset()
    @@ src/modules/ellswift/main_impl.h: int secp256k1_ellswift_xdh(const secp256k1_con
          secp256k1_scalar_clear(&s);
      
     
    + ## src/modules/musig/session_impl.h ##
    +@@ src/modules/musig/session_impl.h: static void secp256k1_nonce_function_musig(secp256k1_scalar *k, const unsigned c
    +         secp256k1_scalar_set_b32(&k[i], buf, NULL);
    + 
    +         /* Attempt to erase secret data */
    +-        memset(buf, 0, sizeof(buf));
    +-        memset(&sha_tmp, 0, sizeof(sha_tmp));
    ++        secp256k1_memclear(buf, sizeof(buf));
    ++        secp256k1_memclear(&sha_tmp, sizeof(sha_tmp));
    +     }
    +-    memset(rand, 0, sizeof(rand));
    +-    memset(&sha, 0, sizeof(sha));
    ++    secp256k1_memclear(rand, sizeof(rand));
    ++    secp256k1_memclear(&sha, sizeof(sha));
    + }
    + 
    + 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) {
    +@@ src/modules/musig/session_impl.h: int secp256k1_musig_nonce_gen_counter(const secp256k1_context* ctx, secp256k1_mu
    +     if (!secp256k1_musig_nonce_gen_internal(ctx, secnonce, pubnonce, buf, seckey, &pubkey, msg32, keyagg_cache, extra_input32)) {
    +         return 0;
    +     }
    +-    memset(seckey, 0, sizeof(seckey));
    ++    secp256k1_memclear(seckey, sizeof(seckey));
    +     return 1;
    + }
    + 
    +

[real-or-random]

This one comes to my mind, too:

secp256k1/src/util.h

Lines 253 to 254 in a88aa93

	/* acc may contain secret values. Try to explicitly clear it. */
	acc = 0;

[theStack]

This one comes to my mind, too:

secp256k1/src/util.h

Lines 253 to 254 in a88aa93

	/* acc may contain secret values. Try to explicitly clear it. */
	acc = 0;

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

[real-or-random]

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]

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]

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.

[theStack]

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 (comment)).

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]

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]

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 (comment)).