-
Notifications
You must be signed in to change notification settings - Fork 38.7k
build: add -bind_at_load to macOS hardened LDFLAGS #17686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: add -bind_at_load to macOS hardened LDFLAGS #17686
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Gitian builds
|
|
Pinging @theuni |
|
Concept ACK Weren't you working on extending |
Yea I started working on a macOS version here: https://github.com/fanquake/core-review/blob/master/macos_dylib_check.py. I can integrate those changes, as well as extend to check for the bind flags as part of this PR. |
that'd be awesome! |
|
Concept ACK, would love to see the |
|
Currently on hold as we investigate some discrepancies with the macOS dynamic loader. |
This performs the same function as -Wl,-z,now, except for ld on macOS. You can check the binaries using otool -l, looking for the LC_DYLD_INFO_ONLY section. lazy_bind_off and lazy_bind_size should both be 0. man ld: -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily.
e576207 to
c78b123
Compare
theuni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c78b123.
c78b123 build: add -bind_at_load to hardened LDFLAGS (fanquake) Pull request description: This performs the same function as `-Wl,-z,now`, except for ld on macOS. You can check the binaries using `otool -l`, and looking for the `LC_DYLD_INFO_ONLY` section; `lazy_bind_off` and `lazy_bind_size` should both be 0. This seems to be the case with our current release binaries. However we can make the check, and applying the flag explicit in configure. man ld: ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` TODO: - [ ] Follow up with `MH_BINDATLOAD` flag. ACKs for top commit: theuni: ACK c78b123. Tree-SHA512: 12259558b84f7e3d75d6fcde63b517685e42b18fcf8e8cfcf347483c5ba089d3b4b6d330e7b7f61f83a328fe4d141b771e8e52ddee9cac6da87dfc073ab1183d
|
The macOS security and dylib checking changes are going to be PR'd shortly, as well as some additional ld flags error checking. |
c78b123 build: add -bind_at_load to hardened LDFLAGS (fanquake) Pull request description: This performs the same function as `-Wl,-z,now`, except for ld on macOS. You can check the binaries using `otool -l`, and looking for the `LC_DYLD_INFO_ONLY` section; `lazy_bind_off` and `lazy_bind_size` should both be 0. This seems to be the case with our current release binaries. However we can make the check, and applying the flag explicit in configure. man ld: ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` TODO: - [ ] Follow up with `MH_BINDATLOAD` flag. ACKs for top commit: theuni: ACK c78b123. Tree-SHA512: 12259558b84f7e3d75d6fcde63b517685e42b18fcf8e8cfcf347483c5ba089d3b4b6d330e7b7f61f83a328fe4d141b771e8e52ddee9cac6da87dfc073ab1183d
…k.py 5ca90f8 scripts: add MACHO lazy bindings check to security-check.py (fanquake) Pull request description: This is a slightly belated follow up to #17686 and some discussion with Cory. It's not entirely clear if we should make this change due to the way the macOS dynamic loader appears to work. However I'm opening this for some discussion. Also related to #17768. #### Issue: [`LD64`](https://opensource.apple.com/source/ld64/) doesn't set the [MH_BINDATLOAD](https://opensource.apple.com/source/xnu/xnu-6153.11.26/EXTERNAL_HEADERS/mach-o/loader.h.auto.html) bit in the header of MACHO executables, when building with `-bind_at_load`. This is in contradiction to the [documentation](https://opensource.apple.com/source/ld64/ld64-450.3/doc/man/man1/ld.1.auto.html): ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` The [`ld` in Apples cctools](https://opensource.apple.com/source/cctools/cctools-927.0.2/ld/layout.c.auto.html) does set the bit, however the [cctools-port](https://github.com/tpoechtrager/cctools-port/) that we use for release builds, bundles `LD64`. However; even if the linker hasn't set that bit, the dynamic loader ([`dyld`](https://opensource.apple.com/source/dyld/)) doesn't seem to ever check for it, and from what I understand, it looks at a different part of the header when determining whether to lazily load symbols. Note that our release binaries are currently working as expected, and no lazy loading occurs. #### Example: Using a small program, we can observe the behaviour of the dynamic loader. Conducted using: ```bash clang++ --version Apple clang version 11.0.0 (clang-1100.0.33.17) Target: x86_64-apple-darwin18.7.0 ld -v @(#)PROGRAM:ld PROJECT:ld64-530 BUILD 18:57:17 Dec 13 2019 LTO support using: LLVM version 11.0.0, (clang-1100.0.33.17) (static support for 23, runtime is 23) TAPI support using: Apple TAPI version 11.0.0 (tapi-1100.0.11) ``` ```cpp #include <iostream> int main() { std::cout << "Hello World!\n"; return 0; } ``` Compile and check the MACHO header: ```bash clang++ test.cpp -o test otool -vh test ... Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 16 1424 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK PIE # Run and dump dynamic loader bindings: DYLD_PRINT_BINDINGS=1 DYLD_PRINT_TO_FILE=no_bind.txt ./test Hello World! ``` Recompile with `-bind_at_load`. Note still no `BINDATLOAD` flag: ```bash clang++ test.cpp -o test -Wl,-bind_at_load otool -vh test Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 16 1424 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK PIE ... DYLD_PRINT_BINDINGS=1 DYLD_PRINT_TO_FILE=bind.txt ./test Hello World! ``` If we diff the outputs, you can see that `dyld` doesn't perform any lazy bindings when the binary is compiled with `-bind_at_load`, even if the `BINDATLOAD` flag is not set: ```diff @@ -1,11 +1,27 @@ +dyld: bind: test:0x103EDF030 = libc++.1.dylib:__ZNKSt3__16locale9use_facetERNS0_2idE, *0x103EDF030 = 0x7FFF70C9FA58 +dyld: bind: test:0x103EDF038 = libc++.1.dylib:__ZNKSt3__18ios_base6getlocEv, *0x103EDF038 = 0x7FFF70CA12C2 +dyld: bind: test:0x103EDF068 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryC1ERS3_, *0x103EDF068 = 0x7FFF70CA12B6 +dyld: bind: test:0x103EDF070 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryD1Ev, *0x103EDF070 = 0x7FFF70CA1528 +dyld: bind: test:0x103EDF080 = libc++.1.dylib:__ZNSt3__16localeD1Ev, *0x103EDF080 = 0x7FFF70C9FAE6 <trim> -dyld: lazy bind: test:0x10D4AC0C8 = libsystem_platform.dylib:_strlen, *0x10D4AC0C8 = 0x7FFF73C5C6E0 -dyld: lazy bind: test:0x10D4AC068 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryC1ERS3_, *0x10D4AC068 = 0x7FFF70CA12B6 -dyld: lazy bind: test:0x10D4AC038 = libc++.1.dylib:__ZNKSt3__18ios_base6getlocEv, *0x10D4AC038 = 0x7FFF70CA12C2 -dyld: lazy bind: test:0x10D4AC030 = libc++.1.dylib:__ZNKSt3__16locale9use_facetERNS0_2idE, *0x10D4AC030 = 0x7FFF70C9FA58 -dyld: lazy bind: test:0x10D4AC080 = libc++.1.dylib:__ZNSt3__16localeD1Ev, *0x10D4AC080 = 0x7FFF70C9FAE6 -dyld: lazy bind: test:0x10D4AC070 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryD1Ev, *0x10D4AC070 = 0x7FFF70CA1528 ``` Note: `dyld` also has a `DYLD_BIND_AT_LAUNCH=1` environment variable, that when set, will force any lazy bindings to be non-lazy: ```bash dyld: forced lazy bind: test:0x10BEC8068 = libc++.1.dylib:__ZNSt3__113basic_ostream ``` #### Thoughts: After looking at the dyld source, I can't find any checks for `MH_BINDATLOAD`. You can see the flags it does check for, such as MH_PIE or MH_BIND_TO_WEAK [here](https://opensource.apple.com/source/dyld/dyld-732.8/src/ImageLoaderMachO.cpp.auto.html). It seems that the lazy binding of any symbols depends on whether or not [lazy_bind_size](https://opensource.apple.com/source/xnu/xnu-6153.11.26/EXTERNAL_HEADERS/mach-o/loader.h.auto.html) from the `LC_DYLD_INFO_ONLY` load command is > 0. Which was mentioned in [#17686](#17686 (comment)). #### Changes: This PR is one of [Corys commits](theuni@7b6ba26), that I've rebased and modified to make build. I've also included an addition to the `security-check.py` script to check for the flag. However, given the above, I'm not entirely sure this patch is the correct approach. If the linker no-longer inserts it, and the dynamic loader doesn't look for it, there might be little benefit to setting it. Or, maybe this is an oversight from Apple and needs some upstream discussion. Looking for some thoughts / Concept ACK/NACK. One alternate approach we could take is to drop the patch and modify security-check.py to look for `lazy_bind_size` == 0 in the `LC_DYLD_INFO_ONLY` load command, using `otool -l`. ACKs for top commit: theuni: ACK 5ca90f8 Tree-SHA512: 444022ea9d19ed74dd06dc2ab3857a9c23fbc2f6475364e8552d761b712d684b3a7114d144f20de42328d1a99403b48667ba96885121392affb2e05b834b6e1c
…ty-check.py 5ca90f8 scripts: add MACHO lazy bindings check to security-check.py (fanquake) Pull request description: This is a slightly belated follow up to bitcoin#17686 and some discussion with Cory. It's not entirely clear if we should make this change due to the way the macOS dynamic loader appears to work. However I'm opening this for some discussion. Also related to bitcoin#17768. #### Issue: [`LD64`](https://opensource.apple.com/source/ld64/) doesn't set the [MH_BINDATLOAD](https://opensource.apple.com/source/xnu/xnu-6153.11.26/EXTERNAL_HEADERS/mach-o/loader.h.auto.html) bit in the header of MACHO executables, when building with `-bind_at_load`. This is in contradiction to the [documentation](https://opensource.apple.com/source/ld64/ld64-450.3/doc/man/man1/ld.1.auto.html): ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` The [`ld` in Apples cctools](https://opensource.apple.com/source/cctools/cctools-927.0.2/ld/layout.c.auto.html) does set the bit, however the [cctools-port](https://github.com/tpoechtrager/cctools-port/) that we use for release builds, bundles `LD64`. However; even if the linker hasn't set that bit, the dynamic loader ([`dyld`](https://opensource.apple.com/source/dyld/)) doesn't seem to ever check for it, and from what I understand, it looks at a different part of the header when determining whether to lazily load symbols. Note that our release binaries are currently working as expected, and no lazy loading occurs. #### Example: Using a small program, we can observe the behaviour of the dynamic loader. Conducted using: ```bash clang++ --version Apple clang version 11.0.0 (clang-1100.0.33.17) Target: x86_64-apple-darwin18.7.0 ld -v @(#)PROGRAM:ld PROJECT:ld64-530 BUILD 18:57:17 Dec 13 2019 LTO support using: LLVM version 11.0.0, (clang-1100.0.33.17) (static support for 23, runtime is 23) TAPI support using: Apple TAPI version 11.0.0 (tapi-1100.0.11) ``` ```cpp #include <iostream> int main() { std::cout << "Hello World!\n"; return 0; } ``` Compile and check the MACHO header: ```bash clang++ test.cpp -o test otool -vh test ... Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 16 1424 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK PIE # Run and dump dynamic loader bindings: DYLD_PRINT_BINDINGS=1 DYLD_PRINT_TO_FILE=no_bind.txt ./test Hello World! ``` Recompile with `-bind_at_load`. Note still no `BINDATLOAD` flag: ```bash clang++ test.cpp -o test -Wl,-bind_at_load otool -vh test Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 16 1424 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK PIE ... DYLD_PRINT_BINDINGS=1 DYLD_PRINT_TO_FILE=bind.txt ./test Hello World! ``` If we diff the outputs, you can see that `dyld` doesn't perform any lazy bindings when the binary is compiled with `-bind_at_load`, even if the `BINDATLOAD` flag is not set: ```diff @@ -1,11 +1,27 @@ +dyld: bind: test:0x103EDF030 = libc++.1.dylib:__ZNKSt3__16locale9use_facetERNS0_2idE, *0x103EDF030 = 0x7FFF70C9FA58 +dyld: bind: test:0x103EDF038 = libc++.1.dylib:__ZNKSt3__18ios_base6getlocEv, *0x103EDF038 = 0x7FFF70CA12C2 +dyld: bind: test:0x103EDF068 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryC1ERS3_, *0x103EDF068 = 0x7FFF70CA12B6 +dyld: bind: test:0x103EDF070 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryD1Ev, *0x103EDF070 = 0x7FFF70CA1528 +dyld: bind: test:0x103EDF080 = libc++.1.dylib:__ZNSt3__16localeD1Ev, *0x103EDF080 = 0x7FFF70C9FAE6 <trim> -dyld: lazy bind: test:0x10D4AC0C8 = libsystem_platform.dylib:_strlen, *0x10D4AC0C8 = 0x7FFF73C5C6E0 -dyld: lazy bind: test:0x10D4AC068 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryC1ERS3_, *0x10D4AC068 = 0x7FFF70CA12B6 -dyld: lazy bind: test:0x10D4AC038 = libc++.1.dylib:__ZNKSt3__18ios_base6getlocEv, *0x10D4AC038 = 0x7FFF70CA12C2 -dyld: lazy bind: test:0x10D4AC030 = libc++.1.dylib:__ZNKSt3__16locale9use_facetERNS0_2idE, *0x10D4AC030 = 0x7FFF70C9FA58 -dyld: lazy bind: test:0x10D4AC080 = libc++.1.dylib:__ZNSt3__16localeD1Ev, *0x10D4AC080 = 0x7FFF70C9FAE6 -dyld: lazy bind: test:0x10D4AC070 = libc++.1.dylib:__ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEE6sentryD1Ev, *0x10D4AC070 = 0x7FFF70CA1528 ``` Note: `dyld` also has a `DYLD_BIND_AT_LAUNCH=1` environment variable, that when set, will force any lazy bindings to be non-lazy: ```bash dyld: forced lazy bind: test:0x10BEC8068 = libc++.1.dylib:__ZNSt3__113basic_ostream ``` #### Thoughts: After looking at the dyld source, I can't find any checks for `MH_BINDATLOAD`. You can see the flags it does check for, such as MH_PIE or MH_BIND_TO_WEAK [here](https://opensource.apple.com/source/dyld/dyld-732.8/src/ImageLoaderMachO.cpp.auto.html). It seems that the lazy binding of any symbols depends on whether or not [lazy_bind_size](https://opensource.apple.com/source/xnu/xnu-6153.11.26/EXTERNAL_HEADERS/mach-o/loader.h.auto.html) from the `LC_DYLD_INFO_ONLY` load command is > 0. Which was mentioned in [bitcoin#17686](bitcoin#17686 (comment)). #### Changes: This PR is one of [Corys commits](theuni@7b6ba26), that I've rebased and modified to make build. I've also included an addition to the `security-check.py` script to check for the flag. However, given the above, I'm not entirely sure this patch is the correct approach. If the linker no-longer inserts it, and the dynamic loader doesn't look for it, there might be little benefit to setting it. Or, maybe this is an oversight from Apple and needs some upstream discussion. Looking for some thoughts / Concept ACK/NACK. One alternate approach we could take is to drop the patch and modify security-check.py to look for `lazy_bind_size` == 0 in the `LC_DYLD_INFO_ONLY` load command, using `otool -l`. ACKs for top commit: theuni: ACK 5ca90f8 Tree-SHA512: 444022ea9d19ed74dd06dc2ab3857a9c23fbc2f6475364e8552d761b712d684b3a7114d144f20de42328d1a99403b48667ba96885121392affb2e05b834b6e1c
c78b123 build: add -bind_at_load to hardened LDFLAGS (fanquake) Pull request description: This performs the same function as `-Wl,-z,now`, except for ld on macOS. You can check the binaries using `otool -l`, and looking for the `LC_DYLD_INFO_ONLY` section; `lazy_bind_off` and `lazy_bind_size` should both be 0. This seems to be the case with our current release binaries. However we can make the check, and applying the flag explicit in configure. man ld: ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` TODO: - [ ] Follow up with `MH_BINDATLOAD` flag. ACKs for top commit: theuni: ACK c78b123. Tree-SHA512: 12259558b84f7e3d75d6fcde63b517685e42b18fcf8e8cfcf347483c5ba089d3b4b6d330e7b7f61f83a328fe4d141b771e8e52ddee9cac6da87dfc073ab1183d
c78b123 build: add -bind_at_load to hardened LDFLAGS (fanquake) Pull request description: This performs the same function as `-Wl,-z,now`, except for ld on macOS. You can check the binaries using `otool -l`, and looking for the `LC_DYLD_INFO_ONLY` section; `lazy_bind_off` and `lazy_bind_size` should both be 0. This seems to be the case with our current release binaries. However we can make the check, and applying the flag explicit in configure. man ld: ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` TODO: - [ ] Follow up with `MH_BINDATLOAD` flag. ACKs for top commit: theuni: ACK c78b123. Tree-SHA512: 12259558b84f7e3d75d6fcde63b517685e42b18fcf8e8cfcf347483c5ba089d3b4b6d330e7b7f61f83a328fe4d141b771e8e52ddee9cac6da87dfc073ab1183d
c78b123 build: add -bind_at_load to hardened LDFLAGS (fanquake) Pull request description: This performs the same function as `-Wl,-z,now`, except for ld on macOS. You can check the binaries using `otool -l`, and looking for the `LC_DYLD_INFO_ONLY` section; `lazy_bind_off` and `lazy_bind_size` should both be 0. This seems to be the case with our current release binaries. However we can make the check, and applying the flag explicit in configure. man ld: ```bash -bind_at_load Sets a bit in the mach header of the resulting binary which tells dyld to bind all symbols when the binary is loaded, rather than lazily. ``` TODO: - [ ] Follow up with `MH_BINDATLOAD` flag. ACKs for top commit: theuni: ACK c78b123. Tree-SHA512: 12259558b84f7e3d75d6fcde63b517685e42b18fcf8e8cfcf347483c5ba089d3b4b6d330e7b7f61f83a328fe4d141b771e8e52ddee9cac6da87dfc073ab1183d
This performs the same function as
-Wl,-z,now, except for ld on macOS.You can check the binaries using
otool -l, and looking for theLC_DYLD_INFO_ONLYsection;lazy_bind_offandlazy_bind_sizeshould both be 0.This seems to be the case with our current release binaries. However we can make the check, and applying the flag explicit in configure.
man ld:
TODO:
MH_BINDATLOADflag.