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

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:check_for_SecureZeroMemory changing 1 files +4 −4
  1. 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.
  2. fanquake added the label Windows on Jan 23, 2023
  3. 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.

  4. hebasto commented at 1:10 pm on May 11, 2023: member
    Concept ACK.
  5. DrahtBot added the label CI failed on Oct 29, 2023
  6. DrahtBot removed the label CI failed on Nov 3, 2023
  7. DrahtBot added the label CI failed on Jan 13, 2024
  8. 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.

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

  10. 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
  11. fanquake force-pushed on Jul 23, 2024
  12. fanquake marked this as ready for review on Jul 23, 2024
  13. 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.
  14. DrahtBot removed the label CI failed on Jul 23, 2024
  15. cleanse: Use SecureZeroMemory for mingw-w64 (release) builds c399c80a09
  16. fanquake force-pushed on Jul 24, 2024
  17. sipa commented at 2:03 pm on July 24, 2024: member
    utACK c399c80a09a393d38368a44ef04753e9f62350f0
  18. DrahtBot requested review from laanwj on Jul 24, 2024
  19. DrahtBot requested review from theuni on Jul 24, 2024
  20. DrahtBot requested review from hebasto on Jul 24, 2024
  21. fanquake commented at 3:40 pm on July 24, 2024: member

    Guix Build (aarch64, x86_64):

     0b2617f05ed438479493daac5d6faa637d9bc8368186f41db270392ae8bcc2ff1  guix-build-c399c80a09a3/output/aarch64-linux-gnu/SHA256SUMS.part
 1255827ffdae24ce4099885caa4517a0d05e883adc49f92bf36226afa910d1b6b  guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu-debug.tar.gz
 2299e1401c19e901c30096b870b34c342318d08c89bc3b6ac02a82e718ddc41ad  guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu.tar.gz
 3dea5a78d4fb14deaa0ebc2d347a08064270722ebd5e69b71c11b54467a550635  guix-build-c399c80a09a3/output/arm-linux-gnueabihf/SHA256SUMS.part
 4d8c7a9c09a26ea81087a23b8821ea98dd2c493d7ed1caa443c394a62d4e0ad17  guix-build-c399c80a09a3/output/arm-linux-gnueabihf/bitcoin-c399c80a09a3-arm-linux-gnueabihf-debug.tar.gz
 523bf4d45d58f998916ec568a9514513ee00d923c7763423690a5e2a26a116a1d  guix-build-c399c80a09a3/output/arm-linux-gnueabihf/bitcoin-c399c80a09a3-arm-linux-gnueabihf.tar.gz
 65194c316a2877e0e3aab8ff2d0baa83f4d174f753ccc0cb7f22ec5763fd61521  guix-build-c399c80a09a3/output/arm64-apple-darwin/SHA256SUMS.part
 72ee6c28681e9709cd5e1e0e8e67ae790cfcb54ecbb33691efa2050c5a867e5c3  guix-build-c399c80a09a3/output/arm64-apple-darwin/bitcoin-c399c80a09a3-arm64-apple-darwin-unsigned.tar.gz
 8ec7da380ebe8a6cea8027f87b427f514d798731c35d75d95dfe8743936080f88  guix-build-c399c80a09a3/output/arm64-apple-darwin/bitcoin-c399c80a09a3-arm64-apple-darwin-unsigned.zip
 99977cdfc6505e0f05aeaa5364ef7f8d4a8372ec6ac81ac3bde6be23689bccf5c  guix-build-c399c80a09a3/output/arm64-apple-darwin/bitcoin-c399c80a09a3-arm64-apple-darwin.tar.gz
105015db4f0715103ad1c46b4b62d286a102b37f8fa504b07b0f88397317c0752c  guix-build-c399c80a09a3/output/dist-archive/bitcoin-c399c80a09a3.tar.gz
1179ebe7ab768c6a898c5dedfeb3c35d7ed6eaeb6855ef81d1c5bbddabde30a5b6  guix-build-c399c80a09a3/output/powerpc64-linux-gnu/SHA256SUMS.part
12e1d4f332fab403ddaa61df4b8b1f29d9d6e1c2828547173ea0b6fa8cfdd5d4dc  guix-build-c399c80a09a3/output/powerpc64-linux-gnu/bitcoin-c399c80a09a3-powerpc64-linux-gnu-debug.tar.gz
13b18355fd4c2310210446d4c9d6b742557da92654d2c5ba2966acc796af9ad4d2  guix-build-c399c80a09a3/output/powerpc64-linux-gnu/bitcoin-c399c80a09a3-powerpc64-linux-gnu.tar.gz
142a8eb8b846cb5ee8731aa7cde4de1d1676bdfc6e1f5d9896c053db5e7bda93fa  guix-build-c399c80a09a3/output/riscv64-linux-gnu/SHA256SUMS.part
15cfd2c41be44737c85c8947de727dfc77f62cfc2168e0561ad27790fa490665eb  guix-build-c399c80a09a3/output/riscv64-linux-gnu/bitcoin-c399c80a09a3-riscv64-linux-gnu-debug.tar.gz
16213e9fe419e1da23e7dbf7353fca2f9f8440222a3ddb350c9910e93aa7729089  guix-build-c399c80a09a3/output/riscv64-linux-gnu/bitcoin-c399c80a09a3-riscv64-linux-gnu.tar.gz
17579ee18e762c4719da2359d1fc31e09a70888ad1d94d198b7e975aced0ce1ca6  guix-build-c399c80a09a3/output/x86_64-apple-darwin/SHA256SUMS.part
181002d831af9ecf21ed27e538ea761c713744dcc7748fc321fb841baa3f87ad47  guix-build-c399c80a09a3/output/x86_64-apple-darwin/bitcoin-c399c80a09a3-x86_64-apple-darwin-unsigned.tar.gz
198a3940f34239e336cd7a6130ad83db9baf37a33eb77017484c1d3cbc25786f45  guix-build-c399c80a09a3/output/x86_64-apple-darwin/bitcoin-c399c80a09a3-x86_64-apple-darwin-unsigned.zip
205d10907bd83e1fdca0c70eddd8a4274b60d608951eab2861980ed390236eb636  guix-build-c399c80a09a3/output/x86_64-apple-darwin/bitcoin-c399c80a09a3-x86_64-apple-darwin.tar.gz
217be978e8f9c541d2d9ad39e09316b1865c1c62875265916974603a8fefc3159e  guix-build-c399c80a09a3/output/x86_64-linux-gnu/SHA256SUMS.part
223e6c6945f769227d47e09ce3b22a02589a2d636a952acff592484cd8f76d273a  guix-build-c399c80a09a3/output/x86_64-linux-gnu/bitcoin-c399c80a09a3-x86_64-linux-gnu-debug.tar.gz
23f0d1d22556d10897c550f95e8f98f15c504c4c00740c5405618b22c583b887ab  guix-build-c399c80a09a3/output/x86_64-linux-gnu/bitcoin-c399c80a09a3-x86_64-linux-gnu.tar.gz
241adc754353889f365755e1b3bcfc960df237c4e196cf591e0e07142bce902352  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/SHA256SUMS.part
258fe1cce9682d5055f3d53954954d5ca15897d7f5f578cf6d3451b1259bd2f72a  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64-debug.zip
26e1e513a81cb23e7ddbe147bb8bdc0881f5cd9c441204dc51d7e339bca1f0bd50  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64-setup-unsigned.exe
2709e74f2f3c015da2321ff22ea6b69d28a54259232567fd9bb86d5a1265c2b318  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64-unsigned.tar.gz
28c4a5673f962928a23515541322b32feb1fbf913ab753fd09ab70d2824ce26610  guix-build-c399c80a09a3/output/x86_64-w64-mingw32/bitcoin-c399c80a09a3-win64.zip
  22. TheCharlatan approved
  23. TheCharlatan commented at 4:12 pm on July 24, 2024: contributor
    ACK c399c80a09a393d38368a44ef04753e9f62350f0
  24. fanquake merged this on Jul 26, 2024
  25. fanquake closed this on Jul 26, 2024
  26. fanquake deleted the branch on Jul 26, 2024
  27. 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.

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

  28. 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)
  29. 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.

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


fanquake DrahtBot hebasto laanwj theuni sipa TheCharlatan real-or-random


laanwj theuni hebasto

Labels
Windows

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me