[wallet] Securely erase potentially sensitive keys/values #10308

tjps commented at 7:39 PM on May 1, 2017: contributor

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.

in src/support/cleanse.h:11 in 2ac544531d outdated

   7 | @@ -8,6 +8,8 @@
   8 |  
   9 |  #include <stdlib.h>
  10 |  
  11 | +// Securely erase a span of memory in a way guaranteed not to be

sipa commented at 7:43 PM on May 1, 2017:

I don't believe anything in C++ can actually guarantee this.

tjps commented at 5:29 PM on May 2, 2017:

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.

tjps commented at 5:36 PM on May 2, 2017:

On further research, I'm not sure OPENSSL_cleanse actually guarantees anything at all.

gmaxwell commented at 1:21 AM on May 3, 2017:

On further research, I'm not sure OPENSSL_cleanse actually guarantees anything at all.

Yep.

tjps commented at 1:24 AM on May 3, 2017:

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?

sipa commented at 1:38 AM on May 3, 2017:

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

tjps commented at 6:33 PM on May 3, 2017:

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.

fanquake added the label Wallet on May 1, 2017

in src/wallet/db.h:186 in 2ac544531d outdated

 191 | -        } catch (const std::exception&) {
 192 | -            return false;
 193 | +        memory_cleanse(datKey.get_data(), datKey.get_size());
 194 | +        bool success = false;
 195 | +        if (datValue.get_data() != NULL)
 196 | +        {

sipa commented at 10:08 PM on May 1, 2017:

Style nit: brace on the same line as the if.

tjps commented at 5:32 PM on May 2, 2017:

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?

sipa commented at 10:34 PM on May 1, 2017: member

Concept ACK

in src/wallet/db.h:191 in 2ac544531d outdated

 197 | +            // Unserialize value
 198 | +            try {
 199 | +                CDataStream ssValue((char*)datValue.get_data(), (char*)datValue.get_data() + datValue.get_size(), SER_DISK, CLIENT_VERSION);
 200 | +                ssValue >> value;
 201 | +                success = true;
 202 | +            } catch (const std::exception&) {

laanwj commented at 1:03 PM on May 2, 2017:

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.

tjps commented at 5:33 PM on May 2, 2017:

Good idea, added.

laanwj commented at 1:07 PM on May 2, 2017: member

LGTM, utACK, as we have the memory_cleanse function anyway let's use it where it makes sense.

jonasschnelli commented at 1:11 PM on May 2, 2017: contributor

utACK 2ac544531d8fd50807f9a8d98b576e3a49a6402c

[wallet] Securely erase potentially sensitive keys/values 6c914ac176

laanwj merged this on May 11, 2017

laanwj closed this on May 11, 2017

laanwj referenced this in commit 94e52273f3 on May 11, 2017

tjps deleted the branch on May 13, 2017

luke-jr referenced this in commit 050f011d3f on Jun 3, 2017

luke-jr referenced this in commit 1ac71b473a on Jun 3, 2017

luke-jr referenced this in commit ac465de1cb on Jun 3, 2017

luke-jr referenced this in commit d83a6d6423 on Jun 5, 2017

luke-jr referenced this in commit 306be23183 on Jun 5, 2017

luke-jr referenced this in commit 28b8b8b603 on Jun 5, 2017

nomnombtc referenced this in commit 7dbbd89cd2 on Jul 17, 2017

codablock referenced this in commit f7174fb655 on Jan 26, 2018

andvgal referenced this in commit e84241dd39 on Jan 6, 2019

CryptoCentric referenced this in commit 164e26a01a on Feb 27, 2019

zkbot referenced this in commit 17324f085f on Apr 30, 2020

zkbot referenced this in commit 7a0fe273d8 on Apr 30, 2020

random-zebra referenced this in commit 6847d0d648 on Jul 28, 2020

barton2526 referenced this in commit 18d97912ec on Jul 14, 2021

DrahtBot locked this on Sep 8, 2021