scripts: add PE .reloc section check to security-check.py #18629
Conversation
|
Concept ACK. |
|
When I was reviewing the debian patches for inclusion in Guix, I did come across
I believe that's true 😄, with the recent upstreaming of guix:b066c25026
Perhaps @skitt can provide more details as to why default PE protections were disabled for binutils 2.34 in |
|
w/re the actual change, won't this break Gitian builds right now since they are on binutils 2.30? |
|
Previous releases of So I decided that it was better to advertise what we really do: by default, the cross-compiler setup produces binaries which don’t support the various hardening flags available in Windows. Projects which know what they’re doing can pass the relevant flags and get the improved security; in other cases, security analysis tools will flag binaries appropriately, and hopefully no one will be fooled. If I (or someone else) can come up with a meaningful set of defaults, I’ll be more than happy to change them again. |
Ah yes that makes sense, the
Hmmm, this could be troubling... We'll do some testing ourselves, but if you have a vague direction in which to point us that would be very much appreciated! |
I don’t remember anything particularly worrying when using either the upstream defaults or explicit options chosen appropriately; issues arose when changing the defaults, usually with binaries ending up with |
That's very reassuring, we'll be explicitly choosing the options then. Thank you so very much for you help! |
Gitian builds
|
|
Thanks for the comments @skitt. We've got some more to follow up on here. Just noting that this also means our test-security-check.py script will fail to run on bullseye, due to the removal of the bitcoin/contrib/devtools# python3 test-security-check.py TestSecurityChecks.test_PE
/usr/bin/x86_64-w64-mingw32-ld: unrecognized option '--no-nxcompat'
/usr/bin/x86_64-w64-mingw32-ld: use the --help option for usage information
collect2: error: ld returned 1 exit status
E
======================================================================
ERROR: test_PE (__main__.TestSecurityChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
File "test-security-check.py", line 52, in test_PE
self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va']),
File "test-security-check.py", line 23, in call_security_check
subprocess.check_call([cc,source,'-o',executable] + options)
File "/usr/lib/python3.8/subprocess.py", line 364, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['x86_64-w64-mingw32-gcc', 'test1.c', '-o', 'test1.exe', '-Wl,--no-nxcompat', '-Wl,--no-dynamicbase', '-Wl,--no-high-entropy-va']' returned non-zero exit status 1.
----------------------------------------------------------------------
Ran 1 test in 0.033s
FAILED (errors=1) |
Good point, I’ll add those options back. |
|
I've opened #18702 which will address the gitian failure here.
Thanks. Looks like this was done in: https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/6d954f9b5f1a5f9e46cc4770284d9bcd3680c2c7. |
|
Concept ACK: ASLR reasons :) |
You’re welcome! I was waiting until the package migrates to testing before mentioning it here. Unfortunately the |
804f390 to
1e8a7fc
Compare
|
I've cherry-picked #18702 into here. Queued up some new builds as well. |
315a4d3 build: fix ASLR for bitcoin-cli on Windows (fanquake) Pull request description: ASLR is not currently working for the `bitcoin-cli.exe` binary. This is due to it not having a .reloc section, which is stripped by default by the mingw-w64 ld we use for gitian builds. A good summary of issues with ld and mingw-w64 is available in this thread: https://sourceware.org/bugzilla/show_bug.cgi?id=19011. All other Windows binaries that we distribute (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-tx and test_bitcoin) do not suffer this issue, and currently having working ASLR. This is due to them exporting (inadvertent or not) libsecp256k1 symbols, and, as a result, the .reloc section is not stripped by ld. This change is a temporary workaround, also the same one described here: https://www.kb.cert.org/vuls/id/307144/, that causes main() to be exported. Exporting a symbol will mean that the .reloc section is not stripped, and ASLR will function correctly. Ultimately, this will be fixed by using a newer version of binutils (that has this [change](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0)). Whether that's through bumping our gitian distro, or Guix. Related to #18629, which has a bunch of additional information in the PR description. If you would like to verify whether or not ASLR is indeed working, with or without this change. One easy way to check is using a tool like [VMMap](https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap). Here are the memory mappings for the 0.20.0rc1 `bitcoind.exe` and `bitcoin-cli.exe` binaries. You'll notice that over machine restarts, even though the image is marked `(ASLR)` (which I assume may be due to the header bit being set), no ASLR is actually occuring for `bitcoin-cli.exe`: #### bitcoind.exe    #### bitcoin-cli.exe    ACKs for top commit: dongcarl: ACK 315a4d3 laanwj: ACK 315a4d3 Tree-SHA512: 95f4dc15420ed9bcdeacb763e11c3c7e563eec594a172746fa0346c13f97db3a8769357dffc89fea1e57ae67133f337b1013a73b584662f5b6c4d251ca20a2b1
|
#18702 was merged so the commit can be removed from here again (sorry). |
1e8a7fc to
3e38023
Compare
No worries! I've rebased on master. |
315a4d3 build: fix ASLR for bitcoin-cli on Windows (fanquake) Pull request description: ASLR is not currently working for the `bitcoin-cli.exe` binary. This is due to it not having a .reloc section, which is stripped by default by the mingw-w64 ld we use for gitian builds. A good summary of issues with ld and mingw-w64 is available in this thread: https://sourceware.org/bugzilla/show_bug.cgi?id=19011. All other Windows binaries that we distribute (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-tx and test_bitcoin) do not suffer this issue, and currently having working ASLR. This is due to them exporting (inadvertent or not) libsecp256k1 symbols, and, as a result, the .reloc section is not stripped by ld. This change is a temporary workaround, also the same one described here: https://www.kb.cert.org/vuls/id/307144/, that causes main() to be exported. Exporting a symbol will mean that the .reloc section is not stripped, and ASLR will function correctly. Ultimately, this will be fixed by using a newer version of binutils (that has this [change](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0)). Whether that's through bumping our gitian distro, or Guix. Related to bitcoin#18629, which has a bunch of additional information in the PR description. If you would like to verify whether or not ASLR is indeed working, with or without this change. One easy way to check is using a tool like [VMMap](https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap). Here are the memory mappings for the 0.20.0rc1 `bitcoind.exe` and `bitcoin-cli.exe` binaries. You'll notice that over machine restarts, even though the image is marked `(ASLR)` (which I assume may be due to the header bit being set), no ASLR is actually occuring for `bitcoin-cli.exe`: #### bitcoind.exe    #### bitcoin-cli.exe    ACKs for top commit: dongcarl: ACK 315a4d3 laanwj: ACK 315a4d3 Tree-SHA512: 95f4dc15420ed9bcdeacb763e11c3c7e563eec594a172746fa0346c13f97db3a8769357dffc89fea1e57ae67133f337b1013a73b584662f5b6c4d251ca20a2b1
Guix builds
|
|
ACK 3e38023 |
Gitian builds
|
…-check.py 3e38023 scripts: add PE .reloc section check to security-check.py (fanquake) Pull request description: The `ld` in binutils has historically had a few issues with PE binaries, there's a good summary in this [thread](https://sourceware.org/bugzilla/show_bug.cgi?id=19011). One issue in particular was `ld` stripping the `.reloc` section out of PE binaries, even though it's required for functioning ASLR. This was [reported by a Tor developer in 2014](https://sourceware.org/bugzilla/show_bug.cgi?id=17321) and they have been patching their [own binutils](https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/binutils) ever since. However their patch only made it into binutils at the [start of this year](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0). It adds an `--enable-reloc-section` flag, which is turned on by default if you are using `--dynamic-base`. In the mean time this issue has also been worked around by other projects, such as FFmpeg, see [this commit](TheRyuu/FFmpeg@91b668a). I have checked our recent supported Windows release binaries, and they do contain a `.reloc` section. From what I understand, we are using all the right compile/linker flags, including `-pie` & `-fPIE`, and have never run into the crashing/entrypoint issues that other projects might have seen. One other thing worth noting here, it how Debian/Ubuntu patch the binutils that they distribute, because that's what we end up using in our gitian builds. In the binutils-mingw-w64 in Bionic (18.04), which we currently use in gitian, PE hardening options/security flags are enabled by default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8ubuntu1/changelog) and the [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/452b3013b8280cbe35eaeb166a43621b88d5f8b7). However in Focal (20.04), this has now been reversed. PE hardening options are no-longer the default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8.8/changelog) and [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/7bd8b2fbc242a8c2fc2217f29fd61f94d3babf6f), which cites same .reloc issue mentioned here. Given that we explicitly specify/opt-in to everything that we want to use, the defaults aren't necessarily an issue for us. However I think it highlights the importance of continuing to be explicit about what we want, and not falling-back or relying on upstream. This was also prompted by the possibility of us doing link time garbage collection, see bitcoin#18579 & bitcoin#18605. It seemed some sanity checks would be worthwhile in-case the linker goes haywire while garbage collecting. I think Guix is going to bring great benefits when dealing with these kinds of issues. Carl you might have something to say in that regard. ACKs for top commit: dongcarl: ACK 3e38023 Tree-SHA512: af14d63bdb334bde548dd7de3e0946556b7e2598d817b56eb4e75b3f56c705c26aa85dd9783134c4b6a7aeb7cb4de567eed996e94d533d31511f57ed332287da
The
ldin binutils has historically had a few issues with PE binaries, there's a good summary in this thread.One issue in particular was
ldstripping the.relocsection out of PE binaries, even though it's required for functioning ASLR. This was reported by a Tor developer in 2014 and they have been patching their own binutils ever since. However their patch only made it into binutils at the start of this year. It adds an--enable-reloc-sectionflag, which is turned on by default if you are using--dynamic-base. In the mean time this issue has also been worked around by other projects, such as FFmpeg, see this commit.I have checked our recent supported Windows release binaries, and they do contain a
.relocsection. From what I understand, we are using all the right compile/linker flags, including-pie&-fPIE, and have never run into the crashing/entrypoint issues that other projects might have seen.One other thing worth noting here, it how Debian/Ubuntu patch the binutils that they distribute, because that's what we end up using in our gitian builds.
In the binutils-mingw-w64 in Bionic (18.04), which we currently use in gitian, PE hardening options/security flags are enabled by default. See the changelog and the relevant commit.
However in Focal (20.04), this has now been reversed. PE hardening options are no-longer the default. See the changelog and relevant commit, which cites same .reloc issue mentioned here.
Given that we explicitly specify/opt-in to everything that we want to use, the defaults aren't necessarily an issue for us. However I think it highlights the importance of continuing to be explicit about what we want, and not falling-back or relying on upstream.
This was also prompted by the possibility of us doing link time garbage collection, see #18579 & #18605. It seemed some sanity checks would be worthwhile in-case the linker goes haywire while garbage collecting.
I think Guix is going to bring great benefits when dealing with these kinds of issues. Carl you might have something to say in that regard.