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

Add note of CVE-2023-4039 to -fstack-protector description #235

Merged

Conversation

thomasnyman
Copy link
Contributor

Fixes #234

@ware
Copy link

ware commented Sep 14, 2023

Is there a reason we're putting this in other than telling people to just use the latest, supported release?

@siddhesh
Copy link
Contributor

Is this within the scope of the guide though? In terms of best practices, one must always use at least -fstack-protector-strong and what the compiler does under the hood shouldn't really change that.

This creeps into the area of providing advisories for compilers (and the idea of providing advisories for missed hardening is dubious anyway, we'd be filing them till the cows mutate and go back into the ocean), which doesn't seem to fit in with this guide IMO.

@edelsohn
Copy link

Why is there a pull request for this change without addressing the objections from multiple people in the issue?

@thomasnyman
Copy link
Contributor Author

Addressed the feedback to #234 based on the discussion at the C/C++ Compiler BP Call 2023-09.27.

The reworded change addressed the underlying issue as a failure in the -fstack-protector[-strong,-all,] feature functioning perfectly in all situations rather than as an immediate security issue, addresses what users of the compiler can do to obtain better results, and adds a general disclaimer earlier in the guide on compiler features that are dependent on heuristics and may not always provide full coverage of protection.

More feedback on the reworded text is welcome.

@@ -53,7 +53,9 @@ Consequently, modern operating systems deploy various run-time mechanisms to pro

To benefit from the protection mechanism provided by the OS the application binaries must be prepared at build time to be compatible with the mitigations. Typically, this means enabling specific option flags for the compiler or linker when the software is built.

Some mechanisms may require additional configuration and fine tuning, for example due to potential compilation issues for certain unlikely edge cases, or performance overhead the mitigation adds for certain program constructs. This problem is exacerbated in projects that rely on an outdated version of an open source software (OSS) compiler. In general, security mitigations are more likely to be enabled by default in modern versions of compilers included with Linux distributions. Note that the defaults used by the upstream GCC project do not enable some of these mitigations.
Some mechanisms may require additional configuration and fine tuning, for example due to potential compilation issues for certain unlikely edge cases, or performance overhead the mitigation adds for certain program constructs. Some compiler features are dependent on heuristics that determine when the protection mechanism is applied and may not always provide full coverage of protection.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this instead:

Compiler security features depend on data flow analysis of programs and heuristics, results of which may vary depending on program source code layout. As a result, the protection mechanisms implemented by these features may not always provide full coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layout -> details

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've incorporated the suggested wording in 2763a9c.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is resolved. Let me know if that's not correct.

@@ -355,6 +357,8 @@ The detection is based on inserting a *canary* value into the stack frame in the

This mitigates potential control-flow hijacking attacks that may lead to arbitrary code execution by corrupting return addresses stored on the stack.

Out-of-date versions of GCC may not detect or defend against overflows of dynamically-sized local variables such as variable-length arrays or buffers allocated using `alloca()` when compiling for 64-bit Arm (Aarch64) processors[^Meta23]. Users of GCC-based toolchains for Aarch64 should ensure they use up-to-date versions of GCC 7 or later that support an alternative stack frame layout that places all local variables below saved registers, with the stack-protector canary between them[^Arm23].
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that there's no release with the aarch64 patchset; the patchset was merely pushed into release branches all the way back to gcc-7. Should we wait until there's an actual release before this note is added? For example, I don't know if a gcc-7 update release will even happen at all; the last one happened in 2019.

Copy link
Contributor

@david-a-wheeler david-a-wheeler Oct 11, 2023

Choose a reason for hiding this comment

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

Possible reword:

Some versions of GCC may not detect or defend against overflows of dynamically-sized local variables such as variable-length arrays or buffers allocated using alloca() when compiling for 64-bit Arm (Aarch64) processors[^Meta23]. Users of GCC-based toolchains for Aarch64 should, before depending on it, determine if they support an alternative stack frame layout that places all local variables below saved registers, with the stack-protector canary between them[^Arm23] [^CVE-2023-4039].

[^CVE-2023-4039] https://nvd.nist.gov/vuln/detail/CVE-2023-4039

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've incorporated the suggested wording in 2763a9c.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this is separately resolved.

thomasnyman and others added 2 commits October 11, 2023 18:10
Co-authored-by: David A. Wheeler <dwheeler@dwheeler.com>
Signed-off-by: Thomas Nyman <thomas.nyman@ericsson.com>
…ndent on heuristics

Co-authored-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Signed-off-by: Thomas Nyman <thomas.nyman@ericsson.com>
@thomasnyman thomasnyman force-pushed the add_note_of_cve-2023-4039_to_stack_protector_description branch from 3b4962a to 2763a9c Compare October 11, 2023 16:11
@thomasnyman
Copy link
Contributor Author

Addressed feedback from C/C++ BP Call on 2023-10-11.

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>
@thomasnyman
Copy link
Contributor Author

thomasnyman commented Nov 1, 2023

Merging based on discussion in C/C++ BP Call on 2023-10-11 as no objections on the new wording has been raised.

@thomasnyman thomasnyman merged commit 407960a into main Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GCC's -fstack-protector fails to guard dynamic stack allocations on Aarch64
5 participants