[wallet] Securely erase potentially sensitive keys/values #10308
Conversation
src/support/cleanse.h
Outdated
| @@ -8,6 +8,8 @@ | |||
|
|
|||
| #include <stdlib.h> | |||
|
|
|||
| // Securely erase a span of memory in a way guaranteed not to be | |||
There was a problem hiding this comment.
I don't believe anything in C++ can actually guarantee this.
There was a problem hiding this comment.
While it's true that there is nothing in the standard that guarantees this, there are numerous compiler-specific attributes for this, as well as libraries that guarantee this (like right now, memory_cleanse is just a wrapper around OPENSSL_cleanse)
My goal with adding the comment is to explain its purpose to other maintainers of the codebase, as it hopefully sees more use.
There was a problem hiding this comment.
On further research, I'm not sure OPENSSL_cleanse actually guarantees anything at all.
There was a problem hiding this comment.
On further research, I'm not sure OPENSSL_cleanse actually guarantees anything at all.
Yep.
There was a problem hiding this comment.
To be fair in this case since OPENSSL_cleanse() exists in an external shared object, we know it won't be optimized out. Should I back out this comment though?
There was a problem hiding this comment.
@tjps Nothing in C++ can guarantee that memory will be overwritten. The memset_s function from the C11 standard does require a guarantee, but gcc/g++ don't implement it I believe. So, writing in the source code that something has a guarantee may be misleading. Furthermore, the memory being overwritten is not the only thing we're after. The compiler is free to for example copy some of the memory to the stack first, where it could live longer.
However, if we restrict ourselves to supported systems and platforms, we know more at this point in time. We know what optimizations are implemented, and what compilers are capable of. It is reasonable that a call won't be optimized out if it's in another module, but that may not be the case if we'd switch to LTO builds, for example.
I would just write "Attempt to overwrite a span of memory" or something.
There was a problem hiding this comment.
Agreed, I reduced the verbiage to make it clear that it is a best-effort attempt.
Also, it is my understanding that LTO only affects the object files you are linking, so dynamically loaded .so's are still safe from compiler analysis. But I could be wrong, I've never actually come across a project of any significant size using LTO.
Comment updated.
src/wallet/db.h
Outdated
| memory_cleanse(datKey.get_data(), datKey.get_size()); | ||
| bool success = false; | ||
| if (datValue.get_data() != NULL) | ||
| { |
There was a problem hiding this comment.
Style nit: brace on the same line as the if.
There was a problem hiding this comment.
That's actually my preference as well, but I thought that was against the style guide for this repo. Can I assume brace on same line for code going forward?
| CDataStream ssValue((char*)datValue.get_data(), (char*)datValue.get_data() + datValue.get_size(), SER_DISK, CLIENT_VERSION); | ||
| ssValue >> value; | ||
| success = true; | ||
| } catch (const std::exception&) { |
There was a problem hiding this comment.
Silently ignored exceptions are one of the larger frustrations when troubleshooting C++ code. Checking more closely it seems this is ok: please add a comment e.g. /* success = false */ here to show what is happening.
|
LGTM, utACK, as we have the |
|
utACK 2ac544531d8fd50807f9a8d98b576e3a49a6402c |
6c914ac [wallet] Securely erase potentially sensitive keys/values (Thomas Snider) Tree-SHA512: 071d88c4093108d4e4eced35a6ffcebe3f499798194f5b1be661ffa5b78b5f55311667f6d2a72758d85290f61f958381ee95d380b9045ca18e9e1875f0e686c8
Github-Pull: bitcoin#10308 Rebased-From: 6c914ac
Github-Pull: bitcoin#10308 Rebased-From: 6c914ac
…ys/values 6c914ac [wallet] Securely erase potentially sensitive keys/values (Thomas Snider) Tree-SHA512: 071d88c4093108d4e4eced35a6ffcebe3f499798194f5b1be661ffa5b78b5f55311667f6d2a72758d85290f61f958381ee95d380b9045ca18e9e1875f0e686c8
…ys/values 6c914ac [wallet] Securely erase potentially sensitive keys/values (Thomas Snider) Tree-SHA512: 071d88c4093108d4e4eced35a6ffcebe3f499798194f5b1be661ffa5b78b5f55311667f6d2a72758d85290f61f958381ee95d380b9045ca18e9e1875f0e686c8
…ys/values 6c914ac [wallet] Securely erase potentially sensitive keys/values (Thomas Snider) Tree-SHA512: 071d88c4093108d4e4eced35a6ffcebe3f499798194f5b1be661ffa5b78b5f55311667f6d2a72758d85290f61f958381ee95d380b9045ca18e9e1875f0e686c8
Memory cleanse backports Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#10308 - bitcoin/bitcoin#11196 - bitcoin/bitcoin#11558 - Only the changes that did not conflict. - bitcoin/bitcoin#16158 Part of #145.
memory_cleanse backports Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#10308 - bitcoin/bitcoin#11196 - bitcoin/bitcoin#11558 - Only the changes that did not conflict. - bitcoin/bitcoin#16158 Part of #145.
d06009f [wallet] Securely erase potentially sensitive keys/values (Thomas Snider) Pull request description: Straightforward back port PR, coming from bitcoin#10308. ACKs for top commit: Fuzzbawls: utACK d06009f random-zebra: utACK d06009f and merging... Tree-SHA512: 85bde0e07b345398b935c47859c531821c188b5b9a67e239ef25aa9aebe495eeb3e39150db3f8f21ca94895005497093a7456ecc3c15324b28a963b5a37196b5
Doing an audit of "memset(.*0" usage I found a few locations that intended to clear potentially sensitive memory right before freeing it, but were not using the nice memory_cleanse() function in the codebase. I have not verified that the compiler was actually eliding those calls, but better to be safe than sorry. Also changed one path that potentially left a value unscrubbed if it threw while deserializing.