scripts: add PE .reloc section check to security-check.py

fanquake commented at 4:12 am on April 14, 2020: member

The ld in binutils has historically had a few issues with PE binaries, there’s a good summary in this thread.

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 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-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.

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 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.

fanquake added the label Windows on Apr 14, 2020

fanquake added the label Scripts and tools on Apr 14, 2020

fanquake added the label Needs gitian build on Apr 14, 2020

fanquake requested review from dongcarl on Apr 14, 2020

laanwj commented at 6:13 am on April 14, 2020: member

Concept ACK. Can we finally close #8248 after this?

dongcarl commented at 2:52 pm on April 14, 2020: member

When I was reviewing the debian patches for inclusion in Guix, I did come across default-secure-pe-flags.patch, but chose not to include it, since it mostly just changed some defaults, and because of the recent removal fanquake mentioned.

<SHILL>

I think Guix is going to bring great benefits when dealing with these kinds of issues.

I believe that’s true :smile:, with the recent upstreaming of guix:b066c25026 , we can use the PACKAGE-WITH-EXTRA-PATCHES procedure in our manifest.scm to apply custom patches to anything that constitutes part of our Guix build environment (binutils, gcc, etc.).

</SHILL>

Perhaps @skitt can provide more details as to why default PE protections were disabled for binutils 2.34 in 452b3013, as it seems that that’s the first version of binutils to include dc9bd8c92a (I ran git tag --contains dc9bd8c92a in the binutils-gdb repo to check) and would therefore actually benefit from the default PE protections. Perhaps he knows something we don’t.

dongcarl commented at 2:55 pm on April 14, 2020: member

w/re the actual change, won’t this break Gitian builds right now since they are on binutils 2.30?

skitt commented at 3:08 pm on April 14, 2020: none

Previous releases of binutils-mingw-w64 in Debian enabled hardening flags by default, but as mentioned elsewhere, this is actually misleading since the flags really require relocations, and those were dropped. When --enable-reloc-section was added, I experimented with enabling that by default, along with all the other flags; but enabling this by default breaks ld in a number of scenarios. In particular it breaks many, many test-cases in the test-suite, but it also causes some problems in real-world scenarios (I don’t have the details to hand).

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.

dongcarl commented at 4:55 pm on April 14, 2020: member

@skitt

enabling this by default breaks ld in a number of scenarios. In particular it breaks many, many test-cases in the test-suite

Ah yes that makes sense, the binutils test suite must expect the default defaults, not the modified/patched defaults.

also causes some problems in real-world scenarios (I don’t have the details to hand)

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!

skitt commented at 5:10 pm on April 14, 2020: none

also causes some problems in real-world scenarios (I don’t have the details to hand)

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 .reloc sections when they (or rather, their build system) didn’t expect them for whatever reason.

dongcarl commented at 5:29 pm on April 14, 2020: member

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 .reloc sections when they (or rather, their build system) didn’t expect them for whatever reason.

That’s very reassuring, we’ll be explicitly choosing the options then. Thank you so very much for you help!

DrahtBot commented at 8:32 pm on April 14, 2020: member

Gitian builds

File	commit 6110ae8326c74704c9e105deca725f2411395969(master)	commit 1095b286b6d119079861ce5df8f444b4decdedbb(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`3331ace5139c1d7f...`	`0d5d1660228fee44...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`907f7a0437496161...`	`324e3d6adbcd12ea...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`f4f849708e920f0a...`	`896f196fedf94212...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`9b2ac36483233976...`	`cc10d2060ef32d10...`
bitcoin-0.20.99-osx-unsigned.dmg	`c361c2f6778ed11c...`	`640bf619c8a5ab25...`
bitcoin-0.20.99-osx64.tar.gz	`c6c1cfe2b5c9b6d0...`	`0aabbba6e65d0f95...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`e305f3090438754d...`	`2a63f7cc29b5c182...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`91c950b7acd4afb2...`	`472f6493c3ebb4b4...`
bitcoin-0.20.99-win64-debug.zip	`d28a2177ac3ed76a...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`c11e0cd9d599dc05...`
bitcoin-0.20.99-win64.zip	`f72a9bebc7fa841c...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`b686db1a7a5e7a3c...`	`396cbdd105525adf...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`bd5dc4697f2f11b7...`	`dd7c1101a3a12785...`
bitcoin-0.20.99.tar.gz	`b95bbf5cc044d937...`	`c25d83172d72a79c...`
bitcoin-core-linux-0.21-res.yml	`86db19c2a7ded867...`	`c1a3aedb25a83b1a...`
bitcoin-core-osx-0.21-res.yml	`cb79ced4115d6598...`	`20f304af9d467537...`
bitcoin-core-win-0.21-res.yml	`9fabd93bbfe4f3eb...`
linux-build.log	`933154247a73aa08...`	`5b615b27b0f14a0f...`
osx-build.log	`e3689679a45483aa...`	`a3689ac1beafa354...`
win-build.log	`b0413c34055279a4...`	`dac699e01c99a82f...`
bitcoin-core-linux-0.21-res.yml.diff		`40e0e8882ef4c367...`
bitcoin-core-osx-0.21-res.yml.diff		`956d51e66d81afa8...`
linux-build.log.diff		`8e82da7d297d99dc...`
osx-build.log.diff		`e45f92e5afde8c3e...`
win-build.log.diff		`fa9cfdd29bb5b226...`

DrahtBot removed the label Needs gitian build on Apr 14, 2020

fanquake commented at 5:25 am on April 15, 2020: member

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 --no-* options from ld in the mingw-w64 binutils.

 0bitcoin/contrib/devtools# python3 test-security-check.py TestSecurityChecks.test_PE
 1/usr/bin/x86_64-w64-mingw32-ld: unrecognized option '--no-nxcompat'
 2/usr/bin/x86_64-w64-mingw32-ld: use the --help option for usage information
 3collect2: error: ld returned 1 exit status
 4E
 5======================================================================
 6ERROR: test_PE (__main__.TestSecurityChecks)
 7----------------------------------------------------------------------
 8Traceback (most recent call last):
 9  File "test-security-check.py", line 52, in test_PE
10    self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va']),
11  File "test-security-check.py", line 23, in call_security_check
12    subprocess.check_call([cc,source,'-o',executable] + options)
13  File "/usr/lib/python3.8/subprocess.py", line 364, in check_call
14    raise CalledProcessError(retcode, cmd)
15subprocess.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.
16----------------------------------------------------------------------
17Ran 1 test in 0.033s
18FAILED (errors=1)

fanquake added the label Waiting for author on Apr 15, 2020

skitt commented at 7:06 am on April 15, 2020: none

Just noting that this also means our test-security-check.py script will fail to run on bullseye, due to the removal of the --no-* options from ld in the mingw-w64 binutils.

Good point, I’ll add those options back.

fanquake commented at 4:01 am on April 19, 2020: member

I’ve opened #18702 which will address the gitian failure here.

Good point, I’ll add those options back.

Thanks. Looks like this was done in: https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/6d954f9b5f1a5f9e46cc4770284d9bcd3680c2c7.

practicalswift commented at 10:54 am on April 20, 2020: contributor

Concept ACK: ASLR reasons :)

skitt commented at 11:47 am on April 20, 2020: none

Good point, I’ll add those options back.

Thanks. Looks like this was done in: https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/6d954f9b5f1a5f9e46cc4770284d9bcd3680c2c7.

You’re welcome! I was waiting until the package migrates to testing before mentioning it here. Unfortunately the binutils migration is going to take a bit longer than usual, and binutils-mingw-w64 is tied into that.

fanquake force-pushed on Apr 22, 2020

fanquake removed the label Waiting for author on Apr 22, 2020

fanquake added the label Needs gitian build on Apr 22, 2020

fanquake added the label Needs Guix build on Apr 22, 2020

fanquake commented at 8:19 am on April 22, 2020: member

I’ve cherry-picked #18702 into here. Queued up some new builds as well.

laanwj referenced this in commit 5dcb061589 on Apr 22, 2020

laanwj commented at 1:19 pm on April 22, 2020: member

#18702 was merged so the commit can be removed from here again (sorry).

scripts: add PE .reloc section check to security-check.py 3e38023af7

fanquake force-pushed on Apr 23, 2020

fanquake commented at 0:42 am on April 23, 2020: member

#18702 was merged so the commit can be removed from here again (sorry).

No worries! I’ve rebased on master.

sidhujag referenced this in commit b1034e7ce6 on Apr 23, 2020

DrahtBot commented at 7:43 am on April 26, 2020: member

Guix builds

File	commit 3e7c118d6555c740601597522246e9c6781a2348(master)	commit 877d4b4cb39eb671be49a1a63ab958691fe662d2(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`ad13235a357c4ba6...`	`db1692cde3aac7eb...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`6a16bed80960f2a7...`	`1b65b1a3131e0638...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`dd87966e706e3a44...`	`0f9b77ff4921478e...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`3ad6fefa96c8f445...`	`d4d1f34a096c19d7...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`5b808c7813967af0...`	`7588b9d66ef74554...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`ccce11ef1b6ba1d0...`	`d48602b452f8bd74...`
bitcoin-0.20.99-win-unsigned.tar.gz	`995669482100f4c5...`	`3917ae97a9d6afb6...`
bitcoin-0.20.99-win64-debug.zip	`88fb41df75aa79db...`	`c139c337f8a180b1...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`947b951f28333937...`	`947b951f28333937...`
bitcoin-0.20.99-win64.zip	`d047174cccc07480...`	`139aae2c1c2fcd33...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`86d1728cc070cc6a...`	`99029df1b72c3914...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`f4ec71fd3745086b...`	`d33b784dac0a5850...`
bitcoin-0.20.99.tar.gz	`9591f5c737b44206...`	`b1edaf9212ae4122...`
guix_build.log	`f4776f9f24609031...`	`e74ee8486254160c...`
guix_build.log.diff		`06ea1c42cbeb9e08...`

DrahtBot removed the label Needs Guix build on Apr 26, 2020

dongcarl commented at 12:00 pm on April 27, 2020: member

ACK 3e38023af724a76972d39cbccfb0bba4c54a0323

DrahtBot commented at 12:32 pm on April 27, 2020: member

Gitian builds

File	commit 65276c7737176a5269b052ceae78dbb44b216bf4(master)	commit fe47be6da3ad056dbcc5f2db4c17933221eeacd5(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`673ecc74f832f550...`	`76dadf67c8e47dc2...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`8cacd39d9032cfa0...`	`18697b407ac596c2...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`f33c821af966c7cc...`	`ab1042944b218597...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`1610dfc4ad6edcc8...`	`3ac98f77e2f569eb...`
bitcoin-0.20.99-osx-unsigned.dmg	`d9b58fd52650821b...`	`45a3eaefbfa465d6...`
bitcoin-0.20.99-osx64.tar.gz	`2dd8003da396d8ff...`	`7e2670b9788ef783...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`6ab423f10d0576b8...`	`2492b3bb985b27e0...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`e2c3d277f9188905...`	`88ded3b5d2df947b...`
bitcoin-0.20.99-win64-debug.zip	`920cfb921c277ab6...`	`9dfa40bf139edfe0...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`499d0e7e3d66123b...`	`791c9b518c6abdf8...`
bitcoin-0.20.99-win64.zip	`b4c5466788c5ab31...`	`41a4a6508e018038...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`771c49516716cf2f...`	`31a3f8b797da7a6a...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`9baf50756b8e295a...`	`895c347213cf097f...`
bitcoin-0.20.99.tar.gz	`20db243e4a63e9be...`	`1fc2036952ccee6f...`
bitcoin-core-linux-0.21-res.yml	`3764dafda91b4998...`	`d7ed83fdd035bf2b...`
bitcoin-core-osx-0.21-res.yml	`c908e4d8f612478d...`	`945b04770e2d6597...`
bitcoin-core-win-0.21-res.yml	`65a1ad714e6b7e4c...`	`432443787e0a77c1...`
linux-build.log	`dbc67f519f1bc039...`	`ec6e55a0f6871c18...`
osx-build.log	`680987f733987400...`	`8f63c0eef3ddda73...`
win-build.log	`861910357a3af2e6...`	`f21909c383a9ff6d...`
bitcoin-core-linux-0.21-res.yml.diff		`cc1e3d989ad83643...`
bitcoin-core-osx-0.21-res.yml.diff		`9531524e18c3ca53...`
bitcoin-core-win-0.21-res.yml.diff		`e93b7e3b21a8b861...`
linux-build.log.diff		`93557f984e403a13...`
osx-build.log.diff		`7b6b97aa16fa46ba...`
win-build.log.diff		`259622ed1a4013ad...`

DrahtBot removed the label Needs gitian build on Apr 27, 2020

fanquake merged this on Apr 28, 2020

fanquake closed this on Apr 28, 2020

fanquake deleted the branch on Apr 28, 2020

sidhujag referenced this in commit d861869e55 on Apr 28, 2020

DrahtBot locked this on Feb 15, 2022

scripts: add PE .reloc section check to security-check.py #18629

Gitian builds

Guix builds

Gitian builds