ossf / wg-best-practices-os-developers

The Best Practices for OSS Developers working group is dedicated to raising awareness and education of secure code best practices for open source developers.
https://openssf.org
Apache License 2.0
772 stars 133 forks source link

_FORTIFY_SOURCE recommendation is a timebomb #659

Open thesamesam opened 1 month ago

thesamesam commented 1 month ago

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 https://github.com/mesonbuild/meson/issues/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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

david-a-wheeler commented 1 month ago

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

thomasnyman commented 1 month ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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.