depends: harden libevent #27118

fanquake · 2023-02-17T12:14:42Z

Use FORTIFY_SOURCE=3 when building libevent in depends. I've upstreamed a change to switch libevent from using =2 to =3 as well: libevent/libevent#1418.

Solves half of #27038, by giving us some fortified funcs in bitcoin-cli.

DrahtBot · 2023-02-17T12:14:45Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan

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:

#24123 (build: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix) 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.

fanquake · 2023-02-17T14:33:36Z

Changed the approach here, from using libevents own hardening option (might send some fixes upstream), and a patch, to just using FORTIFY_SOURCE directly. That gets as what we want at the moment, without other potential side effects.

theuni · 2023-02-17T15:20:11Z

Do you have a before/after count for hardened functions?

fanquake · 2023-02-17T15:39:35Z

Do you have a before/after count for hardened functions?

Using GCC 13 and glibc 2.37 (with only this branch):

# master
nm -C src/bitcoind | grep _chk
                 U __fprintf_chk@GLIBC_2.17
                 U __memcpy_chk@GLIBC_2.17
                 U __snprintf_chk@GLIBC_2.17
                 U __stack_chk_fail@GLIBC_2.17
                 U __stack_chk_guard@GLIBC_2.17
                 U __vsnprintf_chk@GLIBC_2.17
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
32

# this branch
nm -C src/bitcoind | grep _chk
                 U __fdelt_chk@GLIBC_2.17
                 U __fprintf_chk@GLIBC_2.17
                 U __memcpy_chk@GLIBC_2.17
                 U __memmove_chk@GLIBC_2.17
                 U __memset_chk@GLIBC_2.17
                 U __snprintf_chk@GLIBC_2.17
                 U __stack_chk_fail@GLIBC_2.17
                 U __stack_chk_guard@GLIBC_2.17
                 U __vsnprintf_chk@GLIBC_2.17
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
54

If I combine with our own use of FORTIFY_SOURCE=3:

nm -C src/bitcoind | grep _chk
                 U __fdelt_chk@GLIBC_2.17
                 U __fprintf_chk@GLIBC_2.17
                 U __memcpy_chk@GLIBC_2.17
                 U __memmove_chk@GLIBC_2.17
                 U __memset_chk@GLIBC_2.17
                 U __snprintf_chk@GLIBC_2.17
                 U __stack_chk_fail@GLIBC_2.17
                 U __stack_chk_guard@GLIBC_2.17
                 U __vsnprintf_chk@GLIBC_2.17
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
81

hebasto · 2023-02-17T17:04:03Z

Changed the approach here, from using libevents own hardening option

The previous 974e44c0a0e692e1e11e7c067699db94f55ce464 commit, being combined with the current 778e34e8625cc83d0e5a93493c71f01712bef81d one, should work for older compilers as well, no?

fanquake · 2023-02-17T17:22:20Z

should work for older compilers as well, no?

I changed the approach because I don't want us to use the gcc-hardening option.

sedited

ACK 778e34e8625cc83d0e5a93493c71f01712bef81d

fanquake · 2023-02-20T16:37:43Z

Rebased on top of #27027.

sedited · 2023-02-21T07:49:02Z

ACK ff4a73a

ff4a73a depends: use FORTIFY_SOURCE=3 with libevent (fanquake) Pull request description: Use `FORTIFY_SOURCE=3` when building libevent in depends. I've upstreamed a change to switch libevent from using =2 to =3 as well: libevent/libevent#1418. Solves half of bitcoin#27038, by giving us some fortified funcs in `bitcoin-cli`. ACKs for top commit: TheCharlatan: ACK ff4a73a Tree-SHA512: eaf692ec92b288f0cb524c011fc81529f58efa4c43d418a7b3ae7108eba2bccba708a81a28ac6d063267be80ca615637c6e3fccc02497d7367af2eaae0e8d812

hebasto · 2023-03-31T21:03:59Z

This PR introduced a regression when building with depends and disabled hardening:

$ make -C depends HOST=x86_64-w64-mingw32 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 NO_USDT=1
$ ./configure --disable-hardening CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
$ make
...
/usr/bin/x86_64-w64-mingw32-ld: /home/hebasto/git/gui/depends/x86_64-w64-mingw32/lib/libevent.a(evutil.o):evutil.c:(.text+0x2975): undefined reference to `__memcpy_chk'
/usr/bin/x86_64-w64-mingw32-ld: /home/hebasto/git/gui/depends/x86_64-w64-mingw32/lib/libevent.a(evutil.o):evutil.c:(.text+0x29d6): undefined reference to `__memcpy_chk'
/usr/bin/x86_64-w64-mingw32-ld: /home/hebasto/git/gui/depends/x86_64-w64-mingw32/lib/libevent.a(evutil.o):evutil.c:(.text+0x3041): undefined reference to `__strcat_chk'
/usr/bin/x86_64-w64-mingw32-ld: /home/hebasto/git/gui/depends/x86_64-w64-mingw32/lib/libevent.a(evutil.o):evutil.c:(.text+0x3052): undefined reference to `__strcat_chk'
/usr/bin/x86_64-w64-mingw32-ld: /home/hebasto/git/gui/depends/x86_64-w64-mingw32/lib/libevent.a(http.o):http.c:(.text+0x6e6): undefined reference to `__memcpy_chk'
...

fanquake · 2023-04-03T13:19:36Z

This PR introduced a regression when building with depends and disabled hardening:

Followup for discussion in 27406.

Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link libssp for Windows builds, they now fail (after bitcoin#27118), if building with depends, and configuring with --disable-hardening. See: bitcoin#27118 (comment). This change would add a depends opiton such that, if someone wants to build with, for windows, without hardening, they can do so. This may also be useful when building for debugging.

436df1e depends: add NO_HARDEN option (fanquake) Pull request description: Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link `libssp` for Windows builds, they now fail (after #27118), if building with depends, and configuring with `--disable-hardening` (Windows is the odd build out here). See: bitcoin/bitcoin#27118 (comment). This change would add a depends option such that, if someone wants to build with depends, for Windows, without hardening, they can do so. This may also be useful when building for debugging. ACKs for top commit: hebasto: re-ACK 436df1e Tree-SHA512: 5a3ef5ec87b10a5ad0a284201988ce94789451735c7c7e20d337f7232955b0b9a0addab1c3b5725755f00d8ce6741aa9c8cb5e3d48d926515b7dde46acdbcaa0

436df1e depends: add NO_HARDEN option (fanquake) Pull request description: Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link `libssp` for Windows builds, they now fail (after bitcoin#27118), if building with depends, and configuring with `--disable-hardening` (Windows is the odd build out here). See: bitcoin#27118 (comment). This change would add a depends option such that, if someone wants to build with depends, for Windows, without hardening, they can do so. This may also be useful when building for debugging. ACKs for top commit: hebasto: re-ACK 436df1e Tree-SHA512: 5a3ef5ec87b10a5ad0a284201988ce94789451735c7c7e20d337f7232955b0b9a0addab1c3b5725755f00d8ce6741aa9c8cb5e3d48d926515b7dde46acdbcaa0

Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link libssp for Windows builds, they now fail (after #27118), if building with depends, and configuring with --disable-hardening. See: bitcoin/bitcoin#27118 (comment). This change would add a depends opiton such that, if someone wants to build with, for windows, without hardening, they can do so. This may also be useful when building for debugging.

Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link libssp for Windows builds, they now fail (after bitcoin#27118), if building with depends, and configuring with --disable-hardening. See: bitcoin#27118 (comment). This change would add a depends opiton such that, if someone wants to build with, for windows, without hardening, they can do so. This may also be useful when building for debugging.

Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link libssp for Windows builds, they now fail (after #27118), if building with depends, and configuring with --disable-hardening. See: bitcoin/bitcoin#27118 (comment). This change would add a depends opiton such that, if someone wants to build with, for windows, without hardening, they can do so. This may also be useful when building for debugging.

DrahtBot added the Build system label Feb 17, 2023

fanquake force-pushed the harden_libevent branch from e8cb6e6 to 778e34e Compare February 17, 2023 14:32

fanquake mentioned this pull request Feb 17, 2023

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

Merged

theuni mentioned this pull request Feb 17, 2023

build: use _FORTIFY_SOURCE=3 #27027

Merged

DrahtBot mentioned this pull request Feb 17, 2023

guix: Pointer Authentication and Branch Target Identification for aarch64 Linux #24123

Closed

sedited approved these changes Feb 20, 2023

View reviewed changes

depends: use FORTIFY_SOURCE=3 with libevent

ff4a73a

fanquake force-pushed the harden_libevent branch from 778e34e to ff4a73a Compare February 20, 2023 16:37

fanquake requested a review from theuni February 22, 2023 18:12

fanquake merged commit e60a58f into bitcoin:master Feb 28, 2023

fanquake deleted the harden_libevent branch February 28, 2023 10:31

hebasto mentioned this pull request Mar 31, 2023

Rebased cmake-staging branch (post PR#10) hebasto/bitcoin#11

Closed

fanquake mentioned this pull request Apr 3, 2023

depends: add NO_HARDEN= option #27406

Merged

bitcoin locked and limited conversation to collaborators Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

depends: harden libevent #27118

depends: harden libevent #27118

Uh oh!

fanquake commented Feb 17, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Feb 17, 2023 •

edited

Loading

Uh oh!

fanquake commented Feb 17, 2023

Uh oh!

theuni commented Feb 17, 2023

Uh oh!

fanquake commented Feb 17, 2023 •

edited

Loading

Uh oh!

hebasto commented Feb 17, 2023

Uh oh!

fanquake commented Feb 17, 2023

Uh oh!

sedited left a comment

Uh oh!

fanquake commented Feb 20, 2023

Uh oh!

sedited commented Feb 21, 2023

Uh oh!

hebasto commented Mar 31, 2023

Uh oh!

fanquake commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

depends: harden libevent #27118

depends: harden libevent #27118

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Feb 17, 2023

Uh oh!

theuni commented Feb 17, 2023

Uh oh!

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

Uh oh!

Uh oh!

hebasto commented Feb 17, 2023

Uh oh!

fanquake commented Feb 17, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 20, 2023

Uh oh!

sedited commented Feb 21, 2023

Uh oh!

hebasto commented Mar 31, 2023

Uh oh!

fanquake commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fanquake commented Feb 17, 2023 •

edited

Loading

DrahtBot commented Feb 17, 2023 •

edited

Loading

fanquake commented Feb 17, 2023 •

edited

Loading