windows: Enable heap terminate-on-corruption #17916

fanquake · 2020-01-13T08:54:08Z

This PR is currently two separate changes:

Enable heap terminate-on-corruption

This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc.

Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process.
After a process enables this feature, it cannot be disabled.

More info here.

Remove call to SetProcessDEPPolicy()

DEP is always enabled on 64-bit Windows processes, and SetProcessDEPPolicy() only works when called from a 32-bit process. I've tested that our current usage always fails (as expected) with ERROR_NOT_SUPPORTED.

Please don't add a "Needs gitian build" tag here yet.

This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. More info: https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation

SetProcessDEPPolicy() is only supported by 32-bit Windows processes. On 64-bit Windows it fails with ERROR_NOT_SUPPORTED (DEP is always enabled on 64-bit). More info: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy

laanwj · 2020-01-13T09:02:40Z

Concept ACK

sipsorcery · 2020-01-13T09:09:32Z

Setting HeapEnableTerminationOnCorruption seems like a sensible idea (trusting that it doesn't generate false positives like some of the debug memory monitoring tools).

But why remove the SetProcessDEPPolicy(PROCESS_DEP_ENABLE); call? It still has an effect on x86 builds. Or is this in anticipation of deprecating x86 support?

practicalswift · 2020-01-13T12:58:43Z

Concept ACK: enabling mitigations is good, ~~but it seems PROCESS_DEP_ENABLE should not be removed unless defined(_WIN64)?~~

Perhaps Chromium's ApplyProcessMitigationsToCurrentProcess(MitigationFlags flags) in win/src/process_mitigations.cc can serve as inspiration for our Windows mitigation strategies.

fanquake · 2020-01-14T01:59:56Z

But why remove the SetProcessDEPPolicy(PROCESS_DEP_ENABLE); call? It still has an effect on x86 builds. Or is this in anticipation of deprecating x86 support?

We stopped shipping 32-bit windows binaries as part of the 0.18.0 release, and have been removing windows 32-bit support from our build system (see #15939, #17756 etc).

can serve as inspiration for our Windows mitigation strategies.

Yep, as mentioned in the PR body, I have been looking at what other projects like tor, chromium etc are doing.

sipsorcery · 2020-01-14T08:05:39Z

ACK 3d5d7aa.

fanquake · 2020-01-15T03:59:41Z

If anyone would like to test/verify these changes, here are a couple things you can look at.

Process Explorer

Using Process Explorer, you can inspect Bitcoin Core while it's running and check that DEP is enabled. In the "Image File" tab for the process, you are looking for DEP Status: Enabled (Permanent) along with ASLR: High Entropy, Bottom Up etc. Below is the output for the 0.17.1 32-bit, 0.19.0.1 (release binaries) and 0.19.99.0 (this PR, built using WSL).

dumpbin

You can use dumpbin to dump the header info for the Bitcoin Core executables. You are looking for the DLL Characteristics and NX compatible. For example

dumpbin /headers bitcoind.exe (0.17.1 32-bit)
...
             140 DLL characteristics
                   Dynamic base
                   NX compatible

dumpbin /headers bitcoind.exe (0.19.99.0 this PR)
...
             160 DLL characteristics
                   High Entropy Virtual Addresses
                   Dynamic base
                   NX compatible

Verify ERROR_NOT_SUPPORTED

You could also test that our current call to SetProcessDEPPolicy() fails with ERROR_NOT_SUPPORTED.

diff --git a/src/init.cpp b/src/init.cpp
index dc0f2ce05..cf7d6e89f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -879,6 +879,8 @@ bool AppInitBasicSetup()
 #ifdef WIN32
     // Enable Data Execution Prevention (DEP)
     SetProcessDEPPolicy(PROCESS_DEP_ENABLE);
+    std::cout << "Failed to SetProcessDEPPolicy with: " << GetLastError() << "\n\n";
+    std::abort();
 #endif

laanwj · 2020-01-20T19:55:04Z

ACK 3d5d7aa

3d5d7aa windows: remove call to SetProcessDEPPolicy (fanquake) f2645c2 windows: Enable heap terminate-on-corruption (fanquake) Pull request description: This PR is currently two separate changes: #### Enable heap terminate-on-corruption This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc. > Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process. After a process enables this feature, it cannot be disabled. More info [here](https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation). #### Remove call to SetProcessDEPPolicy() DEP is always enabled on 64-bit Windows processes, and `SetProcessDEPPolicy()` only works when called from a 32-bit process. I've tested that our current usage always fails ([as expected](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy#remarks)) with [ERROR_NOT_SUPPORTED](https://github.com/mirror/mingw-w64/blob/16151c441e89081fd398270bb888511ebef6fb35/mingw-w64-headers/include/error.h#L42). Please don't add a "Needs gitian build" tag here yet. ACKs for top commit: sipsorcery: ACK 3d5d7aa. laanwj: ACK 3d5d7aa Tree-SHA512: 0948bcf165685b6b573f2cd950680c34356b856690de655ced2b93d497e02e7b22aa195c99f6ce33202f182622c67302ff31c98ab51b7d050574af3debdee5ce

This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. More info: https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation Github-Pull: bitcoin#17916 Rebased-From: f2645c2

- rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure bitcoin#18274 - httpserver: use own HTTP status codes bitcoin#18168 - tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions bitcoin#17229 - util: Don't allow Base32/64-decoding or ParseMoney(…) on strings with embedded NUL characters. Add tests. bitcoin#17753 - util: Fail to parse empty string in ParseMoney bitcoin#18225 - util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) bitcoin#18270 - Replace the LogPrint function with a macro bitcoin#17218 - Fix wallet unload race condition bitcoin#18338 - qt: Fix Window -> Minimize menu item bitcoin#18549 - windows: Enable heap terminate-on-corruption bitcoin#17916

…licy" This reverts commit 3d5d7aa.

3d5d7aa windows: remove call to SetProcessDEPPolicy (fanquake) f2645c2 windows: Enable heap terminate-on-corruption (fanquake) Pull request description: This PR is currently two separate changes: #### Enable heap terminate-on-corruption This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc. > Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process. After a process enables this feature, it cannot be disabled. More info [here](https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation). #### Remove call to SetProcessDEPPolicy() DEP is always enabled on 64-bit Windows processes, and `SetProcessDEPPolicy()` only works when called from a 32-bit process. I've tested that our current usage always fails ([as expected](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy#remarks)) with [ERROR_NOT_SUPPORTED](https://github.com/mirror/mingw-w64/blob/16151c441e89081fd398270bb888511ebef6fb35/mingw-w64-headers/include/error.h#L42). Please don't add a "Needs gitian build" tag here yet. ACKs for top commit: sipsorcery: ACK 3d5d7aa. laanwj: ACK 3d5d7aa Tree-SHA512: 0948bcf165685b6b573f2cd950680c34356b856690de655ced2b93d497e02e7b22aa195c99f6ce33202f182622c67302ff31c98ab51b7d050574af3debdee5ce

…licy" This reverts commit 3d5d7aa.

fanquake added 2 commits January 13, 2020 16:36

windows: Enable heap terminate-on-corruption

f2645c2

This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. More info: https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation

fanquake added Windows Needs Conceptual Review labels Jan 13, 2020

fanquake removed the Needs Conceptual Review label Jan 15, 2020

laanwj merged commit 3d5d7aa into bitcoin:master Jan 20, 2020

fanquake deleted the win_set_heap_terminate_on_corruption branch January 20, 2020 22:31

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020

Revert part of bitcoin#17916 "windows: remove call to SetProcessDEPPo…

3031715

…licy" This reverts commit 3d5d7aa.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 17, 2020

Revert part of bitcoin#17916 "windows: remove call to SetProcessDEPPo…

fd8c075

…licy" This reverts commit 3d5d7aa.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 25, 2020

Revert part of bitcoin#17916 "windows: remove call to SetProcessDEPPo…

8aceb82

…licy" This reverts commit 3d5d7aa.

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Oct 6, 2021

Revert part of bitcoin#17916 "windows: remove call to SetProcessDEPPo…

753db84

…licy" This reverts commit 3d5d7aa.

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

windows: Enable heap terminate-on-corruption #17916

windows: Enable heap terminate-on-corruption #17916

Uh oh!

fanquake commented Jan 13, 2020

Uh oh!

laanwj commented Jan 13, 2020

Uh oh!

sipsorcery commented Jan 13, 2020

Uh oh!

practicalswift commented Jan 13, 2020 •

edited

Loading

Uh oh!

fanquake commented Jan 14, 2020

Uh oh!

sipsorcery commented Jan 14, 2020

Uh oh!

fanquake commented Jan 15, 2020

Uh oh!

laanwj commented Jan 20, 2020

Uh oh!

Uh oh!

windows: Enable heap terminate-on-corruption #17916

windows: Enable heap terminate-on-corruption #17916

Uh oh!

Conversation

fanquake commented Jan 13, 2020

Enable heap terminate-on-corruption

Remove call to SetProcessDEPPolicy()

Uh oh!

laanwj commented Jan 13, 2020

Uh oh!

sipsorcery commented Jan 13, 2020

Uh oh!

practicalswift commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jan 14, 2020

Uh oh!

sipsorcery commented Jan 14, 2020

Uh oh!

fanquake commented Jan 15, 2020

Process Explorer

dumpbin

Verify ERROR_NOT_SUPPORTED

Uh oh!

laanwj commented Jan 20, 2020

Uh oh!

Uh oh!

practicalswift commented Jan 13, 2020 •

edited

Loading