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

build: Add hardening options #4538

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Nov 14, 2024

Get rid of the FORTIFY options, which were too specific to one compiler feature. Add OIIO_HARDENING that takes a level meaning:

  • 0 : do nothing, not recommended.
  • 1 : enable features that have no (or nearly no) performance impact, recommended default for optimized, shipping code.
  • 2 : enable features that trade off performance for security, recommended for debugging or deploying in security-sensitive environments.
  • 3 : enable features that have a significant performance impact, only recommended for debugging.

Default to 1 for optimized builds, 3 for debug builds (so will be thoroughly tested by our sanitizer and other CI tests). Users that have more stringent security requirements may choose to build with 2 even for shipping code (they should benchmark to see if that is acceptable to them).

These levels turn on a variety of compiler options that are recommended for additional safety. We will add more as they are developed.

There are also some warning suppressions that needed to be added to code in a few areas where it was unavoidable to use some constructs that trigger the elevated safety checks.

Get rid of the FORTIFY options, which were too specific to one
compiler feature option.  Add OIIO_HARDENING that takes a level
meaning:

- 0 : do nothing, not recommended.
- 1 : enable features that have no (or nearly no) performance impact,
      recommended default for optimized, shipping code.
- 2 : enable features that trade off performance for security, recommended
      for debugging or deploying in security-sensitive environments.
- 3 : enable features that have a significant performance impact, only
      recommended for debugging.

Default to 1 for optimized builds, 3 for debug builds (so will be
thoroughly tested by our sanitizer and other CI tests). Users that
have more stringent security requirements may choose to build with 2
even for shipping code (they should benchmark to see if that is
acceptable to them).

These levels turn on a variety of compiler options that are
recommended for additional safety. We will add more as they are
developed.

There are also some warning suppressions that needed to be added to
code in a few areas where it was unavoidable to use some constructs
that trigger the elevated safety checks.

Signed-off-by: Larry Gritz <[email protected]>
# needed for address space randomiztion used by some kernels.
add_compile_options (-fPIE -pie)
add_link_options (-fPIE -pie)
# Protect against stack overwrites. Is allegedly not a performance
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you measured this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not directly. I'm trusting the docs of the compilers and the fact that many distros are starting to build their compilers to have these enabled by default.

The docs say that the pie options were measurable cost on 32 bit platforms, but on 64 is like 0.1%, barely detectable.

If anybody is concerned, then can set OIIO_HARDENING=0 and measure on their own workloads.

@fpsunflower
Copy link
Contributor

OpenSSF maintains a set of options which I think goes a bit beyond what you have here:
https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html

(I haven't read it in great detail, but the TL;DR seemed to contain a few additional flags)

To me this really should be implemented at the cmake level (seems silly for every project to replicate this logic with associated checks on compiler versions, etc ...) I also assume that this set of recommendations will evolve over time.

Of course if we need to have something today we can do it ourselves as this patch kicks off, but going forward, is there any way we can push on the OpenSSF folks to "upstream" their document into build systems? Or is that already underway?

@lgritz
Copy link
Collaborator Author

lgritz commented Nov 14, 2024

Yeah, I based part of this on that document... but this is just a first stab, concentrating on the options that I thought I already had enough knowledge to know if it was a good idea. I assume that a 2nd or more pass will occur where we fill out more into the infrastructure I've set up here.

I don't know if CMake is looking at building this all into a single CMake option, but that sure would be ideal. OpenSSF is also a LF org, so we certainly can make a suggestion that they consider embodying their advice as a cmake patch.

@lgritz
Copy link
Collaborator Author

lgritz commented Nov 14, 2024

Other than a few additional warnings, nothing in our CI is tripping any of the additional assertions or runtime checks, so I didn't have to modify as much code as I thought I might as part of this PR. I think we've already rooted out most of the problems that they are trapping, which is good.

@lgritz
Copy link
Collaborator Author

lgritz commented Nov 14, 2024

Yeah, I based part of this on that document... but this is just a first stab,

To further elaborate: In this first hardening PR, I skipped over a lot of the recommendations where I was unsure at which 0-3 level it should be enabled, or if I wasn't at all sure what the performance implications would be. I think all of those cases will need to be examined individually.

@fpsunflower
Copy link
Contributor

Makes sense.

LGTM on this change -- it seems like a good thing to have and hopefully people who are more steeped in the details of these flags can tell us what we should add/remove if these aren't right.

Just wondering aloud - what are other libraries doing in this space? Is every project out there independently adding some combination of these flags to their build systems? Do distros have some recommended set of flags they like to add to all projects?

@lgritz
Copy link
Collaborator Author

lgritz commented Nov 14, 2024

Yes, distros are starting to do some combination of (a) changing compiler defaults to enable a reasonable number of these, and (b) publishing hardening guides, such as this one from Debian. And there's the OpenSSF guide you cited above.

I'm not sure what projects are doing generally, but these modest steps seemed both prudent and also tick off another box in the OpenSSF badging requirements we're working toward (I forget if it's silver or gold, but at some point we need to say we have turned on reasonable compiler options for hardening). So I expect that to achieve the badges, other ASWF projects will start to follow suit.

@lgritz lgritz merged commit d984586 into AcademySoftwareFoundation:main Nov 15, 2024
28 checks passed
@lgritz lgritz deleted the lg-harden branch November 15, 2024 21:23
@lgritz
Copy link
Collaborator Author

lgritz commented Nov 15, 2024

Another related link I just ran across minutes ago:
https://clang.llvm.org/docs/SafeBuffers.html

lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Nov 21, 2024
Get rid of the FORTIFY options, which were too specific to one compiler
feature. Add OIIO_HARDENING that takes a level meaning:

- 0 : do nothing, not recommended.
- 1 : enable features that have no (or nearly no) performance impact,
recommended default for optimized, shipping code.
- 2 : enable features that trade off performance for security,
recommended for debugging or deploying in security-sensitive
environments.
- 3 : enable features that have a significant performance impact, only
recommended for debugging.

Default to 1 for optimized builds, 3 for debug builds (so will be
thoroughly tested by our sanitizer and other CI tests). Users that have
more stringent security requirements may choose to build with 2 even for
shipping code (they should benchmark to see if that is acceptable to
them).

These levels turn on a variety of compiler options that are recommended
for additional safety. We will add more as they are developed.

There are also some warning suppressions that needed to be added to code
in a few areas where it was unavoidable to use some constructs that
trigger the elevated safety checks.

Signed-off-by: Larry Gritz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants