Skip to content

cc-wrapper: include fortify-headers before libc includes for musl#219421

Merged
risicle merged 2 commits intoNixOS:stagingfrom
risicle:ris-fortify-headers-auto
Aug 6, 2023
Merged

cc-wrapper: include fortify-headers before libc includes for musl#219421
risicle merged 2 commits intoNixOS:stagingfrom
risicle:ris-fortify-headers-auto

Conversation

@risicle
Copy link
Copy Markdown
Contributor

@risicle risicle commented Mar 3, 2023

Description of changes

Musl itself doesn't have support for FORTIFY_SOURCE. Distributions like alpine use the fortify-headers project (https://git.2f30.org/fortify-headers/file/README.html) to provide some basic fortify support using a header #include_next wrapper/passthru mechanism.

This PR does the same, firstly by packaging fortify-headers (in fact this extracts them from the alpine package because upstream only has a bare git repository and we don't want to depend on git in the bootstrap phases), then by applying them from the cc-wrapper on musl systems (or if includeFortifyHeaders is set manually).

This can be tested using the tests in #217390 (cherry-pick on top of this). I added tests.hardeningFlags.fortify1ExplicitEnabledExecTest to that specifically for testing this PR - the hardening-check method won't be able to detect fortify-headers' entirely-inlined approach and fortify-headers really only implements FORTIFY_SOURCE=1 mode. This passes for me for pkgsMusl and pkgsStatic on nixos x86_64.

(Side note: I don't think it would be particularly hard to add FORTIFY_SOURCE=2 or even FORTIFY_SOURCE=3 mode to fortify-headers, but it feels like the author is opposed to this. Yell if you'd be interested for me to try this...)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@risicle risicle added the 6.topic: musl Running or building packages with musl libc label Mar 3, 2023
@risicle risicle requested a review from Ericson2314 as a code owner March 3, 2023 22:19
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Mar 3, 2023
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Mar 3, 2023
@risicle risicle force-pushed the ris-fortify-headers-auto branch from 0a575eb to 0bbae0e Compare March 5, 2023 20:06
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't version number appear in the URL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm the one calling this 1.1alpine1 - the r1 is not an official designation, just alpine's release of the package. And it does differ from upstream's package slightly, including a patch to fix ppoll on some systems https://git.alpinelinux.org/aports/commit/main/fortify-headers?id=4f60e618352e581f7f77a3842e29141da8992d5f. r3 in fact includes patches for clang support, but I'm not using that one yet because I can't find a stable url for it, only appearing in the edge release.

@risicle risicle force-pushed the ris-fortify-headers-auto branch from 0bbae0e to 56b5bb7 Compare August 6, 2023 10:48
@risicle risicle requested a review from a user August 6, 2023 10:48
@risicle
Copy link
Copy Markdown
Contributor Author

risicle commented Aug 6, 2023

(simple rebase)

@trofi I'm not having a lot of luck switching it to -idirafter despite #245550

@risicle
Copy link
Copy Markdown
Contributor Author

risicle commented Aug 6, 2023

It appears to me that what #245550 has done is make both pkgsMusl and pkgsStatic (i.e. cross) act in the way that prevents -idirafter being usable here.

I guess at least it's consistent though.

@trofi
Copy link
Copy Markdown
Contributor

trofi commented Aug 6, 2023

Yeah, I did not make it any better :(

@risicle risicle force-pushed the ris-fortify-headers-auto branch from 56b5bb7 to 95c4a1f Compare August 6, 2023 16:52
@risicle
Copy link
Copy Markdown
Contributor Author

risicle commented Aug 6, 2023

Have updated the comment. Would you approve of merging this as-is until someone decides to sort it out "properly"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: musl Running or building packages with musl libc 6.topic: stdenv Standard environment 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants