Skip to content

gh-112301: Add fortify source level 3 to default compiler options #121520

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 18, 2024
Merged

gh-112301: Add fortify source level 3 to default compiler options #121520

merged 3 commits into from
Jul 18, 2024

Conversation

nohlson
Copy link
Contributor

@nohlson nohlson commented Jul 8, 2024

gh-112301: Added -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 to default compiler options for all builds

This option adds runtime protections for glibc to abort execution when unsafe behavior is encountered. Here are the GNU docs on the option

This is a very brief writeup that I found useful from Red Hat explaining some benefits

Also the OpenSSF compiler hardening guidance gives a very good description of the option

This is an option that theoretically affects the runtime so pyperformance benchmarks were run. The benchmark for this branch shows little overall impact but does impact some benchmark types:

NOTE: I would recommend looking into the details of the benchmarks in the links

Tag Geometric Mean
apps 1.00x slower
asyncio Not Significant
math 1.01x faster
regex 1.03x slower
serialize 1.01x faster
startup 1.00x faster
template 1.00x slower
overall 1.00x faster

A benchmark was run a few weeks ago with this option that was slightly different:

Tag Geometric Mean
apps 1.03x slower
asyncio 1.01x slower
math 1.00x slower
regex 1.02x faster
serialize 1.01x slower
startup 1.01x slower
template 1.01x slower
overall 1.01x slower

The benchmarks show that overall there is little memory impact and overall very small performance impact but within benchmark types there is some movement . Many compilers use -D_FORTIFY_SOURCE=2 by default. Level 3 adds additional bounds checking. My recommendation and the recommendation of the OpenSSF guidance is to raise to level 3 for this protection but would like to discuss further.

Attn: @mdboom

@nohlson nohlson changed the title Add fortify source level 3 to default compiler options gh-112301: Add fortify source level 3 to default compiler options Jul 8, 2024
@bedevere-app bedevere-app bot mentioned this pull request Jul 8, 2024
Open
@nohlson
Copy link
Contributor Author

nohlson commented Jul 8, 2024

@corona10 can we test with buildbots?

@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 9, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 453e34f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 9, 2024
@ghost
Copy link

ghost commented Jul 9, 2024 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@corona10
Copy link
Member

@nohlson I will take a look at this PR by this weekend, sorry, I am too busy these days.

@corona10 corona10 self-assigned this Jul 11, 2024
@corona10 corona10 force-pushed the enable-fortify-source-level-three branch from 1d61e81 to 7a595e7 Compare July 14, 2024 08:28
corona10
corona10 reviewed Jul 14, 2024
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@nohlson
I have 2 questions.

  1. Is this flag the subset of the overall policy that we want to measure?
    Consider applying flags for warnings about potential security issues #112301 (comment)
  2. Do you have a plan to provide some kind of ./configure --disable-openssf-guide?

@nohlson
Copy link
Contributor Author

nohlson commented Jul 17, 2024

@nohlson I have 2 questions.

  1. Is this flag the subset of the overall policy that we want to measure?
    Consider applying flags for warnings about potential security issues #112301 (comment)
  2. Do you have a plan to provide some kind of ./configure --disable-openssf-guide?
  1. This is a subset of the overall policy based on the OpenSSF guidance.
  2. I do not intend to add an argument to configure to turn off these options, however I would be interested in maybe including the ability to disable the new options that do impact performance benchmarks. This is theoretically one of those options however we didn't see a measurable impact. I could see there being a configure argument that disables the performance impacting options if we were to introduce one that has a measurable impact.

I would prefer the options we introduce for this effort to be always on, and put a lot of consideration into benchmark impacting options we do include, and possibly not enabling them if we deem the impact too much rather than enabling everything and making a new configure argument.

@corona10
Copy link
Member

@nohlson

I do not intend to add an argument to configure to turn off these options, however I would be interested in maybe including the ability to disable the new options that do impact performance benchmarks.

I am pretty sure that there will be users who want to disable options that we added for the OpenSSF guide with a single configuration command :)
Adding the optional argument will be helpful to them(even if enabling OpenSSF options is more recommended), I will submit the PR in this week.

@corona10 corona10 merged commit bdab67e into python:main Jul 18, 2024
38 checks passed
@vstinner
Copy link
Member

vstinner commented Jul 18, 2024 •
edited
Loading

This change broke RHEL8 buildbot, test_cext and test_cppext fail with:

  /usr/include/features.h:393:5: error: #warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Werror=cpp]
   #   warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform
       ^~~~~~~
  cc1plus: all warnings being treated as errors

@corona10
Copy link
Member

@nohlson Would you like to check?

@vstinner
Copy link
Member

vstinner commented Jul 19, 2024 •
edited
Loading

I suggest checking for the FORTIFY flag using -Werror in configure.

@mdboom mdboom mentioned this pull request Jul 19, 2024
Closed
@erlend-aasland
Copy link
Contributor

I suggest checking for the FORTIFY flag using -Werror in configure.

We already do.

@vstinner
Copy link
Member

We already do.

For other flags yes, but apparently, not for _FORTIFY_SOURCE=3:

# Enable flags that warn and protect for potential security vulnerabilities.
# These flags should be enabled by default for all builds.
AX_CHECK_COMPILE_FLAG([-fstack-protector-strong], [BASECFLAGS="$BASECFLAGS -fstack-protector-strong"], [AC_MSG_WARN([-fstack-protector-strong not supported])], [-Werror])
AX_CHECK_COMPILE_FLAG([-Wtrampolines], [BASECFLAGS="$BASECFLAGS -Wtrampolines"], [AC_MSG_WARN([-Wtrampolines not supported])], [-Werror])
AX_CHECK_COMPILE_FLAG([-D_FORTIFY_SOURCE=3], [BASECFLAGS="$BASECFLAGS -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3"], [AC_MSG_WARN([-D_FORTIFY_SOURCE=3 not supported])])

encukou added a commit to encukou/cpython that referenced this pull request Jul 22, 2024
@encukou
pythongh-112301: Revert "Add fortify source level 3 to default compil…
24d901a 
…er options (pythongh-121520)"

Adding the flag broke buildbots.

This reverts commit bdab67e.
@nohlson nohlson mentioned this pull request Jul 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@corona10 corona10 corona10 approved these changes

Assignees

@corona10 corona10

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nohlson @bedevere-bot @corona10 @vstinner @erlend-aasland