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

luke-jr · 2019-03-14T13:40:44Z

If we're mlocking something, it's because it's sensitive information. Therefore, don't include it in core dump files, ~~and unmap it from forked processes~~.

The return value is not checked because the madvise calls might fail on older kernels as a rule (unsure).

luke-jr · 2019-05-05T04:46:44Z

No apparent reviewer interest, and it seems to break -daemon with no obvious solution.

…nformation in core dumps

luke-jr · 2020-03-04T20:16:58Z

Reopened due to renewed interest, and removed the DONTFORK part that was broken

practicalswift · 2020-03-04T23:07:26Z

Concept ACK

DrahtBot · 2020-03-05T14:05:57Z

Gitian builds

File	commit `a71c347` (master)	commit `2d32037` (master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz	`263dd4df65588db4...`	`87a836ebe2640c24...`
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz	`3f779204013ffde0...`	`b25121535f4b3e4e...`
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz	`f6cc7d17fdf2b4f0...`	`9bf22b2ea053fe9f...`
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz	`b08643b04f8d69c0...`	`0811dc229d30b135...`
bitcoin-0.19.99-osx-unsigned.dmg	`82112ea8d8b8dfb7...`	`3f15eeb9c8995188...`
bitcoin-0.19.99-osx64.tar.gz	`e7408d167771af4e...`	`452145dfff313f76...`
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz	`af924ded182d7648...`	`98cdc0cf6d4dcb61...`
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz	`d6a4f981ad9fa31f...`	`f71881fa68838b30...`
bitcoin-0.19.99-win64-debug.zip	`b0796bf49b421fa0...`	`8b6b299372408b9b...`
bitcoin-0.19.99-win64-setup-unsigned.exe	`3a473d863012b1fe...`	`340157f6bb540f2b...`
bitcoin-0.19.99-win64.zip	`f658d68b14d640c3...`	`5a0e6165b71cc67f...`
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz	`dbab3fa3cc95a255...`	`554ef07154623a5d...`
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz	`fa419172b1f67f66...`	`de1b54355262591a...`
bitcoin-0.19.99.tar.gz	`38d9e79747ecfb3b...`	`558c333681cb5459...`
bitcoin-core-linux-0.20-res.yml	`0bb8e45015337601...`	`4e8a46fb1c0d3f43...`
bitcoin-core-osx-0.20-res.yml	`587b85f6de99b01c...`	`14718c9a1dc15f0c...`
bitcoin-core-win-0.20-res.yml	`4cb36ab35ba216ed...`	`903f3ad4472eea5e...`
linux-build.log	`17b4963debeaff4c...`	`cb1bc7e73f69d362...`
osx-build.log	`de1d4010171bee32...`	`30eed6610c634837...`
win-build.log	`a7428da768940c64...`	`6a23faa140bab3c4...`
bitcoin-core-linux-0.20-res.yml.diff		`88743d508c492414...`
bitcoin-core-osx-0.20-res.yml.diff		`a6e23042337e3f57...`
bitcoin-core-win-0.20-res.yml.diff		`c308f24f99ffc760...`
linux-build.log.diff		`50481d877cfcb295...`
osx-build.log.diff		`1125ea1d898b8949...`
win-build.log.diff		`7e982717b61ae018...`

laanwj · 2020-03-05T19:16:22Z

Concept ACK

practicalswift · 2020-03-05T20:12:20Z

Code review ACK d831831 -- patch looks correct

Please note that I have not verified that the desired end result is achieved.

That should be done before merge :)

jonatack

ACK d831831

Code review and rebased on master/built/tests/ran bitcoind. The concept appears correct according to man core

  There are various circumstances in which a core dump file is not produced:

  ... / ...

  In addition, a core dump may exclude part of the address space of the
  process if the madvise(2) MADV_DONTDUMP flag was employed.

and implementation correct as per man madvise

  MADV_DONTDUMP (since Linux 3.4)
    Exclude from a core dump those pages in the range specified by
    addr and length.  This is useful in applications that have
    large areas of memory that are known not to be useful in a
    core dump.  The effect of MADV_DONTDUMP takes precedence over
    the bit mask that is set via the /proc/[pid]/coredump_filter
    file (see core(5)).

  ... / ...

  RETURN VALUE
    On success, madvise() returns zero. 
    On error, it returns -1 and errno is set appropriately.

EDIT: tested as described in #15600 (comment) below

jonatack · 2020-03-25T15:30:10Z

src/support/lockedpool.cpp

@@ -250,6 +250,9 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
    addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
    if (addr) {
        *lockingSuccess = mlock(addr, len) == 0;
+#ifdef MADV_DONTDUMP
+        madvise(addr, len, MADV_DONTDUMP);


should the return value be used to signal success/failure to the user?

laanwj · 2020-03-25T15:35:23Z

I checked the manual page for madvise and I think the usage is correct.

Not checking the return value is okay in this case because if it fails, there's no problem. The function is not marked as "warn_unused_result" so this won't result in compiler warnings either.

/usr/include/x86_64-linux-gnu/sys/mman.h:

extern int madvise (void *__addr, size_t __len, int __advice) __THROW;

ACK d831831

jonatack · 2020-03-25T15:38:30Z

Agree.

/usr/include/x86_64-linux-gnu/sys/mman.h:

#ifdef __USE_MISC
/* Advise the system about particular usage patterns the program follows
   for the region starting at ADDR and extending LEN bytes.  */
extern int madvise (void *__addr, size_t __len, int __advice) __THROW;
#endif

vasild

ACK d831831

But notice support for FreeBSD is easy to hook, so I would suggest to add it.

I tested this on FreeBSD by writing some specific string to the allocated memory and calling abort();. Then ./src/bitcoind crashes immediately. It works as expected: without madvise() the string is present in the core file. With madvise() the string is absent.

I also verified that the effects of the new call need not be explicitly undone when freeing up the memory. For example we undo the mlock() call in FreeLocked() by doing munlock(). I was worried that if we don't do madvise(MADV_DODUMP) when we free the page to the OS, then maybe later, if we allocate it again then MADV_DONTDUMP would be in effect without us asking for it. But this is not the case.

For reference here are the changes I did to test it:

basic test

--- i/src/support/lockedpool.cpp
+++ w/src/support/lockedpool.cpp
@@ -250,15 +250,23 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
     addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
     if (addr == MAP_FAILED) {
         return nullptr;
     }
     if (addr) {
         *lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
+#if 1
+#if defined(MADV_DONTDUMP) // Linux
         madvise(addr, len, MADV_DONTDUMP);
+#elif defined(MADV_NOCORE) // FreeBSD
+        madvise(addr, len, MADV_NOCORE);
 #endif
+#endif
+        memset(addr, 'A', 16);
+        memset((char*)addr + 16, 'B', 16);
+        memset((char*)addr + 32, 'C', 16);
+        abort();
     }
     return addr;
 }
 void PosixLockedPageAllocator::FreeLocked(void* addr, size_t len)
 {
     len = align_up(len, page_size);

test that undo is not necessary

--- i/src/support/lockedpool.cpp
+++ w/src/support/lockedpool.cpp
@@ -250,15 +250,36 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
     addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
     if (addr == MAP_FAILED) {
         return nullptr;
     }
     if (addr) {
         *lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
+#if 1
+#if defined(MADV_DONTDUMP) // Linux
         madvise(addr, len, MADV_DONTDUMP);
+#elif defined(MADV_NOCORE) // FreeBSD
+        madvise(addr, len, MADV_NOCORE);
 #endif
+#endif
+        FreeLocked(addr, len);
+
+        // Allocate again to confirm that the effect of MADV_DONTDUMP/MADV_NOCORE
+        // has vanished. The string must be in the core file.
+        void* new_addr = mmap(addr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+
+        if (new_addr == MAP_FAILED) {
+            perror("mmap");
+            exit(1);
+        }
+
+        assert(new_addr == addr);
+
+        memset(addr, 'A', 16);
+        memset((char*)addr + 16, 'B', 16);
+        memset((char*)addr + 32, 'C', 16);
+        abort();
     }
     return addr;
 }
 void PosixLockedPageAllocator::FreeLocked(void* addr, size_t len)
 {
     len = align_up(len, page_size);

vasild · 2020-03-25T15:08:55Z

src/support/lockedpool.cpp

+#ifdef MADV_DONTDUMP
+        madvise(addr, len, MADV_DONTDUMP);
+#endif


FreeBSD has the same functionality under a different name MADV_NOCORE:

Suggested change

#ifdef MADV_DONTDUMP

madvise(addr, len, MADV_DONTDUMP);

#endif

#if defined(MADV_DONTDUMP) // Linux

madvise(addr, len, MADV_DONTDUMP);

#elif defined(MADV_NOCORE) // FreeBSD

madvise(addr, len, MADV_NOCORE);

#endif

jonatack · 2020-03-25T23:33:27Z

@vasild thanks for the interesting tests! Good idea. Here are similar steps I took to reproduce them:

test that without madvise string is present in core dump

(pr/15600)$ uname -a
Linux 4.19.0-5-amd64 #1 SMP Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux
(pr/15600)$ ulimit -c unlimited && git diff && make

--- a/src/support/lockedpool.cpp
+++ b/src/support/lockedpool.cpp
@@ -23,6 +23,7 @@
 #include <algorithm>
+#include <cstring>
 #ifdef ARENA_DEBUG
@@ -253,9 +254,14 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
     if (addr) {
         *lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
-        madvise(addr, len, MADV_DONTDUMP);
-#endif
+        memset(addr, 'A', 16);
+        memset((char*)addr + 16, 'B', 16);
+        memset((char*)addr + 32, 'C', 16);
+        printf("\n");
+        printf("Process is aborting\n");
+        abort();
+        printf("Control not reaching here\n");
+        return 0;
     }
     return addr;
 }

(pr/15600)$ bitcoind

Process is aborting
Aborted (core dumped)
(pr/15600)$ grep AAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBCCCCCCCCCCCCCCCC core
Binary file core matches
(pr/15600)$

test that with madvise string is absent from core dump

(pr/15600)$ uname -a
Linux 4.19.0-5-amd64 #1 SMP Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux
(pr/15600)$ ulimit -c unlimited && git diff && make

--- a/src/support/lockedpool.cpp
+++ b/src/support/lockedpool.cpp
@@ -23,6 +23,7 @@
 #include <algorithm>
+#include <cstring>
 #ifdef ARENA_DEBUG
 #include <iomanip>
 #include <iostream>
@@ -253,9 +254,21 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
     if (addr) {
         *lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
+#if defined(MADV_DONTDUMP) // Linux
         madvise(addr, len, MADV_DONTDUMP);
+#elif defined(MADV_NOCORE) // FreeBSD
+        madvise(addr, len, MADV_NOCORE);
 #endif
+        memset(addr, 'A', 16);
+        memset((char*)addr + 16, 'B', 16);
+        memset((char*)addr + 32, 'C', 16);
+        printf("\n");
+        printf("Process is aborting\n");
+        abort();
+        printf("Control not reaching here\n");
+        return 0;
     }
     return addr;
 }

(pr/15600)$ bitcoind

Process is aborting
Aborted (core dumped)
(pr/15600)$ grep AAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBCCCCCCCCCCCCCCCC core
(pr/15600)$

laanwj · 2020-03-26T15:55:20Z

I'm going ahead to merge this. Let's add FreeBSD support in a new PR.

This is a followup to 23991ee / bitcoin#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.

…including sensitive information in core dumps d831831 lockedpool: When possible, use madvise to avoid including sensitive information in core dumps (Luke Dashjr) Pull request description: If we're mlocking something, it's because it's sensitive information. Therefore, don't include it in core dump files, ~~and unmap it from forked processes~~. The return value is not checked because the madvise calls might fail on older kernels as a rule (unsure). ACKs for top commit: practicalswift: Code review ACK d831831 -- patch looks correct laanwj: ACK d831831 jonatack: ACK d831831 vasild: ACK d831831 Tree-SHA512: 9a6c1fef126a4bbee0698bfed5a01233460fbcc86380d984e80dfbdfbed3744fef74527a8e3439ea226167992cff9d3ffa8f2d4dbd5ae96ebe0c12f3eee0eb9e

f852030 lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov) Pull request description: This is a followup to 23991ee / #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. ACKs for top commit: sipa: ACK f852030 if someone verifies this works as intended on *BSD. laanwj: ACK f852030 practicalswift: Code-review ACK f852030 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :) Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a

…FreeBSD) f852030 lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov) Pull request description: This is a followup to 23991ee / bitcoin#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. ACKs for top commit: sipa: ACK f852030 if someone verifies this works as intended on *BSD. laanwj: ACK f852030 practicalswift: Code-review ACK f852030 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :) Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a

Locked memory manager Add a pool for locked memory chunks, replacing `LockedPageManager`. Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#8321 - bitcoin/bitcoin#8753 - bitcoin/bitcoin#9063 - bitcoin/bitcoin#9070 - bitcoin/bitcoin#11385 - bitcoin/bitcoin#12048 - Excludes change to benchmark. - bitcoin/bitcoin#15117 - bitcoin/bitcoin#16161 - Excludes Travis CI changes. - Includes change from bitcoin/bitcoin#13163 - bitcoin/bitcoin#15600 - bitcoin/bitcoin#18443 - Assorted small changes from: - bitcoin/bitcoin#9233 - bitcoin/bitcoin#10483 - bitcoin/bitcoin#10645 - bitcoin/bitcoin#10969 - bitcoin/bitcoin#11351 - bitcoin/bitcoin#19111 - Excludes change to `src/rpc/server.cpp` - bitcoin/bitcoin#9804 - Only the commit for `src/key.cpp` - bitcoin/bitcoin#9598

This is a followup to 23991ee53 / bitcoin/bitcoin#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.

…nformation in core dumps Summary: Issue bitcoin#16824 > On a crash, bitcoin-qt may dump a core file that contains what was in memory at the time of the crash, for debugging purposes. The problem here is that bitcoin-qt stores the user's wallet.dat unencrypted in memory. With this information it becomes rather trivial to reconstruct parts of a user's wallet.dat from a .core dump alone. > You can find the wallets within the core file simply by grepping for known parts of a wallet.dat ex: xxd bitcoin-qt.core | grep "6231 0500" With this information you can find the offset of the wallet within the core file, and reconstruct it per a known wallet.dat's length. Upon reloading the extracted wallet into bitcoin-qt, you'll lose address book information - but balance is retained. This has been assigned CVE-2019-15947. This is a backport of Core [[bitcoin/bitcoin#15600 | PR15600]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix, Fabien Reviewed By: #bitcoin_abc, deadalnix, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8879

Summary: > This is a followup to [[bitcoin/bitcoin#15600 | PR15600]] 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. This is a backport of Core [[bitcoin/bitcoin#18443 | PR18443]] Depends on D8879 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix, Fabien Reviewed By: #bitcoin_abc, deadalnix, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8880

bitcoin/bitcoin#15600