cleanse: switch to SecureZeroMemory for Windows cross-compile #26950

fanquake · 2023-01-23T11:59:20Z

This PR switches our Windows release builds to use the SecureZeroMemory() provided by mingw-w64.

DrahtBot · 2023-01-23T11:59:23Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, TheCharlatan
Concept ACK	hebasto, laanwj, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30423 (contrib: simplify test-security-check by fanquake)
#29790 ([DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP by hebasto)
#27038 (security-check: test for _FORTIFY_SOURCE usage in release binaries by fanquake)
#25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2023-05-11T13:10:06Z

Concept ACK.

laanwj · 2024-04-24T07:26:58Z

Concept ACK

Using SecureZeroMemory when it's available on Windows makes a lot of sense imo.

contrib/devtools/security-check.py

theuni

Concept ACK. Can't think of any reason why not, if mingw supports it.

sipa · 2024-07-24T14:03:16Z

utACK c399c80

fanquake · 2024-07-24T15:40:23Z

Guix Build (aarch64, x86_64):

b2617f05ed438479493daac5d6faa637d9bc8368186f41db270392ae8bcc2ff1  guix-build-c399c80a09a3/output/aarch64-linux-gnu/SHA256SUMS.part
255827ffdae24ce4099885caa4517a0d05e883adc49f92bf36226afa910d1b6b  guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu-debug.tar.gz
299e1401c19e901c30096b870b34c342318d08c89bc3b6ac02a82e718ddc41ad  guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu.tar.gz
dea5a78d4fb14deaa0ebc2d347a08064270722ebd5e69b71c11b54467a550635  guix-build-c399c80a09a3/output/arm-linux-gnueabihf/SHA256SUMS.part
d8c7a9c09a26ea81087a23b8821ea98dd2c493d7ed1caa443c394a62d4e0ad17  guix-build-c399c80a09a3/output/arm-linux-gnueabihf/bitcoin-c399c80a09a3-arm-linux-gnueabihf-debug.tar.gz
23bf4d45d58f998916ec568a9514513ee00d923c7763423690a5e2a26a116a1d  guix-build-c399c80a09a3/output/arm-linux-gnueabihf/bitcoin-c399c80a09a3-arm-linux-gnueabihf.tar.gz
5194c316a2877e0e3aab8ff2d0baa83f4d174f753ccc0cb7f22ec5763fd61521  guix-build-c399c80a09a3/output/arm64-apple-darwin/SHA256SUMS.part
2ee6c28681e9709cd5e1e0e8e67ae790cfcb54ecbb33691efa2050c5a867e5c3  guix-build-c399c80a09a3/output/arm64-apple-darwin/bitcoin-c399c80a09a3-arm64-apple-darwin-unsigned.tar.gz
ec7da380ebe8a6cea8027f87b427f514d798731c35d75d95dfe8743936080f88  guix-build-c399c80a09a3/output/arm64-apple-darwin/bitcoin-c399c80a09a3-arm64-apple-darwin-unsigned.zip
9977cdfc6505e0f05aeaa5364ef7f8d4a8372ec6ac81ac3bde6be23689bccf5c  guix-build-c399c80a09a3/output/arm64-apple-darwin/bitcoin-c399c80a09a3-arm64-apple-darwin.tar.gz
5015db4f0715103ad1c46b4b62d286a102b37f8fa504b07b0f88397317c0752c  guix-build-c399c80a09a3/output/dist-archive/bitcoin-c399c80a09a3.tar.gz
79ebe7ab768c6a898c5dedfeb3c35d7ed6eaeb6855ef81d1c5bbddabde30a5b6  guix-build-c399c80a09a3/output/powerpc64-linux-gnu/SHA256SUMS.part
e1d4f332fab403ddaa61df4b8b1f29d9d6e1c2828547173ea0b6fa8cfdd5d4dc  guix-build-c399c80a09a3/output/powerpc64-linux-gnu/bitcoin-c399c80a09a3-powerpc64-linux-gnu-debug.tar.gz
b18355fd4c2310210446d4c9d6b742557da92654d2c5ba2966acc796af9ad4d2  guix-build-c399c80a09a3/output/powerpc64-linux-gnu/bitcoin-c399c80a09a3-powerpc64-linux-gnu.tar.gz
2a8eb8b846cb5ee8731aa7cde4de1d1676bdfc6e1f5d9896c053db5e7bda93fa  guix-build-c399c80a09a3/output/riscv64-linux-gnu/SHA256SUMS.part
cfd2c41be44737c85c8947de727dfc77f62cfc2168e0561ad27790fa490665eb  guix-build-c399c80a09a3/output/riscv64-linux-gnu/bitcoin-c399c80a09a3-riscv64-linux-gnu-debug.tar.gz
213e9fe419e1da23e7dbf7353fca2f9f8440222a3ddb350c9910e93aa7729089  guix-build-c399c80a09a3/output/riscv64-linux-gnu/bitcoin-c399c80a09a3-riscv64-linux-gnu.tar.gz
579ee18e762c4719da2359d1fc31e09a70888ad1d94d198b7e975aced0ce1ca6  guix-build-c399c80a09a3/output/x86_64-apple-darwin/SHA256SUMS.part
1002d831af9ecf21ed27e538ea761c713744dcc7748fc321fb841baa3f87ad47  guix-build-c399c80a09a3/output/x86_64-apple-darwin/bitcoin-c399c80a09a3-x86_64-apple-darwin-unsigned.tar.gz
8a3940f34239e336cd7a6130ad83db9baf37a33eb77017484c1d3cbc25786f45  guix-build-c399c80a09a3/output/x86_64-apple-darwin/bitcoin-c399c80a09a3-x86_64-apple-darwin-unsigned.zip
5d10907bd83e1fdca0c70eddd8a4274b60d608951eab2861980ed390236eb636  guix-build-c399c80a09a3/output/x86_64-apple-darwin/bitcoin-c399c80a09a3-x86_64-apple-darwin.tar.gz
7be978e8f9c541d2d9ad39e09316b1865c1c62875265916974603a8fefc3159e  guix-build-c399c80a09a3/output/x86_64-linux-gnu/SHA256SUMS.part
3e6c6945f769227d47e09ce3b22a02589a2d636a952acff592484cd8f76d273a  guix-build-c399c80a09a3/output/x86_64-linux-gnu/bitcoin-c399c80a09a3-x86_64-linux-gnu-debug.tar.gz
f0d1d22556d10897c550f95e8f98f15c504c4c00740c5405618b22c583b887ab  guix-build-c399c80a09a3/output/x86_64-linux-gnu/bitcoin-c399c80a09a3-x86_64-linux-gnu.tar.gz
1adc754353889f365755e1b3bcfc960df237c4e196cf591e0e07142bce902352  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/SHA256SUMS.part
8fe1cce9682d5055f3d53954954d5ca15897d7f5f578cf6d3451b1259bd2f72a  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64-debug.zip
e1e513a81cb23e7ddbe147bb8bdc0881f5cd9c441204dc51d7e339bca1f0bd50  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64-setup-unsigned.exe
09e74f2f3c015da2321ff22ea6b69d28a54259232567fd9bb86d5a1265c2b318  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64-unsigned.tar.gz
c4a5673f962928a23515541322b32feb1fbf913ab753fd09ab70d2824ce26610  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64.zip

TheCharlatan

ACK c399c80

real-or-random · 2024-10-22T15:34:22Z

@fanquake Can you elaborate on the motivation for this PR? The code in the #else should work for any gcc and clang, on any platform. While it uses an asm block, there's no actual asm instruction inside that could be platform-specific.

See also its platform-independent usage in Linux:
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux/string.h#L369-L376
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux/compiler.h#L89-L102

So I'm not sure if this commit has improved anything. But I also don't see how it has worsened anything (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).

I'm not claiming that this is wrong, I'm just trying to understand. We'd like to include a cleanse function in libsecp256k1, see bitcoin-core/secp256k1#1579 Was this needed to fix some cross-compilation? I don't know much about Windows, but I worry a bit that defined(WIN32) guarantees the availability of the SecureZeroMemory, even on targets beyond those supported by Core.

laanwj · 2024-10-22T17:04:48Z

i think the goal here was to make Windows code consistent between MSVC and mingw. MSVC is windows' official toolchain so it's leading for that. Using something else for Mingw could be considered a workaround.
(though sure, whether you prefer OS-specific or compiler-specific defines could be debated)

fanquake · 2024-10-23T08:43:40Z

@real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.

(except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).

I would think the code would not be worse performance wise, as it's implemented using __stosb.

real-or-random · 2024-10-25T13:56:26Z

@real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.

Thanks, makes sense.

(except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).

I would think the code would not be worse performance wise, as it's implemented using __stosb.

Indeed, just not on ARM, see https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/intrincs/RtlSecureZeroMemory.c. But sure, this is entirely fine, in particular for Core.

I think we may still stick to the MSVC macro because we currently have only compiler-specific ifdefs, and no OS-specific ifdefs.

…cross-compile c399c80 cleanse: Use SecureZeroMemory for mingw-w64 (release) builds (fanquake) Pull request description: This PR switches our Windows release builds to use the [`SecureZeroMemory()`](https://learn.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa366877(v=vs.85)) provided by mingw-w64. ACKs for top commit: sipa: utACK c399c80 TheCharlatan: ACK c399c80 Tree-SHA512: dbb20b16c85061d2f9408a3cf69cecc16765f8f61b25a1707146767b664c7ad0caf36975380814ef8e7c49a30199daebac6d5d7a3585354d1adac8e9770199c6

b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script) 745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow) 01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow) 432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script) 1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script) 8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script) f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script) ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script) e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script) df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script) 57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script) e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script) 62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script) 745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow) 4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov) 69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script) ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script) 9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow) 479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script) ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script) 63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script) 3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script) 3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b654479 kwvg: utACK b654479 Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043

fanquake added the Windows label Jan 23, 2023

DrahtBot mentioned this pull request Jan 23, 2023

[POC] guix: produce a fully -static-pie bitcoind #25573

Draft

DrahtBot added CI failed and removed CI failed labels Oct 29, 2023

DrahtBot added the CI failed label Jan 13, 2024

DrahtBot mentioned this pull request Apr 2, 2024

[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP #29790

Closed

laanwj reviewed Apr 24, 2024

View reviewed changes

contrib/devtools/security-check.py Outdated Show resolved Hide resolved

This was referenced Jul 12, 2024

contrib: simplify test-security-check #30423

Merged

security-check: test for _FORTIFY_SOURCE usage in release binaries #27038

Merged

build: Introduce CMake-based build system #30454

Merged

fanquake changed the title ~~cleanse: switch to SecureZeroMemory for Windows cross-compile, check for usage~~ cleanse: switch to SecureZeroMemory for Windows cross-compile Jul 23, 2024

fanquake force-pushed the check_for_SecureZeroMemory branch from 7fe2153 to 60a8cac Compare July 23, 2024 16:18

fanquake marked this pull request as ready for review July 23, 2024 16:33

theuni reviewed Jul 23, 2024

View reviewed changes

DrahtBot removed the CI failed label Jul 23, 2024

cleanse: Use SecureZeroMemory for mingw-w64 (release) builds

c399c80

fanquake force-pushed the check_for_SecureZeroMemory branch from 60a8cac to c399c80 Compare July 24, 2024 13:59

DrahtBot requested review from laanwj, theuni and hebasto July 24, 2024 14:03

TheCharlatan approved these changes Jul 24, 2024

View reviewed changes

fanquake merged commit 02c76ad into bitcoin:master Jul 26, 2024
16 checks passed

fanquake deleted the check_for_SecureZeroMemory branch July 26, 2024 06:09

real-or-random mentioned this pull request Oct 22, 2024

Clear sensitive memory without getting optimized out (revival of #636) bitcoin-core/secp256k1#1579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cleanse: switch to SecureZeroMemory for Windows cross-compile #26950

cleanse: switch to SecureZeroMemory for Windows cross-compile #26950

Uh oh!

fanquake commented Jan 23, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Jan 23, 2023 •

edited

Loading

Uh oh!

hebasto commented May 11, 2023

Uh oh!

laanwj commented Apr 24, 2024 •

edited

Loading

Uh oh!

Uh oh!

theuni left a comment

Uh oh!

sipa commented Jul 24, 2024

Uh oh!

fanquake commented Jul 24, 2024 •

edited

Loading

Uh oh!

TheCharlatan left a comment

Uh oh!

Uh oh!

real-or-random commented Oct 22, 2024

Uh oh!

laanwj commented Oct 22, 2024 •

edited

Loading

Uh oh!

fanquake commented Oct 23, 2024

Uh oh!

real-or-random commented Oct 25, 2024

Uh oh!

Uh oh!

cleanse: switch to SecureZeroMemory for Windows cross-compile #26950

cleanse: switch to SecureZeroMemory for Windows cross-compile #26950

Uh oh!

Conversation

fanquake commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

hebasto commented May 11, 2023

Uh oh!

laanwj commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 24, 2024

Uh oh!

fanquake commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

real-or-random commented Oct 22, 2024

Uh oh!

laanwj commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 23, 2024

Uh oh!

real-or-random commented Oct 25, 2024

Uh oh!

Uh oh!

fanquake commented Jan 23, 2023 •

edited

Loading

DrahtBot commented Jan 23, 2023 •

edited

Loading

laanwj commented Apr 24, 2024 •

edited

Loading

fanquake commented Jul 24, 2024 •

edited

Loading

laanwj commented Oct 22, 2024 •

edited

Loading