Clear sensitive memory without getting optimized out #636
Conversation
src/util.h
Outdated
| #if defined(_MSC_VER) | ||
| /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */ | ||
| SecureZeroMemory(ptr, n); | ||
| #elif defined(__GNUC__) |
There was a problem hiding this comment.
I couldn't figure out when GCC started to support assembly but it's some version <= 2.95... So if you have such an old compiler, I guess you're on your own.
There was a problem hiding this comment.
GCC 2.95 is likely the earliest we'd would ever encounter someone wanting to support. (Haiku OS uses it still, I believe :) )
| #include <stdlib.h> | ||
| #include <stdint.h> | ||
| #include <stdio.h> | ||
|
|
||
| #if defined(_MSC_VER) | ||
| // For SecureZeroMemory | ||
| #include <Windows.h> |
There was a problem hiding this comment.
I think that's okay but I'm not entirely sure if we want that. It's used in Bitcoin Core too.
There was a problem hiding this comment.
I would be that this should be version gated, I'll see if I can figure out where thats supported to.
|
I think using a sizeof on an argument is likely to introduce almost impossible to detect bugs when e.g. someone hands it the first element of an array (esp one passed from another function). It doesn't feel particularly typesafe to me. Is there a reason why you preferred this approach to having every subsystem (like field, scalar) define a clear function for its relevant type? |
|
No, I don't prefer that to be honest. It's just the approach taken in #448, and I was somewhat worried about it, too. I'll try to change it. |
|
I removed the macro from the PR. Also I removed I'm happy to take suggestions for additional code locations where memory should be cleared. |
src/util.h
Outdated
| * just not remove it entirely. See "Dead Store Elimination (Still) Considered Harmful" by | ||
| * Yang et al. (USENIX Security 2017) for more background. | ||
| */ | ||
| memset(ptr, 0, len); |
There was a problem hiding this comment.
Now that "clearing" is cleanly separated from "setting to zero", it's a good idea to replace 0 with a different value here for testing that the code does not rely on the fact that the "_clear" functions set to 0. We could actually always use a different value here, or use a different value in (some runs of) tests. What do people think about this?
There was a problem hiding this comment.
Did you try checking the binary that it's actually not optimized out?
There was a problem hiding this comment.
Not yet. But yes, we should do this with a few compilers.
There was a problem hiding this comment.
it's a good idea to replace 0 with a different value here for testing
I don't think it's a good idea to use a different value in testing. But I don't see a downside to just replace 0 with 42. This also has the (slight) advantage that it'll make it easier to detect now improper usage of *_clear when rebasing -zkp on top of this. -zkp uses *_clear for initializing in many places.
There was a problem hiding this comment.
I like the idea of using a non-zero constant byte value. Using 0 is more likely to result in silent failures when uninitialized memory is accidentally used.
| @@ -49,10 +49,11 @@ static int secp256k1_fe_normalizes_to_zero(secp256k1_fe *r); | |||
| * implementation may optionally normalize the input, but this should not be relied upon. */ | |||
| static int secp256k1_fe_normalizes_to_zero_var(secp256k1_fe *r); | |||
|
|
|||
| /** Set a field element equal to a small integer. Resulting field element is normalized. */ | |||
| /** Set a field element equal to a small integer. Resulting field element is normalized; it has | |||
| * magnitude 0 if a == 0, and magnitude 1 otherwise. */ | |||
| static void secp256k1_fe_set_int(secp256k1_fe *r, int a); | |||
There was a problem hiding this comment.
(I wondered why this accepts int. An unsigned type seems more appropriate. For type safety, we would even want to exclude too large values and use something as small as unsigned char. But I guess this is slower and just not worth the hassle)?
There was a problem hiding this comment.
Agree, though this seems like something for another patch.
|
What about clearing the memory of the temporary Field Elements in the GE addition? https://github.com/bitcoin-core/secp256k1/blob/master/src/group_impl.h#L419 (and I think most of this file) |
|
Thanks, collecting more: And I think there more in the field module... I guess I'll just go through the entire codebase. 🤷♂️ |
There was a problem hiding this comment.
I confirmed by looking at the disassembly that both the memory barrier and volatile function pointer method do work with gcc 9.1.0.
Looking at the disassembly also confirmed that the memory barrier method lets gcc optimize the memset and the volatile function pointer method lets gcc actually call memset.
I didn't look at Windows.
I changed the memset overwrite from 0x00 to 0x42 and added a function
void secp256k1_do_nothing(void) {
unsigned char foo[32];
secp256k1_mem_clear(foo, sizeof(foo));
}
After compiling the tests with -fno-stack-protector (for brevity) the output of objdump -d tests is:
000000000001ac30 <secp256k1_do_nothing>:
1ac30: 66 0f 6f 05 e8 2c 04 movdqa 0x42ce8(%rip),%xmm0 # 5d920 <secp256k1_ge_const_g+0x80>
1ac37: 00
1ac38: 48 8d 44 24 d8 lea -0x28(%rsp),%rax
1ac3d: 0f 29 44 24 d8 movaps %xmm0,-0x28(%rsp)
1ac42: 0f 29 44 24 e8 movaps %xmm0,-0x18(%rsp)
1ac47: c3 retq
1ac48: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
1ac4f: 00
This moves 16 bytes of 0x42 located at 5d920 (readelf -x .rodata tests) into xmm0 and then moves two times 16 bytes from xmm0 into the foo buffer.
With the volatile function pointer method the disassembly is as follows:
000000000001acd0 <secp256k1_do_nothing>:
1acd0: 48 83 ec 38 sub $0x38,%rsp
1acd4: 48 8b 05 ed a2 04 00 mov 0x4a2ed(%rip),%rax # 64fc8 <memset@GLIBC_2.2.5>
1acdb: ba 20 00 00 00 mov $0x20,%edx
1ace0: be 42 00 00 00 mov $0x42,%esi
1ace5: 48 8d 7c 24 10 lea 0x10(%rsp),%rdi
1acea: 48 89 44 24 08 mov %rax,0x8(%rsp)
1acef: 48 8b 44 24 08 mov 0x8(%rsp),%rax
1acf4: ff d0 callq *%rax
1acf6: 48 83 c4 38 add $0x38,%rsp
1acfa: c3 retq
1acfb: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
This calls memset with arguments rsp + 10, 0x42 and 32 as expected (see the calling convention).
|
Results are positive and very similar to the above with clang version 8.0.0. |
|
@jonasnick did you try to benchmark if this affects performance in a non negligible way? |
|
On my system the memory barrier and volatile function pointer method have no measurable performance difference. However, using one of the methods vs. using nothing in |
There was a problem hiding this comment.
approach ACK. Very cool, someone had to do it eventually.
Clearing no secrets in pippenger is fine, not sure why I added it.
Looks like fe_clear is replaced everywhere with set_int(0) and the right memsets are replaced with the corresponding _clear function
I noticed that a few field.h comments about magnitudes are wrong (independent of this PR).
For example a field element set to 0 with fe_set_int will have magnitude 0 but can be correctly compared with secp256k1_fe_equal.
Tests run fine under valgrind, also with endo and using the volatile function pointer method. I didn't test on Windows.
Going through the codebase to look for locations where a _clear is missing can happen in another PR.
|
Looks like memsetting to (I changed run_benchmark count argument from 10 to 500) |
|
@jonasnick That sounds like a deal breaker, if true. |
|
Note that's just signing time, not verification time. But I agree, let's stick to 0, that's just simpler. |
|
Concept ACK. There are a number of data types which end up being cleared using the generic |
real-or-random
force-pushed
the
cleanse
branch
2 times, most recently
from
7c525da
to
b808227
Compare
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 <[email protected]>
Co-Authored-By: isle2983 <[email protected]> Co-Authored-By: Pieter Wuille <[email protected]>
All of the invocations of secp256k1mem_clear() 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 <[email protected]>
This gives the caller more control about whether the state should be cleaned (= should be considered secret), which will be useful for example for Schnorr signature verification in the future. Moreover, it gives the caller the possibility to clean a hash struct without finalizing it.
|
I rebased this and implemented @sipa's suggestion for the hash API. Now the caller is responsible to call nit: Also renamed |
Indeed, so this is ready for review. For looking into more places in the future, in particular within some arithmetic function, I was wondering if "overdoing" this will introduce more stores to memory, which is not only slightly worse for security but will probably also affect performace. For example, the
https://www.gnu.org/software/libc/manual/html_node/Erasing-Sensitive-Data.html AFAIU and I've seen from looking at generated ASM this should not happen with our memory barrier approach but I'm not 100% sure and any insight will be helpful. Note to myself: Cleaning registers is out of the scope currently but today the cited USENIX paper reminded me of a simple but expensive way to do this: Just run the operation again. (For example, run another SHA256 transform on dummy midstate and dummy input.) This will reuse and hence overwrite the registers (modulo inlining etc). |
|
Apparently stores of 64-byte zero-over-zero can be faster than stores of other values, see https://twitter.com/BRIAN_____/status/1260913021116993536. This would be a reason not so overwrite with zeros. But I think we shouldn't have secrets that are all zeroes.This would be relevant for indistinguishability-based primitives with arbitrary inputs, e.g., encryption, but not for signing and key exchange. |
|
Conceptually I think it makes sense to have separate secure-erase wiping and zeroing of values. It needs a big redo by now though; are you still interesting in working on this? |
Yeah, I certainly want to come back to this, but I don't want to commit to a specific timeline. Marking as draft for now. |
…vival of #636) 765ef53 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner) 349e6ab Introduce separate _clear functions for hash module (Tim Ruffing) 99cc9fd Don't rely on memset to set signed integers to 0 (Tim Ruffing) 97c57f4 Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing) 9bb368d Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing) e3497bb Separate between clearing memory and setting to zero in tests (Tim Ruffing) d79a6cc Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing) 1c08126 Add secp256k1_memclear() for clearing secret data (Tim Ruffing) e7d3844 Don't clear secrets in pippenger implementation (Tim Ruffing) Pull request description: 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 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 #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. ACKs for top commit: sipa: reACK 765ef53 real-or-random: ACK 765ef53 Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959
This is a rebase of #448, including a few changes (mostly implementing what I described in #185). I haven't tested this on Windows and I actually haven't reviewed this in detail so far, but people can start to have a look.
Fixes #185 and closes #448.