build: Drop option to disable hardening.

davidgumberg commented at 6:57 pm on March 14, 2025: contributor

Follow up to #32038 which dropped NO_HARDEN from depends builds, this PR drops the ENABLE_HARDENING build option since disabling hardening of binaries should not be a supported or maintained use case. With this change, hardening flags are always enabled.

Individual hardening flags and options can still be disabled by appending flags, e.g.:

0cmake -B build \
1  -DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -fno-stack-protector -fcf-protection=none -fno-stack-clash-protection' \
2  -DAPPEND_LDFLAGS='-Wl,-z,lazy -Wl,-z,norelro -Wl,-z,noseparate-code'

There is an issue with NetBSD 10.0’s dynamic linker that makes one of the hardening linker flags, -z separate-code, problematic, so this PR also introduces a check to prevent the use of this flag in NetBSD versions < 11.0, (where this issue is fixed). The fix for this might be backported to NetBSD 10.0.

I suggest reviewing the diff with whitespace changes hidden (git diff -w or using github’s hide whitespace option)

DrahtBot commented at 6:57 pm on March 14, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32071.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, laanwj, janb84, vasild, musaHaruna
Concept ACK	sipa
Stale ACK	maflcko

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Build system on Mar 14, 2025

maflcko commented at 8:44 am on March 15, 2025: member

review ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb 📮

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb 📮
3kuLcmyw1Kv99uptN9UVbNG0JwSyCRffWoUy3VFb9NZGP/v7+SpCYNa5PrWtPOGQHROhCkU6Rgf2BpXdp95KKAg==

laanwj approved

laanwj commented at 7:50 am on March 17, 2025: member

Concept and code review ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb rationale: #32038 (comment) #32038 (comment)

in CMakeLists.txt:484 in ecf2046d4b outdated

487-    try_append_linker_flag("/DYNAMICBASE" TARGET hardening_interface)
488-    try_append_linker_flag("/HIGHENTROPYVA" TARGET hardening_interface)
489-    try_append_linker_flag("/NXCOMPAT" TARGET hardening_interface)
490-  else()
491+add_library(hardening_interface INTERFACE)
492+target_link_libraries(core_interface INTERFACE hardening_interface)

vasild commented at 11:59 am on March 17, 2025:

hardening_interface is not needed anymore? The surrounding code appends stuff directly to core_interface. Maybe remove these 2 lines and s/hardening_interface/core_interface/ all over the place?

davidgumberg commented at 2:59 am on March 18, 2025:

Thanks for catching this, will address in a refactor follow-up, or if I touch here again.

davidgumberg commented at 10:11 pm on March 21, 2025:

Fixed in latest push

vasild approved

vasild commented at 11:59 am on March 17, 2025: contributor

ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb

hebasto commented at 1:12 pm on March 17, 2025: member

Approach ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb.

The only case I’m aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.

For more details, see:

laanwj commented at 3:01 pm on March 17, 2025: member

The only case I’m aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.

Right. Not sure if it’s worth it in this specific case, as it’s going to be fixed upstream soon, but imo in general if some flags (hardening or otherwise) are a problem on some platform this should be taken into account by the build system automatically instead of having the user disable hardening entirely, which is a hassle and creates a bigger risk than necessary.

davidgumberg commented at 3:56 am on March 18, 2025: contributor

The only case I’m aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.

in general if some flags (hardening or otherwise) are a problem on some platform this should be taken into account by the build system automatically instead of having the user disable hardening entirely.

Okay, I think this branch will break building on NetBSD, and shouldn’t be merged as-is, I’ll follow up.

fanquake commented at 3:59 am on March 18, 2025: member

I’ll follow up.

Do you want to split out 7d34c19853e7a5528d69c5f30580e7e9712e61f0? That can be merged now.

davidgumberg commented at 4:34 am on March 18, 2025: contributor

Do you want to split out 7d34c19? That can be merged now.

Opened https://github.com/bitcoin/bitcoin/pull/32087

fanquake referenced this in commit ece0b41da6 on Mar 18, 2025

hebasto commented at 9:28 am on March 18, 2025: member

The only case I’m aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.

in general if some flags (hardening or otherwise) are a problem on some platform this should be taken into account by the build system automatically instead of having the user disable hardening entirely.

Okay, I think this branch will break building on NetBSD, and shouldn’t be merged as-is, I’ll follow up.

Perhaps I should clarify that only the libbitcoinkernel.so shared library is broken. All executables run fine, except that invoking ldd (or any other operation involving the dynamic linker) fails.

Therefore, I’m fine with dropping NO_HARDEN.

fanquake commented at 7:24 am on March 21, 2025: member

@davidgumberg want to rebase here?

davidgumberg force-pushed on Mar 21, 2025

davidgumberg commented at 10:13 pm on March 21, 2025: contributor

Force-pushed to rebase and address @vasild feedback to drop hardening_interface, and added a check to CMakeLists.txt to avoid -z separate-code on NetBSD < 11.0 (thanks @hebasto). I’ve also updated the PR description.

davidgumberg force-pushed on Mar 21, 2025

in CMakeLists.txt:541 in 0e98ec819b outdated

585+  # have 4 `PT_LOAD` segments.
586+  # Relevant discussions:
587+  # - https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934
588+  # - https://mail-index.netbsd.org/tech-userlevel/2023/01/05/msg013666.html
589+  if(CMAKE_SYSTEM_NAME STREQUAL "NetBSD" AND CMAKE_SYSTEM_VERSION VERSION_LESS 11.0)
590+      try_append_linker_flag("-Wl,-z,noseparate-code" TARGET core_interface)

hebasto commented at 1:28 pm on March 24, 2025:

style nit: two-spaces indent.

davidgumberg commented at 8:40 pm on March 24, 2025:

Thanks, fixed

hebasto approved

hebasto commented at 1:42 pm on March 24, 2025: member

ACK 0e98ec819b65cfc1728a6aecf5e43a7c73756a66, I have reviewed the code and it looks OK.

Here are NetBSD CI jobs for this branch: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/14036068740.

DrahtBot requested review from vasild on Mar 24, 2025

DrahtBot requested review from maflcko on Mar 24, 2025

DrahtBot requested review from laanwj on Mar 24, 2025

build: Use `-z noseparate-code` on NetBSD < 11.0

This can be dropped once Bitcoin Core no longer supports NetBSD 10.0 or
if upstream fix is backported.

NetBSD's dynamic linker ld.elf_so < 11.0 supports exactly 2 `PT_LOAD`
segments and binaries linked with `-z separate-code` have 4 `PT_LOAD`
segments.

https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934
https://mail-index.netbsd.org/tech-userlevel/2023/01/05/msg013666.html

f57db75e91

build: Drop option for disabling hardening

Building unhardened executables is not a supported use case that should
be maintained and those that want unhardened executables can still
override them by appending disable flags.

For example:

cmake -B build -DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -fno-stack-protector -fcf-protection=none -fno-stack-clash-protection' -DAPPEND_LDFLAGS='-Wl,-z,lazy -Wl,-z,norelro -Wl,-z,noseparate-code'

00ba3ba303

build: refactor: hardening flags -> core_interface 77e553ab6a

davidgumberg force-pushed on Mar 24, 2025

hebasto approved

hebasto commented at 8:46 am on March 25, 2025: member

re-ACK 77e553ab6a0a98d065884a83490f28eec6df0e23.

laanwj approved

laanwj commented at 8:51 am on March 25, 2025: member

re-ACK 77e553ab6a0a98d065884a83490f28eec6df0e23

fanquake commented at 8:08 am on March 27, 2025: member

cc @theuni

janb84 commented at 7:11 pm on April 3, 2025: contributor

ACK 77e553a

code reviewed ✅
build ✅
tested ✅

in CMakeLists.txt:538 in 77e553ab6a

582+  #       NetBSD 10.0 or if upstream fix is backported.
583+  # NetBSD's dynamic linker ld.elf_so < 11.0 supports exactly 2
584+  # `PT_LOAD` segments and binaries linked with `-z separate-code`
585+  # have 4 `PT_LOAD` segments.
586+  # Relevant discussions:
587+  # - https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934

fanquake commented at 6:35 am on April 4, 2025:

This whole block seems pretty verbose (we don’t need to repeat all the details from the NetBSD mailinglist), and its not actually clear these workarounds are needed for anything other than for the experimental kernel library, according to #32071 (comment)? The linked comment in our repo just repeats what’s in the mailing list post, so could also be dropped.

hebasto commented at 8:03 am on April 4, 2025:

From the mentioned #32071 (comment):

Perhaps I should clarify that only the libbitcoinkernel.so shared library is broken. All executables run fine, except that invoking ldd (or any other operation involving the dynamic linker) fails.

I believe it’s helpful to ensure that running ldd bitcoind produces no errors.

fanquake commented at 10:13 am on April 11, 2025:

I believe it’s helpful to ensure that running ldd bitcoind produces no errors.

It’s not really clear that disabling hardening based on (a buggy?) ldd is the correct tradeoff.

I also don’t completely understand your comment in regards to the binaries running fine, but simultaneously, any operation using the dynamic linker (i.e running the binaries) failing.

davidgumberg commented at 10:22 pm on April 11, 2025:

This whole block seems pretty verbose (we don’t need to repeat all the details from the NetBSD mailinglist), and its not actually clear these workarounds are needed for anything other than for the experimental kernel library, according to #32071 (comment)? The linked comment in our repo just repeats what’s in the mailing list post, so could also be dropped.

Agreed, but I am not sure if this is blocking and would prefer to only change this if retouching to avoid invalidating ACK’s.

It’s not really clear that disabling hardening based on (a buggy?) ldd is the correct tradeoff.

That is fair, but only separate-code is being disabled, and while having separate-code enabled is strictly better in theory, if I understand correctly, in practice it is dubious whether or not there are any likely situations where an attacker will not be able to find a gadget in a text segment but find one in the page overlap between text and data segments.

Upstream discussion in LLD where -noseparate-code was made default (https://reviews.llvm.org/D64903):

I don’t think -z separate-code is a very effective defense against a ROP/JOP. If a program is not very small, it shouldn’t be too hard to find a gadget from a text segment, and I don’t think an additional piece of data that is at the remaining part of the last page of a text segment can significantly increase a risk. If your program is vulnerable to ROP/JOP, and gadgets are at predictable places, it’s a game end regardless of an existence of some extra data at the end of a text segment.

luke-jr commented at 2:30 am on April 10, 2025: member

this PR also introduces a check to prevent the use of this flag in NetBSD versions < 11.0, (where this issue is fixed). The fix for this might be backported to NetBSD 10.0.

If the fix is backported, presumably your change will still disable it?

vasild approved

vasild commented at 9:51 am on April 10, 2025: contributor

ACK 77e553ab6a0a98d065884a83490f28eec6df0e23

sipa commented at 11:44 am on April 11, 2025: member

Concept ACK

davidgumberg commented at 10:43 pm on April 11, 2025: contributor

this PR also introduces a check to prevent the use of this flag in NetBSD versions < 11.0, (where this issue is fixed). The fix for this might be backported to NetBSD 10.0.

If the fix is backported, presumably your change will still disable it?

Yes, but I think that’s okay, since only one flag of dubious hardening benefit is disabled on NetBSD. More discussion in this comment: #32071 (review)

maflcko added the label DrahtBot Guix build requested on Apr 16, 2025

musaHaruna commented at 5:33 pm on April 17, 2025: none

tested ACK 77e553 Code reviewed and built successfully

DrahtBot requested review from sipa on Apr 17, 2025

DrahtBot commented at 8:51 pm on April 19, 2025: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 055254e212a260c0d108f424ac32e0f2e8f3a07a(master)	commit 641ced6465a0a1163c8340395ed2063b7cd8a367(pull/32071/merge)
*-aarch64-linux-gnu-debug.tar.gz	`6958692cb4a5c8d1...`	`d38c18f2bc9ff027...`
*-aarch64-linux-gnu.tar.gz	`3490d7bae603cd71...`	`1c7ae998b9c5b7c8...`
*-arm-linux-gnueabihf-debug.tar.gz	`70f77901477929bb...`	`8712014bc0997a01...`
*-arm-linux-gnueabihf.tar.gz	`e4973cb9a9cb92af...`	`61a0a2149904fe08...`
*-arm64-apple-darwin-codesigning.tar.gz	`a9c64587c616e9bc...`	`c8010709380b0272...`
*-arm64-apple-darwin-unsigned.tar.gz	`e9da12bf10fbb599...`	`7b519c49e9eca17e...`
*-arm64-apple-darwin-unsigned.zip	`fe1bde00f81b6b83...`	`579b60fdc3372143...`
*-powerpc64-linux-gnu-debug.tar.gz	`be2bdb34ab87fa18...`	`0c3a94a717606628...`
*-powerpc64-linux-gnu.tar.gz	`b2edc2247d66e3a3...`	`eb7cdc23dcec60cb...`
*-riscv64-linux-gnu-debug.tar.gz	`7e57194d799282e1...`	`d81d323f9415875c...`
*-riscv64-linux-gnu.tar.gz	`552b26dd046591b0...`	`62745b263504bcfb...`
*-x86_64-apple-darwin-codesigning.tar.gz	`c0879281383ca6d3...`	`6c22146f4be2a76f...`
*-x86_64-apple-darwin-unsigned.tar.gz	`abfab6ceeecba5f0...`	`7c8e07cf1dfcd45f...`
*-x86_64-apple-darwin-unsigned.zip	`9530db22ec18aa4c...`	`ad990733e4c751da...`
*-x86_64-linux-gnu-debug.tar.gz	`6387ec83bfd80c7c...`	`e8385ccdaaf88586...`
*-x86_64-linux-gnu.tar.gz	`09b3afaa1f7cbb7d...`	`7feb84525f638e76...`
*.tar.gz	`ee9a589f63dd7186...`	`1c73c9327e285b3c...`
SHA256SUMS.part	`627adcd835150acb...`	`1f6542e1d219c144...`
guix_build.log	`228812ae9a01ba79...`	`028a5d94dab94a66...`
guix_build.log.diff		`0374696142bfc83a...`

DrahtBot removed the label DrahtBot Guix build requested on Apr 19, 2025

fanquake commented at 12:13 pm on April 28, 2025: member

Guix Build:

 0db95c26122204cf686ae4d0a7d6f2e2f2cd30d7ea0dce425f28bf4fa1af9fa12  guix-build-77e553ab6a0a/output/aarch64-linux-gnu/SHA256SUMS.part
 13b5b3fd4b6ca8539e2ea5e50afdea37593f1f2e0c0b26851c20fc0505dd79a42  guix-build-77e553ab6a0a/output/aarch64-linux-gnu/bitcoin-77e553ab6a0a-aarch64-linux-gnu-debug.tar.gz
 24c6b3a3949cc70e3f1a82fa38ed5c204c54074a79b267d67c02439279099c2ff  guix-build-77e553ab6a0a/output/aarch64-linux-gnu/bitcoin-77e553ab6a0a-aarch64-linux-gnu.tar.gz
 3b9d2d30e3b5df7ad303c951176feb18a18c42e43c95a038128ca10669bb4a6a9  guix-build-77e553ab6a0a/output/arm-linux-gnueabihf/SHA256SUMS.part
 458cb80d16437acba0bbaa5df0a69521270dbc61b84a5b8284c18b4e96263b0a6  guix-build-77e553ab6a0a/output/arm-linux-gnueabihf/bitcoin-77e553ab6a0a-arm-linux-gnueabihf-debug.tar.gz
 5aa55ed2d8be3cfa760d373ab971ebf686c040785b0c64fd2e77779475a866165  guix-build-77e553ab6a0a/output/arm-linux-gnueabihf/bitcoin-77e553ab6a0a-arm-linux-gnueabihf.tar.gz
 65b217641817eda09edbf4efdc74b9c110defcb4bdd85c9d37c91b0721dcf7674  guix-build-77e553ab6a0a/output/arm64-apple-darwin/SHA256SUMS.part
 76453fb94f8c9015c080eda0d0eb3818b9c07e1073b7dae05758b7a0f9259aa15  guix-build-77e553ab6a0a/output/arm64-apple-darwin/bitcoin-77e553ab6a0a-arm64-apple-darwin-codesigning.tar.gz
 8ad867c154209ddb30b245f813a531406e55b0803ad100187f66ec22d8a8a8780  guix-build-77e553ab6a0a/output/arm64-apple-darwin/bitcoin-77e553ab6a0a-arm64-apple-darwin-unsigned.tar.gz
 92b176309d5e44bc7c8cc8d6a352d47c59699ce449e566653fefc661c1e72ecda  guix-build-77e553ab6a0a/output/arm64-apple-darwin/bitcoin-77e553ab6a0a-arm64-apple-darwin-unsigned.zip
10b433b9c2bd8d9258414edbd98f247771c2edcfa532eba8dc8bde10d93c98048b  guix-build-77e553ab6a0a/output/dist-archive/bitcoin-77e553ab6a0a.tar.gz
11356a8f1c01fd3ea760ed6e120814f3fe9b9a4ce69b7c22b504369a702d776ef3  guix-build-77e553ab6a0a/output/powerpc64-linux-gnu/SHA256SUMS.part
12fc42a3468634083cceaa0a606953e6199b0f0071ced469395bce98c763aee2a2  guix-build-77e553ab6a0a/output/powerpc64-linux-gnu/bitcoin-77e553ab6a0a-powerpc64-linux-gnu-debug.tar.gz
13f52e9149e7e792592c48d31f1b0ce319c5ee887077157df2c6a6665fbebc8112  guix-build-77e553ab6a0a/output/powerpc64-linux-gnu/bitcoin-77e553ab6a0a-powerpc64-linux-gnu.tar.gz
1439aefacf7be73c6dbf444532c3bc344ecebf9290cb6a80e1072bfb2e8eae6f0c  guix-build-77e553ab6a0a/output/riscv64-linux-gnu/SHA256SUMS.part
151c3ea57370a8e04dea9d86aac34af7e5ba118f2835431137e4475efeed5543c5  guix-build-77e553ab6a0a/output/riscv64-linux-gnu/bitcoin-77e553ab6a0a-riscv64-linux-gnu-debug.tar.gz
16fb63f9bd863e5305ae0b385a2c2b1d0dbb61ef872d3fda1d8ca12fed9e23455b  guix-build-77e553ab6a0a/output/riscv64-linux-gnu/bitcoin-77e553ab6a0a-riscv64-linux-gnu.tar.gz
1763da72119d2ff82095a4309bdbc92623067c0f8187be1ce56ba7ddbe9ff11960  guix-build-77e553ab6a0a/output/x86_64-apple-darwin/SHA256SUMS.part
18935697e705a322fec78048fb1df34d5fd00aa9353afce508a387f7b7958eb7a9  guix-build-77e553ab6a0a/output/x86_64-apple-darwin/bitcoin-77e553ab6a0a-x86_64-apple-darwin-codesigning.tar.gz
19a77aeff6f41b5d9b4278cdb1417b59520a6f09bef360063f3a69d3e213addf26  guix-build-77e553ab6a0a/output/x86_64-apple-darwin/bitcoin-77e553ab6a0a-x86_64-apple-darwin-unsigned.tar.gz
20df59e2ac7f5676a7753ffba9571e2a503d177171d80e6d2260f595645ce367bb  guix-build-77e553ab6a0a/output/x86_64-apple-darwin/bitcoin-77e553ab6a0a-x86_64-apple-darwin-unsigned.zip
21439ce4c9c108527d1c1f26f70b575e27d91321a41bad824bac5adff658bd8d29  guix-build-77e553ab6a0a/output/x86_64-linux-gnu/SHA256SUMS.part
22011a4bd59cac89991163b0e04bfdb2f75ca1f590cd7880bf46f1ee6522c69351  guix-build-77e553ab6a0a/output/x86_64-linux-gnu/bitcoin-77e553ab6a0a-x86_64-linux-gnu-debug.tar.gz
237f1e7dbf479e939e6ffb1cceb7fd80e9bca8a186be8e6b7cf4d99d468eedc8a8  guix-build-77e553ab6a0a/output/x86_64-linux-gnu/bitcoin-77e553ab6a0a-x86_64-linux-gnu.tar.gz
24aa9ceb2a5d639edf7a222810c2d4ffc5305cc274333153145606ff4ad77669bd  guix-build-77e553ab6a0a/output/x86_64-w64-mingw32/SHA256SUMS.part
25f75a42a14138aa927f9483f0d5a4b2d160c9ee61bfc2715b8551f8eace90a563  guix-build-77e553ab6a0a/output/x86_64-w64-mingw32/bitcoin-77e553ab6a0a-win64-codesigning.tar.gz
269bf895bd1f16d7b8524213090e770dc7bdb11a1803f2358613d34aab6ba5435a  guix-build-77e553ab6a0a/output/x86_64-w64-mingw32/bitcoin-77e553ab6a0a-win64-debug.zip
2705bff16e246264f47f29a0e1097ebcb666cbc34a07173dffff17061d1a2a2c08  guix-build-77e553ab6a0a/output/x86_64-w64-mingw32/bitcoin-77e553ab6a0a-win64-setup-unsigned.exe
28bd29ba767147ccd87e1c43209e3a80b357aedbe9fb508d70e506704d72697fbe  guix-build-77e553ab6a0a/output/x86_64-w64-mingw32/bitcoin-77e553ab6a0a-win64-unsigned.zip

fanquake merged this on Apr 28, 2025

fanquake closed this on Apr 28, 2025

hebasto referenced this in commit 8a08c00220 on Apr 28, 2025

build: Drop option to disable hardening. #32071

Code Coverage & Benchmarks

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]