lockedpool: avoid sensitive data in core files (FreeBSD) #18443

pull vasild wants to merge 1 commits into bitcoin:master from vasild:madvise changing 1 files +3 −1

vasild commented at 7:49 PM on March 26, 2020: member

This is a followup to 23991ee53 / #15600 to also use madvise(2) on FreeBSD to avoid sensitive data allocated with secure_allocator ending up in core files in addition to preventing it from going to the swap.

lockedpool: avoid sensitive data in core files (FreeBSD)

This is a followup to
23991ee53 / https://github.com/bitcoin/bitcoin/pull/15600
to also use madvise(2) on FreeBSD to avoid sensitive data allocated
with secure_allocator ending up in core files in addition to preventing
it from going to the swap.

f85203097f

in src/support/lockedpool.cpp:258 in f85203097f
```
 252 | @@ -253,8 +253,10 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
 253 |      }
 254 |      if (addr) {
 255 |          *lockingSuccess = mlock(addr, len) == 0;
 256 | -#ifdef MADV_DONTDUMP
 257 | +#if defined(MADV_DONTDUMP) // Linux
 258 |          madvise(addr, len, MADV_DONTDUMP);
 259 | +#elif defined(MADV_NOCORE) // FreeBSD
```
luke-jr commented at 7:56 PM on March 26, 2020:

I think it would be preferable to use a new #ifdef here, just in case there's ever a system with both defined, but only one supported at runtime...

sipa commented at 10:56 PM on March 26, 2020:

As there are currently no such systems, and the flags do exactly the same thing, it seems unlikely that that would ever be an issue.

laanwj commented at 7:10 AM on March 27, 2020:

even more if both defined this is "bizarro" territory and it might be better to fail the compile to reconsider what to do here, i don't think naively calling both in order (in what order?) is ever a good idea … (but to be clear: leaving it like this is fine with me)

vasild commented at 9:08 AM on March 27, 2020:

I think if there is a system that defines MADV_DONTDUMP, but it is not supported at runtime (!?) then that system is badly broken. Regardless if it also defines MADV_NOCORE that works.
luke-jr commented at 7:56 PM on March 26, 2020: member

Concept ACK
sipa commented at 10:56 PM on March 26, 2020: member

ACK f85203097f78d9daa1d35c4097a80beab31da2a4 if someone verifies this works as intended on *BSD.
laanwj commented at 8:42 AM on March 27, 2020: member

ACK f85203097f78d9daa1d35c4097a80beab31da2a4 I'd assume the MADV_ constants are guaranteed to be in the same header as mmap, sys/mman.h?

Edit: I've verified that the call to madvise is done on FreeBSD. I have not test whether it is effective.
vasild commented at 9:14 AM on March 27, 2020: member

I verified this works as intended on FreeBSD 12.

Yes, the MADV_ constants are in sys/mman.h: https://www.freebsd.org/cgi/man.cgi?query=madvise https://linux.die.net/man/2/madvise
laanwj commented at 10:18 AM on March 27, 2020: member

Yes, the MADV_ constants are in sys/mman.h:

Ok good to know—if this was not the case, it would be better to have an autotools check instead.
practicalswift commented at 12:20 PM on March 29, 2020: contributor

Code-review ACK f85203097f78d9daa1d35c4097a80beab31da2a4 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)
laanwj added the label Utils/log/libs on Apr 1, 2020
laanwj merged this on May 4, 2020
laanwj closed this on May 4, 2020
vasild deleted the branch on May 4, 2020
sidhujag referenced this in commit 5df88526d3 on May 4, 2020
zkbot referenced this in commit 5ef5d8d268 on Jul 31, 2020
zkbot referenced this in commit 7d94064616 on Sep 29, 2020
Fabcien referenced this in commit 72559fd656 on Jan 12, 2021
PastaPastaPasta referenced this in commit 615c39bd6d on Jun 27, 2021
PastaPastaPasta referenced this in commit 148371619e on Jun 28, 2021
PastaPastaPasta referenced this in commit 77b5e61dfd on Jun 29, 2021
PastaPastaPasta referenced this in commit a7457f9828 on Jul 1, 2021
PastaPastaPasta referenced this in commit 425a7c88fa on Jul 1, 2021
PastaPastaPasta referenced this in commit aaaffab9d0 on Jul 14, 2021
PastaPastaPasta referenced this in commit b566c464ed on Jul 15, 2021
furszy referenced this in commit 771c84b2d3 on Jan 3, 2022
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#15600 lockedpool: When possible, use madvise to avoid including sensitive information in core dumps