Feedback before initial release of Compiler Options Hardening Guide for C and C++

[thomasnyman]

Opening this issue for collecting feedback on the proposed Compiler Options Hardening Guide for C and C++ before its initial release.

Tagged release candidate for the guide can be found here.

[tcullum-rh]

Hi,

This guide is quite comprehensive! My only current suggestion is that as a developer, I would really appreciate more info about this:

Those who build programs for production may choose to omit some options that hurt performance if the program only processes trusted data, but remember that it’s not helpful to deploy programs that that are insecure and rapidly do the wrong thing. Existing programs may need to be modified over time to work with some of these options.

It would be helpful if for each option (or only the relevant options), a statement was made as to what the performance or other drawbacks are and any relevant workarounds.

[thomasnyman]

It would be helpful if for each option (or only the relevant options), a statement was made as to what the performance or other drawbacks are and any relevant workarounds.

Thank you for the feedback @tcullum-rh.

They way in which we have tried to accommodate this in the current version of the guide are the Performance implications and When not to use? sections in the detailed descriptions for each option (excluding the Warning options, as they do not impact the produced binary directly).

For example, for -fstack-protector-strong:

Stack protector supports three different heuristics that are used to determine which functions are instrumented with run-time checks during compilation:

-fstack-protector-strong: instrument any function that takes the address of any of its local variables on the right-hand-side of an assignment or as part of a function argument allocates a local array, regardless of type or length
allocates a local struct or union which contains an array, regardless of the type of length of the array
has explicit local register variables
-fstack-protector: instrument functions that call alloca() or allocate character arrays of n bytes or more in size . The threshold for instrumentation is adjustable via the --param=ssp-buffer-size=n option (default: 8 bytes).
-fstack-protector-all: instrument all functions.

The performance overhead is dependent on the number of function’s instrumented and the frequency at which instrumented functions are activated at run-time. Enabling -fstack-protector-strong is recommended as it provides the best balance between function coverage and performance. Projects using older compiler versions can consider -fstack-protector-all or -fstack-protector with a stricter threshold, e.g. --param=ssp-buffer-size=4.

When not to use?

-fstack-protector-strong is recommended for all applications with conventional stack behavior. Applications with hand-written assembler optimization that make assumptions about the layout of the stack may be incompatible with stack-protector functionality.

In general, the performance impact is surely the most frequently asked question when considering the deployment of these types of compiler options. Sadly, it is very difficult to estimate the impact in general, as it is dependent on the interaction with specific characteristics of the source code. Therefore, we've opted to try and characterize where the overhead comes from to try to help developers with how to measure the overhead in meaningful benchmarks in their project.

We are of course open to other suggestions of what kind of information would be useful for practitioners.

[david-a-wheeler]

@tcullum-rh : We agree in principle that important issues/caveats with an option should be mentioned with the option. In several cases where the issues are clear we have tried to do this. Sadly, often the answer is the lawyer's answer: "It depends". Whether or not there's a nontrivial performance impact often depends on many other factors, in particular, optimizations can sometimes (but not always) reduce or eliminate performance costs. If there's an important case where there's a clear issue and we haven't discussed it, please propose adding it! In general, the only way to answer the question is to run benchmarks. Everything else is only a quick estimate of the likely result.

[david-a-wheeler]

I received the following feedback from Daniel Stenbert, author of curl:

The document is a noble and worthy effort I think. Would probably have helped me if it had existed many years ago when I got started.

But wow. That page totally overwhelmed me and I felt like I was drowning when I tried to scrolled through it. My first feedback is that it needs to be shortened, by a lot. The footnotes and finer details could easily be moved to sub pages for example, and there could be much less "free prose" as right now it is very chatty.

Note: We could try to tighten up some prose.

We could also split the "summary" page from a "details" page. I don't know that we need to do that on this initial release but it's something worth discussing!

... I did not see any mention of -Wno-system-headers. Without that, lots of those warning options will be next to impossible to use in practice since they will find flaws all over in headers that are not yours. That option, combined with marking third party include files as "system headers" is really key to being able to really up the warning level on the compiler options.

I agree that we don't normally want warnings from system headers. However, in gcc -Wno-system-headers is the default. clang also normally suppresses warnings from system headers.

I hate to clutter up the options with default values. Maybe we should mention this as "we assume that warnings from system headers are suppressed, as is the default"? We could also mention that they might want to suppress warnings from third party include files as system headers, but that gets really specific - I'm not sure we can say in general how to do that (suggestions welcome).

Otherwise I think we for curl already use all of the recommended options plus a whole range of additional ones - as the exact selection of warnings to use that are good and not good always become rather project specific. Increasing the option levels only bring you so far and we've been at the point of zero-warnings with maximum warnings for many years. When that level is reached, then good static code analyzers can take you a little further and eventually fuzzing can take you yet some distance.

I think this comment suggests we're on the right track (at least). Everyone agrees that compiler option flags won't magically make software totally secure. However, if these flags are already in use by widely-used projects like curl, that suggests we haven't gone off the deep end into impossible requests.

We might want to emphasize that in practice projects end up with a specific set of flags (as much as you can do but no more).

[david-a-wheeler]

I created a PR #259 to briefly discuss system headers.

[eslerm]

Thank you for this!

Could you consider adding -mbranch-protection=standard for arm64? This is default in Debian and Fedora. In 2019 using glibc, Arm measured the use of the bti+pac options to reduce ~98% of ROP and JOP gadgets.

And x86_64 should have -fcf-protection=full for similar code reuse protection.

Is --enable-default-pie more appropriate?

[eslerm]

For developers, could we recommend -fstrict-flex-arrays=3 and -fsanitize=bounds?

https://people.kernel.org/kees/bounded-flexible-arrays-in-c
https://www.youtube.com/watch?v=V2kzptQG5_A

Distros need to test these flags and report breaks upstream before they can be a widely used.

[kees]

Please also add -Wimplicit-fallthrough

[david-a-wheeler]

@kees: Thanks so much for your feedback! I think adding -Wimplicit-fallthrough is a great idea.

[david-a-wheeler]

eslerm commented 24 minutes ago

For developers, could we recommend -fstrict-flex-arrays=3 and -fsanitize=bounds?

FYI, we have a separate issue specifically discussing -fstrict-flex-arrays here: #249

I'm supportive for adding -fstrict-flex-arrays but that does imply a lot of changes for some programs. Also, last I checked clang didn't support level 3 (which I think is a mistake on their part, but I don't control the clang project :-) ).

I don't think people have seriously considered adding -fsanitize=bounds, which instruments array bounds. The problem is the non-trivial performance overhead. The design of C (and thus C++) makes it challenging to have good performance when doing the kind of bounds-checking that is the default in almost all other programming languages. Here's one study, I'm sure there are more: https://zatoichi-engineer.github.io/2017/10/05/sanitize-object-size.html. Some people may find the performance loss acceptable for their use case, but C and C++ tend to used in the very situations where performance matters. So it's harder to argue for it in general. That isn't necessarily the end of the story, though. The current draft focuses only on option flags for production use. There's been discussion about extending this document to also include flags for use during testing and debugging. That WOULD make sense there - it makes sense to turn on sanitizers and run dynamic tools like fuzzers and test suites. Maybe you could help us expand the document to those use cases?

[eslerm]

Thanks David. Ack on production use.

Clang now supports level 3, but only on rather recent versions.

There is a similar ongoing discussion with GCC: RFC: Introduce -fhardened to enable security-related flags.

[kees]

Clang supports -fstrict-flex-arrays=3, so no worries there.

[david-a-wheeler]

This article has lots of ideas, I'm not sure we can do them all before an initial release: https://developers.redhat.com/articles/2022/06/02/use-compiler-flags-stack-protection-gcc-and-clang

[david-a-wheeler]

Per eslerm comment (#250 (comment)) I created a PR proposing to add branch protections here: #260
This covers -mbranch-protection=standard and -fcf-protection=full.

[david-a-wheeler]

eslerm commented 4 days ago asked:

Is --enable-default-pie more appropriate?

Interesting question. First, some context. I believe that --enable-default-pie is a GCC option used when recompiling the GCC compiler. By using it, the generated GCC compiler will default to including -fPIE and -pie options by default when it compiles something else. As far as I know that is GCC-specific. I believe a somewhat-equivalent configuration option clang/LLVM world is CLANG_DEFAULT_PIE_ON_LINUX. See this discussion on the clang default setting.

The problem with these options is that many application developers do NOT recompile their own compiler. They instead use a compiler provided to them (e.g., the system compiler or one distributed for their chipset). So while these options are valid if you're recompiling the compiler (e.g., a Linux distro), I think we cannot remove -fPIE and -pie because using them ensures you get the safer setting.

We could add a note that "if you recompile the compiler itself, it's best to modify its configuration itself so its defaults are maximally secure for any software compiled using it. For example..." or something along those lines.

[david-a-wheeler]

I've created a PR to note that if you are compiling a compiler, make its defaults as safe/secure as you can. For many readers this isn't helpful (many developers do not compile their own compilers), but this is a useful note for Linux distributions and some others who do compile their own compilers: #261

[david-a-wheeler]

I've added this PR to add -Wimplicit-fallthrough: #262

[david-a-wheeler]

This PR proposes to add the option -fstrict-flex-arrays=3: #263

Note that this PR, if accepted, would fix issue #249.

[david-a-wheeler]

I propose that before the initial release we merge the PRs in response to this issue. That is, at least accept #259, #260, #261, #262, #263. If there are other PRs that are (mainly) ready to go, let's get them in. I'd like our first draft to have at least the main agreed-on results that I believe are captured here.

[david-a-wheeler]

We should probably discuss adding -mshstk to the C/C++ Compiler Hardening Guide. I've created a separate issue #267 to track this. Is anyone aware of any problems with this flag? If they are, please comment on #267.

[david-a-wheeler]

@gkunz @thomasnyman and others - I'd love to hear your thoughts on #259, #260, #261, #262, #263 . If you think they makes sense, please merge (or say it's okay to merge). We might want to implement #267 but I expect we'll create merge conflicts as long as we have a number of changes outstanding. Merge conflicts aren't the end of the world, but I'd rather not need to deal with them unnecessarily :-).

[thesamesam]

For developers, could we recommend -fstrict-flex-arrays=3 and -fsanitize=bounds?

people.kernel.org/kees/bounded-flexible-arrays-in-c youtube.com/watch?v=V2kzptQG5_A

Distros need to test these flags and report breaks upstream before they can be a widely used.

I like the idea of this but this may deserve an "emerging features" section or similar (things we want to work towards). It's not suitable for global use right now, but it'd be great if anyone wants to take up work to get us there. Listing it as a goal would let us list it without giving people the wrong idea.

[SecurityCRob]

We'll close this out as 1.0 is out now!