Consider applying flags for warnings about potential security issues

[mdboom]

Feature or enhancement

Proposal:

At a recent meeting of OpenSSF's Memory Safety SIG, I became aware of the C/C++ hardening guide they are putting together.

At a high-level, they recommend compiling with the following flags:

-O2 -Wall -Wformat=2 -Wconversion -Wtrampolines -Wimplicit-fallthrough \
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 \
-D_GLIBCXX_ASSERTIONS \
-fstrict-flex-arrays=3 \
-fstack-clash-protection -fstack-protector-strong \
-Wl,-z,nodlopen -Wl,-z,noexecstack \
-Wl,-z,relro -Wl,-z,now \
-fPIE -pie -fPIC -shared

(-shared doesn't really make sense as a global CFLAG, so I removed it.)

When compiling on most x86 architectures (amd64, i386 and x32), add:

-fcf-protection=full

At @sethmlarson's urging, I compiled CPython on Linux/x86_64/gcc with these flags. From the complete build log, there are 3,084 warnings, but otherwise the result builds and passes all unit tests.

The warnings are of these types: (EDIT: Table updated to not double count the same line)

warning type	count
sign-conversion	2,341
conversion	595
array-bounds=	131
format-nonliteral	11
stringop-overflow=	2
float-conversion	2
stringop-overread	1
maybe-uninitialized	1
total	3,084

**Top warnings per file.**

filename	count
./Modules/binascii.c	208
Objects/unicodeobject.c	142
./Include/internal/pycore_runtime_init.h	128
Parser/parser.c	114
./Modules/_decimal/libmpdec/mpdecimal.c	94
./Modules/posixmodule.c	85
./Modules/socketmodule.c	76
./Modules/_pickle.c	75
Objects/longobject.c	65
./Modules/arraymodule.c	49
total	3,084

I am not a security expert, so I don't know a good way to assess how many of these are potentially exploitable, and how many are harmless false positives. Some are probably un-resolvable (format-literal is pretty hard to avoid when wrapping sprintf, for example).

At a high level, I think the process to address these and make incremental progress maybe looks something like:

• Pick one of the warning types, and assess how many false positives it gives and how onerous it is to fix them. From this, build concensus about whether it's worth addressing.
• Fix all of the existing instances.
• Turn that specific warning into an error so it doesn't creep back in.

But this is just to start the discussion about how to move forward.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

• gh-112301: fix compiler warning about a possible use of an uninitialized variable #112308

[colesbury]

I don't think we want -fstrict-flex-arrays=3. We need flexible array members and we need C++ support, so we're forced to rely on the (widely supported) compiler extension of using field[0] or field[1] as a flexible array member.

[sobolevn]

Some are probably un-resolvable (format-literal is pretty hard to avoid when wrapping sprintf, for example)

These warnings do no make much sense in current use-cases:

Objects/unicodeobject.c:2592:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2592 |                     sprintf(buffer, fmt, va_arg(*vargs, long)) :
      |                     ^~~~~~~
Objects/unicodeobject.c:2593:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2593 |                     sprintf(buffer, fmt, va_arg(*vargs, unsigned long));
      |                     ^~~~~~~
Objects/unicodeobject.c:2597:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2597 |                     sprintf(buffer, fmt, va_arg(*vargs, long long)) :
      |                     ^~~~~~~
Objects/unicodeobject.c:2598:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2598 |                     sprintf(buffer, fmt, va_arg(*vargs, unsigned long long));
      |                     ^~~~~~~
Objects/unicodeobject.c:2602:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2602 |                     sprintf(buffer, fmt, va_arg(*vargs, Py_ssize_t)) :
      |                     ^~~~~~~
Objects/unicodeobject.c:2603:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2603 |                     sprintf(buffer, fmt, va_arg(*vargs, size_t));
      |                     ^~~~~~~
Objects/unicodeobject.c:2606:17: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2606 |                 len = sprintf(buffer, fmt, va_arg(*vargs, ptrdiff_t));
      |                 ^~~
Objects/unicodeobject.c:2610:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2610 |                     sprintf(buffer, fmt, va_arg(*vargs, intmax_t)) :
      |                     ^~~~~~~
Objects/unicodeobject.c:2611:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2611 |                     sprintf(buffer, fmt, va_arg(*vargs, uintmax_t));
      |                     ^~~~~~~
Objects/unicodeobject.c:2615:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2615 |                     sprintf(buffer, fmt, va_arg(*vargs, int)) :
      |                     ^~~~~~~
Objects/unicodeobject.c:2616:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
 2616 |                     sprintf(buffer, fmt, va_arg(*vargs, unsigned int));
      |                     ^~~~~~~

I think that they should be silenced / ignored.

[sethmlarson]

@mdboom Are you okay with me editing your topic to create a checklist style table with links to either why we're not implementing or the actual implementation? My guess is we'll be adopting these one by one :)

[mdboom]

@mdboom Are you okay with me editing your topic to create a checklist style table with links to either why we're not implementing or the actual implementation? My guess is we'll be adopting these one by one :)

@sethmlarson: Good idea.

[hugovk]

At a high level, I think the process to address these and make incremental progress maybe looks something like:

• Pick one of the warning types, and assess how many false positives it gives and how onerous it is to fix them. From this, build concensus about whether it's worth addressing.
• Fix all of the existing instances.
• Turn that specific warning into an error so it doesn't creep back in.

Sounds a good approach.

To share another method that could additionally help: as part of #101100, we're working through a lot of docs "nit-picky" warnings.

When building the docs, we only allow warnings to occur in files that already have warnings and are listed in a .nitignore file. Once a file has been cleaned, we remove it from the list to prevent regressions.

We also fail the docs build if we "accidentally" clean a file: if warnings do not occur in a file where we previously expected warnings, so the file must also be removed from the list, again to prevent regressions.

This does need some custom tooling, but it's helped us make gradual progress, and we've fixed 40% so far.

[carsonRadtke]

RE: @hugovk's .nitignore

I am in favor of a solution like this. It would not require any custom tooling as we could change the build arguments to whatever we find consensus in and then silence compiler warnings for offending lines until somebody comes along and fixes them.

This also allows us to silence errors locally, but enforce them globally. That way we could still have -Wformat-nonliteral, but allow non-compliance during the compilation of 'unicodeobject.c'. (I am not advocating for this flag, just using it as an example)