Skip to content
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

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

Merged
merged 2 commits into from
Aug 6, 2023

Conversation

risicle
Copy link
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.

Sorry, something went wrong.

@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 the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500 labels Mar 3, 2023
@risicle risicle force-pushed the ris-fortify-headers-auto branch from 0a575eb to 0bbae0e Compare March 5, 2023 20:06
# upstream only accessible via git - unusable during bootstrap, hence
# extract from the alpine package
src = fetchurl {
url = "https://dl-cdn.alpinelinux.org/alpine/v3.17/main/x86_64/fortify-headers-1.1-r1.apk";
Copy link
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
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
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
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
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
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 10.rebuild-linux: 101-500 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants