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

build: Drop option to disable hardening. #32071

Code Coverage & Benchmarks

Reviews

Conflicts