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

_FORTIFY_SOURCE recommendation is a timebomb #659

Open
thesamesam opened this issue Oct 14, 2024 · 8 comments
Open

_FORTIFY_SOURCE recommendation is a timebomb #659

thesamesam opened this issue Oct 14, 2024 · 8 comments

Comments

@thesamesam
Copy link
Contributor

thesamesam commented Oct 14, 2024

The pattern that the documentation for _FORTIFY_SOURCE encourages is problematic.

We had this for years with _FORTIFY_SOURCE=2 and it caused unnecessary hassle when _FORTIFY_SOURCE=3 became available, as software would either clobber 3 or provoke warnings by setting its own.

We should encourage the use of AX_ADD_FORTIFY_SOURCE-like macros which first check the default fortification level and only override if it would improve protection. Not unconditionally setting it and blasting over defaults. I plan to handle this in Meson by addressing mesonbuild/meson#12341.

Note that many of the remaining blockers on our tracker bug in Gentoo at https://bugs.gentoo.org/847148 are issues of this nature.

@thomasnyman
Copy link
Contributor

thomasnyman commented Oct 14, 2024

@thesamesam: Thanks for reporting this. AX_ADD_FORTIFY_SOURCE is a useful addition and I am definitely in favor of covering that, but it is specific to Autotools.

Do I understand the concern with the current -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 pattern correctly that you are worried about the case, where in the future, there might be a -D_FORTIFY_SOURCE=4 or any -D_FORTIFY_SOURCE=N where N>3. In such a future the recommended stanza could in end up downgrading the fortification level for distribution compilers that may default to any _FORTIFY_SOURCE larger than3?

I'm wondering are there any good solutions for this that are build system agnostic? Ideally you'd want to be able to request a minimum fortification level from the compiler directly, which isn't currently possible.

For GCC specifically -fhardened / -Whardened might be preferable. We don't use that in the tl;dr; listing currently as these options are not applicable for Clang.

@thesamesam
Copy link
Contributor Author

thesamesam commented Oct 14, 2024

Do I understand the concern with the current -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 pattern correctly ...

Exactly. It's problematic also if wanting to do testing with =2 or =1 for coverage but that matters far less and someone doing that could workaround that with a patch to the compiler.

We unfortunately hit precisely that scenario you describe when going from level 2 to 3 and are still mopping up such cases.

I think the only options we have here are for:

  1. it to be implemented in Meson (which I will handle);
  2. we write some CMake snippet people can copy/paste, inspired by the Autoconf macro? (I'd say we could add it into CMake proper but they don't tend to be interested in that sort of thing AFAIK);
  3. recommend -fhardened where feasible.

@thomasnyman
Copy link
Contributor

Looking at AX_ADD_FORTIFY_SOURCE more closely it also doesn't give the desired behavior from the perspective of a developer who wants to ensure the maximum fortification level supported by the compiler is used since in its current form AX_ADD_FORTIFY_SOURCE gives up setting _FORTIFY_SOURCE=3 if any fortification level is already defined.

@david-a-wheeler
Copy link
Contributor

FYI, autotools (specifically automake) has multiple flag variables: https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html

@david-a-wheeler
Copy link
Contributor

Thomas will try to see if _FORTIFY_SOURCE=99 works on clang and gcc. Then we can ask for "maximum available" by default.

@thomasnyman
Copy link
Contributor

@06kellyjac beat me to it but the conclusion was that specifying a _FORTIFY_SOURCE higher than supported will cause a warning with glibc:

/usr/include/features.h:420:5: warning: #warning _FORTIFY_SOURCE > 3 is treated like 3 on this platform [-Wcpp]
420 | # warning _FORTIFY_SOURCE > 3 is treated like 3 on this platform

We discussed this issue at length during the C/C++ Compiler BP Guide call 2023-10-17 and came to the conclusion that we should try and suggest approaches for setting flags, including the -D_FORTIFY_SOURCE, which are compatible with as many distribution defaults as possible.

Not all distributions set their default flags in the same way though, @06kellyjac noted the approach in NIX OS, with a wrapper around the compiler and bintools that injects hardening flags may be more robust to differences in upstream build configurations.

We shouldn't of course tell Linux distributions what do do, but we can aim describe how the major ones set their default flags and strive for recommendations that are are compatible with as many as possible.

That said, there wasn't a clear consensus on what the best option for the build-system agnostic TL;DR; would be. Recommending -fhardened there currently seems problematic as well as that would mean we should move -D_FORTIFY_SOURCE to a situational flag.

@thomasnyman
Copy link
Contributor

Getting back to this, should we, instead of providing the tl;dr; as a list of options, provide the tl;dr, as suitable snippets or macros for common build systems?

Thinking more about AX_ADD_FORTIFY_SOURCE, it might be better to have a Automake macro that adds the correct set of flags, including conditional flags, automatically. Thoughts?

@david-a-wheeler
Copy link
Contributor

Getting back to this, should we, instead of providing the tl;dr; as a list of options, provide the tl;dr, as suitable snippets or macros for common build systems?

I think we should not replace the existing tl;dr. There are too many different build systems, including bespoke makefiles & scripts.

However, I think we could add a section on "how to do this in common build systems" and do what you suggested. That section will probably get long as we add build systems, so wouldn't be a tl;dr anyway, but it'd be good to provide.

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

No branches or pull requests

3 participants