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

fanquake commented at 11:59 AM on January 23, 2023: member

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

fanquake added the label Windows on Jan 23, 2023

DrahtBot commented at 11:59 AM on January 23, 2023: contributor

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 commented at 1:10 PM on May 11, 2023: member

Concept ACK.

DrahtBot added the label CI failed on Oct 29, 2023

DrahtBot removed the label CI failed on Nov 3, 2023

DrahtBot added the label CI failed on Jan 13, 2024

laanwj commented at 7:26 AM on April 24, 2024: member

Concept ACK

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

in contrib/devtools/security-check.py:171 in 7fe215390e outdated

 166 | +    # We are looking for rep stosb, which is f3 aa (243 170).
 167 | +    # We search for 170, and check for a preceding 243,
 168 | +    # so we don't match the endbr64 instruction at the
 169 | +    # beginning of the function.
 170 | +    aa = content.index(170)
 171 | +    return content[aa-1] == 243

laanwj commented at 7:30 AM on April 24, 2024:

This checks that the function is there, not whether it is used, in the right places. But it may be enough as a start. Edit: Oh never mind, you're not looking for SecureZeroMemory itself, only its implementation within memory_cleanse. This makes sense.

fanquake commented at 11:40 AM on April 24, 2024:

Oh never mind, you're not looking for SecureZeroMemory itself, only its implementation within memory_cleanse. This makes sense.

Yea. Happy to try and extend / improve things further where we can.

fanquake renamed this:
~~cleanse: switch to SecureZeroMemory for Windows cross-compile, check for usage~~
cleanse: switch to SecureZeroMemory for Windows cross-compile
on Jul 23, 2024

fanquake force-pushed on Jul 23, 2024

fanquake marked this as ready for review on Jul 23, 2024

theuni commented at 4:53 PM on July 23, 2024: member

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

DrahtBot removed the label CI failed on Jul 23, 2024

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

fanquake force-pushed on Jul 24, 2024

sipa commented at 2:03 PM on July 24, 2024: member

utACK c399c80a09a393d38368a44ef04753e9f62350f0

DrahtBot requested review from laanwj on Jul 24, 2024

DrahtBot requested review from theuni on Jul 24, 2024

DrahtBot requested review from hebasto on Jul 24, 2024

fanquake commented at 3:40 PM on July 24, 2024: member

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 approved

TheCharlatan commented at 4:12 PM on July 24, 2024: contributor

ACK c399c80a09a393d38368a44ef04753e9f62350f0

fanquake merged this on Jul 26, 2024

fanquake closed this on Jul 26, 2024

fanquake deleted the branch on Jul 26, 2024

real-or-random commented at 3:34 PM on October 22, 2024: contributor

@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.

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 https://github.com/bitcoin-core/secp256k1/pull/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 commented at 5:04 PM on October 22, 2024: member

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 commented at 8:43 AM on October 23, 2024: member

@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 commented at 1:56 PM on October 25, 2024: contributor

@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.

PastaPastaPasta referenced this in commit c55c7c62be on Oct 25, 2024

PastaPastaPasta referenced this in commit df3c2392ca on Oct 27, 2024

PastaPastaPasta referenced this in commit 565f2db930 on Oct 27, 2024

bitcoin locked this on Oct 25, 2025