Skip to content

Conversation

@nohlson
Copy link
Contributor

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

TagGeometric Mean
apps1.00x slower
asyncioNot Significant
math1.01x faster
regex1.03x slower
serialize1.01x faster
startup1.00x faster
template1.00x slower
overall1.00x faster

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

TagGeometric Mean
apps1.03x slower
asyncio1.01x slower
math1.00x slower
regex1.02x faster
serialize1.01x slower
startup1.01x slower
template1.01x slower
overall1.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

@nohlsonnohlson changed the title Add fortify source level 3 to default compiler optionsgh-112301: Add fortify source level 3 to default compiler optionsJul 8, 2024
@nohlson
Copy link
ContributorAuthor

@corona10 can we test with buildbots?

@corona10corona10 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-botbedevere-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

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.

@corona10corona10 self-assigned this Jul 11, 2024
@corona10corona10force-pushed the enable-fortify-source-level-three branch from 1d61e81 to 7a595e7CompareJuly 14, 2024 08:28
Copy link
Member

@corona10corona10 left a comment

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?
    #112301 (comment)
  2. Do you have a plan to provide some kind of ./configure --disable-openssf-guide?

@nohlson
Copy link
ContributorAuthor

@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.

@vstinner
Copy link
Member

vstinner commented Jul 18, 2024

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

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

@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
…er options (pythongh-121520)" Adding the flag broke buildbots. This reverts commit bdab67e.
Sign up for freeto 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.

5 participants

@nohlson@bedevere-bot@corona10@vstinner@erlend-aasland