guix: Test security-check sanity before performing them (with macOS) #22381
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
From ci: |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
This was referenced Jul 2, 2021
892d689 to
5ebc5aa
Compare
When using mypy ignore directives, the error code needs to be specified. Somehow mypy doesn't print it by default...
The CI environment is a moving target, and these tests are somewhat fragile, so for now, disable them.
This is important to make sure that we're not testing tools different from the one we're building with. Introduce determine_wellknown_cmd, which encapsulates how we should handle well-known tools specification (IFS splitting, env override, etc.).
We use these flags in our test-security-check make target, but they are only available because debian patches them in. We can patch them in for our Guix builds so that we can check the sanity of our security/symbol checking suite before running them.
Also fix test-security-check.py to account for new PE PIE failure indication.
Now that our release binaries are build in a glibc 2.24 and 2.27 environment, we can't use a symbol from glibc 2.28 to test our checks. Replace renameat2() with nextup(), which was introduced in 2.24. Note that this also means re-disabling the test for RISC-V, however RISC-V is built in a glibc 2.27 environment, and our minimum required glibc for that binary is 2.27.
5ebc5aa to
5b4703c
Compare
|
I've made a few changes here, including rebasing now that #22405 has been merged, fixing up the ELF test-symbol-check test to account for it being run in the new glibc environments, and re-ordered some commits. Note that I've also removed the test-security(symbol)-check target from being run for Linux in the CI, mainly due to these tests being somewhat fragile. We can look at running them again when the security and symbol checks have been split up. I also have one bugfix for the symbol-check tests, that I'll PR shortly. |
Guix builds: |
hebasto
reviewed
Jul 7, 2021
|
Concept ACK! 😄 |
|
|
I seem to be getting matching results! |
fanquake
commented
Jul 9, 2021
There was a problem hiding this comment.
I'm going to go-ahead and merge this now. For additional context, I've also copied over some of the comments I left when reviewing #20980, that highlight the benefits of using Guix. The changes we're making here to patch our mingw-w64 toolchain and run additional security / sanity checks would be much harder / awkward to achieve inside gitian.
| @@ -900,6 +900,7 @@ if test x$use_hardening != xno; then | |||
| ]) | |||
| fi | |||
|
|
|||
| AX_CHECK_LINK_FLAG([[-Wl,--enable-reloc-section]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,--enable-reloc-section"],, [[$LDFLAG_WERROR]]) | |||
There was a problem hiding this comment.
From #20980 (comment)
I think testing for this, and adding to our hardened ldflags when available is fine. It's enabled by default, however we like to be explicit. It's also available with the binutils (2.34) we are using for gitian builds.
Note that some of these flags also imply each other:
--high-entropy-va implies --dynamic-base & --enable-reloc-section
--dynamic-base implies --enable-reloc-section
| Author: Stephen Kitt <skitt@debian.org> | ||
|
|
||
| This patch adds "no-" variants to disable the various security flags: | ||
| "no-dynamicbase", "no-nxcompat", "no-high-entropy-va", "disable-reloc-section". |
There was a problem hiding this comment.
From #20980 (comment)
Checked that this matches https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/blob/master/debian/patches/disable-flags.patch bar whitespace changes.
| self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--nxcompat','-Wl,--dynamicbase','-Wl,--high-entropy-va','-no-pie','-fno-PIE']), | ||
| (1, executable+': failed RELOC_SECTION')) | ||
| self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--nxcompat','-Wl,--dynamicbase','-Wl,--high-entropy-va','-pie','-fPIE']), | ||
| self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--disable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE']), |
There was a problem hiding this comment.
From #20980 (comment)
At this stage we have already given in to not being able to run the test security check target for windows in gitian due to lack of --no options in ld, so adding --disable here to test --enable-reloc-section isn't making anything worse. If anything this speaks to the usefulness of Guix, given how easy it is to patch these --no/--disable flags back into our toolchain. It would be much more difficult trying to achieve the same using gitian.
|
Guix builds: |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 10, 2021
laanwj
added a commit
that referenced
this pull request
Aug 18, 2021
…mbol-check 5449d44 scripts: prevent GCC optimising test symbols in test-symbol-check (fanquake) Pull request description: I noticed in #22381 that when the test-symbol-check target was being built with Clang and run in the CI it would fail due to using a too-new version of `pow` (used [here](https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/contrib/devtools/test-symbol-check.py#L85)). Our CIs use Focal (glibc 2.31) and the version of `pow` was the optimized version introduced in [glibc 2.29](https://lwn.net/Articles/778286/): ```bash * Optimized generic exp, exp2, log, log2, pow, sinf, cosf, sincosf and tanf. ``` This made sense, except for that if it was failing when built using Clang, why hadn't it also been failing when being built with GCC? Turns out GCC is optimizing away that call to `pow` at all optimization levels, including `-O0`, see: https://godbolt.org/z/53MhzMxT7, and this has been the case forever, or at least since GCC 5.x. Clang on the other hand, will only optimize away the `pow` call at `-O1` and `-O2`, not `-O0`: https://godbolt.org/z/Wbnqj3q6c. Thus when this test was built with Clang (we don't pass `-O` so we default to `-O0`) it was failing in the CI environment, because it would actually have a call to the "new" `pow`. Avoid this issue by using a symbol that won't be optimized away, or that we are unlikely to ever have versioning issues with. ACKs for top commit: laanwj: ACK 5449d44 Tree-SHA512: 3a26c5c3a5f2905fd0dd90892470e241ba625c0af3be2629d06d5da3a97534c1d6a55b796bbdd41e2e6a26a8fab7d981b98c45d4238565b0eb7edf3c5da02007
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 20, 2021
…test-symbol-check 5449d44 scripts: prevent GCC optimising test symbols in test-symbol-check (fanquake) Pull request description: I noticed in bitcoin#22381 that when the test-symbol-check target was being built with Clang and run in the CI it would fail due to using a too-new version of `pow` (used [here](https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/contrib/devtools/test-symbol-check.py#L85)). Our CIs use Focal (glibc 2.31) and the version of `pow` was the optimized version introduced in [glibc 2.29](https://lwn.net/Articles/778286/): ```bash * Optimized generic exp, exp2, log, log2, pow, sinf, cosf, sincosf and tanf. ``` This made sense, except for that if it was failing when built using Clang, why hadn't it also been failing when being built with GCC? Turns out GCC is optimizing away that call to `pow` at all optimization levels, including `-O0`, see: https://godbolt.org/z/53MhzMxT7, and this has been the case forever, or at least since GCC 5.x. Clang on the other hand, will only optimize away the `pow` call at `-O1` and `-O2`, not `-O0`: https://godbolt.org/z/Wbnqj3q6c. Thus when this test was built with Clang (we don't pass `-O` so we default to `-O0`) it was failing in the CI environment, because it would actually have a call to the "new" `pow`. Avoid this issue by using a symbol that won't be optimized away, or that we are unlikely to ever have versioning issues with. ACKs for top commit: laanwj: ACK 5449d44 Tree-SHA512: 3a26c5c3a5f2905fd0dd90892470e241ba625c0af3be2629d06d5da3a97534c1d6a55b796bbdd41e2e6a26a8fab7d981b98c45d4238565b0eb7edf3c5da02007
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is #20980 rebased (to include the Boost Process fix), and with an additional commit (892d689) to fix running the
test-security-checktarget for the macOS build. It should pass inside Guix, as well as when cross-compiling on Ubuntu, or building natively on macOS.Note that the
test-security-checkmay output some warnings (similar too):but those can be ignored, and come about due to us passing
-platform_versionwhen-mmacosx-version-minis already part ofCC.Guix builds: