[Crypto] KeyGen improvement #3788
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3788 +/- ##
===========================================
+ Coverage 0 53.33% +53.33%
===========================================
Files 0 819 +819
Lines 0 76757 +76757
===========================================
+ Hits 0 40935 +40935
- Misses 0 32511 +32511
- Partials 0 3311 +3311
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| func overwrite(data []byte) { | ||
| _, err := rand.Read(data) // checking err is enough | ||
| if err != nil { | ||
| // zero the buffer if randomizing failed | ||
| for i := 0; i < len(data); i++ { | ||
| data[i] = 0 | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
is there any benefit in overwriting with random data instead of just writing zeros in the first place?
| func overwrite(data []byte) { | |
| _, err := rand.Read(data) // checking err is enough | |
| if err != nil { | |
| // zero the buffer if randomizing failed | |
| for i := 0; i < len(data); i++ { | |
| data[i] = 0 | |
| } | |
| } | |
| } | |
| func overwrite(data []byte) { | |
| for i := range data { | |
| data[i] = 0 | |
| } | |
| } | |
BoringSSL simply memsets the memory on free:
void OPENSSL_cleanse(void *ptr, size_t len) {
#if defined(OPENSSL_WINDOWS)
SecureZeroMemory(ptr, len);
#else
OPENSSL_memset(ptr, 0, len);
#if !defined(OPENSSL_NO_ASM)
/* As best as we can tell, this is sufficient to break any optimisations that
might try to eliminate "superfluous" memsets. If there's an easy way to
detect memset_s, it would be better to use that. */
__asm__ __volatile__("" : : "r"(ptr) : "memory");
#endif
#endif // !OPENSSL_NO_ASM
}
(with an additional caveat of preventing compiler optimizations.)
There was a problem hiding this comment.
Good question. First, this mitigation (either randomizing or zeroing) can only be helpful against "static" memory dump attacks that happen after the computation has ended. However, a more powerful attacker could use "dynamic" memory dumps during the computation to read sensitive data. In that case, the final "static" dump can help identify the memory areas to look at. Zeroing the sensitive areas gives an obvious hint for such attacks.
Of course, an experimented attacker may avoid this mitigation by comparing multiple dynamic memory dumps and seeing the buffers evolution (especially that Golang initializes new allocated buffers with zeroes 😞 ). Randomizing instead of zeroing makes the attack slightly harder but not impossible. Randomizing is also a common practice in "grey box" cryptography implementations.
Another limitation of overwriting buffers after use (either by randomizing or zeroing) is that it assumes the sensitive buffer was not copied in other areas. Such copies could have been made to the stack or registers, and these need to be overwritten as well. Looking at the assembly level for a specific compiler/architecture can answer the question, but that's not possible in our case (lib is written without a specific target compiler/arch).
This mitigation is nice to have but I don't rely much on it, the library is still vulnerable to memory dumps and should only be run in safe environments.
Now back to the compiler optimizations: I am still wondering how to force the Go complier to not omit the call to overwrite(). Any thoughts about that?
There was a problem hiding this comment.
We may be overthinking it too much. Our goal is to do a due diligence so that the memory we free won't have secret material in it. As long as we can check that memset is not optimized out by compiler, we should be fine. (otherwise we down the rabbithole of software memory enclaves, mlock, PROT_NONE, and other "fun" system things)
For the long term solution, a runtime/secret is probably the right way of solving it: golang/go#21865
PS. As far as physical access goes, I think this is an uphill battle: w/o enabled IOMMU one has full memory access via the PCIe DMA.
There was a problem hiding this comment.
For the long term solution, a runtime/secret is probably the right way of solving it
Nice, yes I can wait for that to be available in Go. Thanks!
As far as physical access goes, I think this is an uphill battle.
Once our model includes physical access, our library is totally insecure, even without memory access (because of side channel leakages). In my comment above, I was only considering remote memory access.
My overall opinion:
- this mitigation makes the lib more secure only in a "weak" attack model. In other models it is not very relevant IMHO.
- It doesn't cost much to add it, so I decided to add a slightly stronger mitigation, that is commonly used in grey box crypto implementations (overthinking there is required and common).
- It is noticeable in security audits so it's good to add it for that purpose.
tarakby
requested review from
jordanschalm,
AlexHentschel,
peterargue,
zhangchiqing and
SaveTheRbtz
|
misc additional question regarding Lines 9 to 12 in 03ca889 I didn't find a struct commonHasher in the code base. Is this outdated? Or is it just a typo, and the comment should read "... embeds common hasher" ?
|
|
bors merge |
3788: [Crypto] KeyGen improvement r=tarakby a=tarakby So far, key generation implementation assumes the input seed has uniformly distributed entropy. This is only valid if function callers use the output of a secure RNG as a seed. This PR relaxes the strong seed assumption and implements further mechanism to extract the input entropy and use it in the rest of the process. A seed with enough entropy is now sufficient to generate keys. - update BLS key generation seed handling by implementing the[ IETF draft algorithm](https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-keygen) in section 2.3. - this improves compatibility to IETF draft recommendations. - improves handling "bad" seeds with non uniform entropy. - a test against BLST key gen is added for compatibility check. - update ECDSA key generation by using HKDF to extract entropy from input seed and expand it into key bytes. - improves handling "bad" seeds with non uniform entropy. - minor improvement by overwriting sensitive data in memory after computation. - consolidate minimum seed length as a module constant instead of an algorithm specific length (consequence of the algorithm update) Side change: - add SHA2-256 light computation function (outside of the existing interface). 3789: [Tools] Add bootstrap command to generate grpc TLS keys r=peterargue a=peterargue This new command allows generating Access API TLS keys Co-authored-by: Tarak Ben Youssef <tarak.benyoussef@dapperlabs.com> Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
|
Build failed (retrying...): |
3788: [Crypto] KeyGen improvement r=tarakby a=tarakby So far, key generation implementation assumes the input seed has uniformly distributed entropy. This is only valid if function callers use the output of a secure RNG as a seed. This PR relaxes the strong seed assumption and implements further mechanism to extract the input entropy and use it in the rest of the process. A seed with enough entropy is now sufficient to generate keys. - update BLS key generation seed handling by implementing the[ IETF draft algorithm](https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-keygen) in section 2.3. - this improves compatibility to IETF draft recommendations. - improves handling "bad" seeds with non uniform entropy. - a test against BLST key gen is added for compatibility check. - update ECDSA key generation by using HKDF to extract entropy from input seed and expand it into key bytes. - improves handling "bad" seeds with non uniform entropy. - minor improvement by overwriting sensitive data in memory after computation. - consolidate minimum seed length as a module constant instead of an algorithm specific length (consequence of the algorithm update) Side change: - add SHA2-256 light computation function (outside of the existing interface). Co-authored-by: Tarak Ben Youssef <tarak.benyoussef@dapperlabs.com> Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com>
|
Build failed: |
So far, key generation implementation assumes the input seed has uniformly distributed entropy. This is only valid if function callers use the output of a secure RNG as a seed.
This PR relaxes the strong seed assumption and implements further mechanism to extract the input entropy and use it in the rest of the process. A seed with enough entropy is now sufficient to generate keys.
Side change: