Bitcoin-Qt (Windows only): enable DEP for bitcoin-qt.exe #1614

Diapolo commented at 6:50 AM on July 20, 2012: none

this enables DEP on all Windows version which support the SetProcessDEPPolicy() call in Kernel32.dll
use a dynamic approach via GetProcAddress() to not rely on headers or compiler libs
this is the same way the Tor-project does it

See https://en.wikipedia.org/wiki/Data_Execution_Prevention for a detailed explanation of DEP! It is possible to enable this directly when linking, but this needs much more testing than this small patch :). I consider it a valuable 0.7 security feature on Windows.

To verify if DEP is enabled for bitcoin-qt.exe you can use Sysinternals ProcessExplorer:

verify DEP state

in src/init.cpp:None in 5df8d35d39 outdated

 311 | +#ifdef WIN32
 312 | +    // Enable Data Execution Prevention (DEP)
 313 | +    // Minimum supported OS versions: WinXP SP3, WinVista >= SP1, Win Server 2008
 314 | +    // A failure is non-critical and needs no further attention!
 315 | +#ifndef PROCESS_DEP_ENABLE
 316 | +#define PROCESS_DEP_ENABLE 0x00000001

laanwj commented at 6:22 AM on July 24, 2012:

Why do we need to define this flag ourselves?

Diapolo commented at 6:30 AM on July 24, 2012:

It IS in the MinGW headers (winbase.h) but just enabled for #if (_WIN32_WINNT >= 0x0601), which is Windows 7. As this is not correct (the used function and it's parameters are there since WinXP SP3).

I think it's better to re-define that flag instead of just using 0x00000001.

laanwj commented at 2:11 AM on July 26, 2012:

OK, fair enough. Can you add a comment saying this? Then we know when it can be removed, in the future.

Diapolo commented at 5:13 AM on July 26, 2012:

Yes, I'll include such a comment later today, so we can get this in.

laanwj commented at 2:13 AM on July 26, 2012: member

ACK, this is a proven way to improve security (or at least it should limit damage of exploited bugs in a lot of cases).

in src/init.cpp:None in e35be3e092 outdated

 312 | +    // Enable Data Execution Prevention (DEP)
 313 | +    // Minimum supported OS versions: WinXP SP3, WinVista >= SP1, Win Server 2008
 314 | +    // A failure is non-critical and needs no further attention!
 315 | +#ifndef PROCESS_DEP_ENABLE
 316 | +// We define this here, because GCCs winbase.h limits this to _WIN32_WINNT >= 0x0601 (Windows 7),
 317 | +// which is not correct. Can be removed, when GCCs winbase.h is fixed!

Diapolo commented at 8:06 AM on July 26, 2012:

Last rebase just adds this comment.

laanwj commented at 10:56 AM on July 27, 2012: member

It's fine with me now. We need some of the other devs to take a look at this.

Diapolo commented at 12:06 PM on July 27, 2012: none

@laanwj I hope anyone is using Windows or at least appreciates DEP support ;).

sipa commented at 5:28 PM on July 31, 2012: member

Looks good, but I cannot test whether it works as intended.

Diapolo commented at 5:24 AM on August 1, 2012: none

As I feared, no core dev is on Windows :). Can someone try if this compiles fine or how can we go on?

laanwj commented at 7:31 AM on August 2, 2012: member

You tested it yesterday with code on the stack, and it worked. Shall we be bold and merge this? It's the same code as used by Tor so we can't go much wrong.

Diapolo commented at 8:00 AM on August 2, 2012: none

I strongly encourage you / the core devs to merge this. IF (what I don't expect to happen) there are errors, we can easily fix / disable this patch during the coming RC phase.

It's a great security benefit with little code IMHO!

Bitcoin-Qt (Windows only): enable DEP for bitcoin-qt.exe

- this enables DEP on all Windows version which support the
  SetProcessDEPPolicy() call in Kernel32.dll
- use a dynamic approach via GetProcAddress() to not rely on headers or
  compiler libs
- this is the same way the Tor-project does it

3d88c9b4d3

BitcoinPullTester commented at 3:16 AM on August 9, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3d88c9b4d3714daddd77ab72d0e44b61c0b9800a for binaries and test log.

luke-jr commented at 10:17 PM on August 12, 2012: member

Does this work on ReactOS? (I think it should be merged either way)

laanwj commented at 6:25 AM on August 13, 2012: member

I'm fine with merging this as long as it does not crash on any platform.

It doesn't worry me if it doesn't manage to enable DEP on some more obscure platform. Until a developer steps up and cares about ReactOS, support for that is not a concern.

Diapolo commented at 8:42 PM on August 13, 2012: none

@luke-jr I'm not able or really willing to test this with ReactOS ^^, but as I just use the Windows-API here with no bad things happening, when the code fails, I think this is fine.

laanwj referenced this in commit a55ed9d5bb on Aug 14, 2012

laanwj merged this on Aug 14, 2012

laanwj closed this on Aug 14, 2012

laanwj commented at 4:03 AM on August 14, 2012: member

To go even further (for 0.8?) we should add the following MinGW linker flags on win32:

--dynamicbase  The image base address may be relocated using address space layout randomization ( ASLR ). This feature was introduced with MS Windows Vista for i386 PE targets.

--nxcompat The image is compatible with the Data Execution Prevention. This feature was introduced with MS Windows XP SP2 for i386 PE targets.

Diapolo commented at 8:56 AM on August 14, 2012: none

@laanwj I use these 2 linker flags since a few weeks now for my local build and they work fine. I just wanted to ensure at least basic DEP get's in before 0.7 and wanted to create another pull for ASLR and DEP linker-flags after this got in :).

suprnurd referenced this in commit 690cb58f80 on Dec 5, 2017

DrahtBot locked this on Sep 8, 2021