Bitcoin-Qt (Windows only): enable DEP for bitcoin-qt.exe #1614
Conversation
| // Minimum supported OS versions: WinXP SP3, WinVista >= SP1, Win Server 2008 | ||
| // A failure is non-critical and needs no further attention! | ||
| #ifndef PROCESS_DEP_ENABLE | ||
| #define PROCESS_DEP_ENABLE 0x00000001 |
There was a problem hiding this comment.
Why do we need to define this flag ourselves?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, fair enough. Can you add a comment saying this? Then we know when it can be removed, in the future.
There was a problem hiding this comment.
Yes, I'll include such a comment later today, so we can get this in.
|
ACK, this is a proven way to improve security (or at least it should limit damage of exploited bugs in a lot of cases). |
| // A failure is non-critical and needs no further attention! | ||
| #ifndef PROCESS_DEP_ENABLE | ||
| // We define this here, because GCCs winbase.h limits this to _WIN32_WINNT >= 0x0601 (Windows 7), | ||
| // which is not correct. Can be removed, when GCCs winbase.h is fixed! |
There was a problem hiding this comment.
Last rebase just adds this comment.
|
It's fine with me now. We need some of the other devs to take a look at this. |
|
@laanwj I hope anyone is using Windows or at least appreciates DEP support ;). |
|
Looks good, but I cannot test whether it works as intended. |
|
As I feared, no core dev is on Windows :). Can someone try if this compiles fine or how can we go on? |
|
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. |
|
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! |
- 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
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3d88c9b4d3714daddd77ab72d0e44b61c0b9800a for binaries and test log. |
|
Does this work on ReactOS? (I think it should be merged either way) |
|
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. |
|
@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. |
Bitcoin-Qt (Windows only): enable DEP for bitcoin-qt.exe
|
To go even further (for 0.8?) we should add the following MinGW linker flags on win32: |
|
@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 :). |
* Merge pull request bitcoin#7154 a3c3ddb [Qt] add InMempool() info to transaction details (Jonas Schnelli) * Merge pull request bitcoin#7218 fa5769e [qt] Fix misleading translation (MarcoFalke) fa8c8d7 torcontrol debug: Change to a blanket message that covers both cases (MarcoFalke) * Merge pull request bitcoin#7255 6fd0a07 Remove hardcoded fee from CoinControl ToolTip (fanquake) 5fdf32d Replace some instances of formatWithUnit with formatHtmlWithUnit (fanquake) * Merge pull request bitcoin#7263 a5a0831 Double semicolon cleanup. (21E14) * Merge pull request bitcoin#7334 fa989fb [qt] coincontrol workaround is still needed in qt5.4 (fixed in qt5.5) (MarcoFalke) * Merge pull request bitcoin#7329 9d263bd Typo fixes in comments (Chris Wheeler) * Merge bitcoin#7396: [Qt] Add option to increase/decrease font size in the console window 43abb02 [Qt] Add a new chevron/arrow icon for the console prompt line (Jonas Schnelli) 56c9e66 [Qt] keep scroll position in GUI console after changing font size (Jonas Schnelli) 3a3a927 [Qt] Add option to increase/decrease font size in the console window (Jonas Schnelli) * Merge bitcoin#7628: QT: Add 'copy full transaction details' option b51ed40 QT: Add 'copy full transaction details' option (Eric Shaw) * Merge bitcoin#7668: Fix history deletion bug after font size change 21e45a0 Fix history deletion bug after font change (Andrew C) * Copy/Move font size related icons into theme folders * Use formatTxDate for date/time in TxPlainTextRole
SetProcessDEPPolicy() call in Kernel32.dll
compiler libs
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: