#1185 Enable "-Werror=format-security" by default
Closed None Opened 10 years ago by halfie.

It would be great if we could turn on the "-Werror=format-security" compilation hardening flag in Fedora for all packages.

This flags turns warnings, given for using format functions in a way that might result in possible security problems, into errors. For details, see http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html URL.

Format string vulnerabilities are (unfortunately) still common. See
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661548 URL. In short, such proactive hardening action caused a FTBFS (in this very particular case) and further investigations found out that this package had a security vulnerability (CVE-2012-1152). For more such cases, see [3], [4] and [5].

For learning more about format string vulnerability, see
https://www.owasp.org/index.php/Format_string_attack URL.

Implementing this change requires a single line change to be made to the
/usr/lib/rpm/redhat/macros file (part of redhat-rpm-config package). Here is my patch to do so, https://bitbucket.org/dhiru/redhat-rpm-config/branch/strict-format.

At this point in time, turning on this flag should be relatively safe, since many FTBFS issues have been found (and fixed) already by Debian and Ubuntu folks. This means that all the hard work has already been done for us ;)

Please note that I am currently working on doing a mass rebuild using this new flag.

'''Current Problem'''

The following code is problematic but Fedora's default compilation flags don't catch the problem.

{{{
$ cat problem.c

include <stdio.h>

int main(void)
{

char buf[8192];
int ret;

ret = scanf("%s", buf);

printf(buf);  /* crappy insecure code! */

printf("%d\n", ret);

return 0;

}
}}}

'''On Fedora 20'''

$ gcc problem.c # no problems!

$ gcc rpm -E '%{?__global_cflags}' problem.c # still no problems ;(

'''On Debian (and Ubuntu)'''

{{{
$ gcc problem.c # catches the problem (using gcc's defaults)
problem.c: In function ‘main’:
problem.c:7:2: warning: format not a string literal and no format arguments [-Wformat-security]
printf(buf);
^

$ DEB_BUILD_HARDENING=1 gcc problem.c # catches the problem and prevents compilation!
problem.c: In function ‘main’:
problem.c:7:2: error: format not a string literal and no format arguments [-Werror=format-security]
printf(buf);
^
cc1: some warnings being treated as errors

}}}

DEB_BUILD_HARDENING=1 by itself does NOT turn on PIE by default, so it does NOT behave in the same way as Fedora's "%global _hardened_build 1" thing.

'''On Ubuntu'''

{{{
$ dpkg-buildflags
CFLAGS=-g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security
CPPFLAGS=-D_FORTIFY_SOURCE=2
CXXFLAGS=-g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security
FFLAGS=-g -O2
LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro

$ gcc -dumpspecs | grep format # on Ubuntu
%{!fno-stack-protector:%{!fstack-protector-all:%{!ffreestanding:%{!nostdlib:-fstack-protector}}}} %{!Wno-format-security:%{!Wformat|!Wformat=2|!Wall:-Wformat} -Wformat-security}
}}}

'''Notes'''

"Using "dpkg-buildflags" is the recommended way to incorporate the build flags in Debian." [2]

So, for Fedora, "-Werror=format-security" needs to be turned on in redhat-rpm-config.

Also note that we don't need to explicitly mention "-Wformat" since it it included in "-Wall" which we already use in Fedora.

Ubuntu has been using "-Wformat -Wformat-security" flags since its 8.04 LTS release!

Ubuntu has been using "-Werror=format-security" flag since its 13.04 release.

'''References'''

[1] https://www.owasp.org/index.php/Format_string_attack

[2] https://wiki.debian.org/Hardening

[3] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661536

[4] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=654270

[5] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=649322

[6] https://wiki.ubuntu.com/ToolChain/CompilerFlags

[7] https://wiki.debian.org/HardeningWalkthrough


At present, this warns about calls to 'printf' and 'scanf'
functions where the format string is not a string literal and
there are no format arguments, as in 'printf (foo);'.
This seems like a reasonable heuristic that's not likely to break much correct code. Still, perhaps we should wait for the results of the mass rebuild you are working on to judge the amount of false positives.

Please produce a list of all packages affected (as well as the number of times this warning appears in each package, if possible). I'd like to see what we're dealing with.

We're currently doing a mass rebuild. It's going to take at least a week though. The results will be noted here when done.

From the 2013-10-30 FESCo meeting:
* AGREED: defer pending results of mass rebuild (+;9, -0, 0:0)
(notting, 18:33:30)
* AGREED: ask gcc to make warning part of -Wall now, defer enabling it
as an error until the mass rebuild test is done (+:7, -:0, 0:0)
(notting, 18:38:28)

halfie/bressers - can you file that RFE and note it here? In the meantime, we'll wait for the mass rebuild results.

bressers, halfie -- is the mass rebuild done? If so, we'll discuss this at tomorrow's meeting.

info Adding to gcc's -Wall as a local Fedora patch was rejected by the gcc maintainers

info Deferred another week awaiting results of mass rebuild

Any news on the test mass rebuild?

The builds are still going (we need more time).

The builds are now done and we have 399 packages which FTBFS.

I am all set to start filing the bugs (once given the green signal). In addition, I am willing to help in patching these packages.

I believe that this work is important and will benefit everyone (including upstream and other distributions).

halfie: would you mind attaching the list of affected packages to this ticket for us to evaluate?

Additionally, if we do give you the go-ahead to file bugs, please follow http://fedoraproject.org/wiki/Mass_bug_filing to the letter. It makes life so much easier.

Side-note to FESCo in general: can we update the Mass_bug_filing page to require that the subject of the mass-bugs contains the component name? Otherwise, when looking at the tracking bug (and in the tree view), you usually end up with dozens or hundreds of bugs that cannot easily be differentiated from one another without clicking into them. I'll make an edit to the page if we agree.

Replying to [comment:12 sgallagh]:

halfie: would you mind attaching the list of affected packages to this ticket for us to evaluate?

[http://people.fedoraproject.org/~halfie/rebuild-logs.txt]

  • AGREED: 1) wait a week for devel feedback (from people already interested); 2) if there are no show-stoppers identified, mass file bugs; 3) 3 weeks later, enable in rawhide configuration by default (+8, 0, -0) (sgallagh, 18:14:50)
  • AGREED: Please file a Change page for this. (+7, 0, 0) (sgallagh, 18:17:44)

I'm somewhat flummoxed that gcc itself fails to build with this error enabled...

That being said, 22 package failures are pretty manageable.

AGREED: Give the go-ahead to mass-file bugs (+5,-0,0) (mmaslano, 18:14:48)
Change owner shouldn't file bugs for those already fixed (mmaslano, 18:15:27)

Feel free to mass file bugs, but please try not to file bugs for already fixed packages.

Leaving open for step 3 - enable in rawhide.

-Werror would make sense for code constructions causing undefined behavior. However, this checker only detects poor coding style and should really be treated as a warning only. Enforcing coding style via -Werror is a way to hell. FYI the GCC bug you refer to in this ticket is already closed NOTABUG:

https://bugzilla.redhat.com/show_bug.cgi?id=1025300#c3

I'm not exactly happy about the way this has been planned (at least if I understand this correctly)

there is no respect to upstream release cycles, if the builds will start to fail in three weeks, we are required to patch packages now, no matter whether a new upstream release is on its way, making the patch and the associated work in vain, right?

my concrete case: https://bugzilla.redhat.com/show_bug.cgi?id=1037177#c1

Where do you get "the builds will start to fail in three weeks" ?

When the macro change is made? This would only affect you if you need to push a build for some other reason, before the new upstream release with the fix. Sure, it would mean carrying a small patch for a short time, but we often do have to do that anyhow?

I have no time to read this ticket and please don't ask me to read it. What I want to say here is this feature should have not approved without proper examples of how to fix source code written in various programming languages.

FESCo should take care of this in future before approving any such features. Also, feature owner should announce on devel list before going for mass bug filing and explain how to fix it. For me filed bug was of less information.

Replying to [comment:22 pnemade]:

I have no time to read this ticket and please don't ask me to read it. What I want to say here is this feature should have not approved without proper examples of how to fix source code written in various programming languages.

"The fix for these errors is quite simple (in most cases). It's a matter of changing a line like, printf(foo), to read printf("%s", foo), instead. That's it. More details are available on Format-Security-FAQ."
- Second bullet point on https://fedoraproject.org/wiki/Changes/FormatSecurity#Scope

Thanks.I have read it but the code for which I am not familiar like qt one qDebug() function, I have no idea how to rewrite it to make this feature happy. It took almost 2 days to search and search and then giveup to send email on devel list for help.

Replying to [comment:22 pnemade]:

I have no time to read this ticket and please don't ask me to read it. What I want to say here is this feature should have not approved without proper examples of how to fix source code written in various programming languages.

IMHO each packager should be generally able to use the programming languages their packages are written in. At the very least, https://fedoraproject.org/wiki/Package_maintainer_responsibilities requires packagers to ask for help (which you have done).

Also, feature owner should announce on devel list before going for mass bug filing and explain how to fix it.
This has been discussed on fedora-devel since Nov 20, almost two weeks before filing the bugs.

IMHO, turning this warning into an error is a horribly flawed idea. It just has way too many false positives. For example, here's the snippet it complains about in Qt 3:
{{{
QString line;
line.fill( '-', 60 );
qDebug( line.ascii() );
}}}
As you can see, the format string being passed here is provably constant. The code just avoids spelling out 60 dashes.

Using -Werror=anything is '''always''' a bad idea. Expecting maintainers to actually go and patch the code for false positives rather than just adding -Wno-error=format-security is also unrealistic.

There are also valid uses of variable format strings which cannot simply be fixed by adding a "%s" (because they're actual format strings), think e.g. a printf wrapper for logging which adds a timestamp in front of the format string. Format strings could also be translatable. This flag makes our compiler no longer comply to the C/C++ standards and breaks valid and perfectly secure code.

I agree with Kevin and will propose to keep it as warning only added to CFLAGS on next FESCo meeting.
It still can break packages that add -Werror but number of such packages should absolute minimum.

I agree with previous comment. Few people complained to me personally.

btw I guess we didn't approve the last step: "Leaving open for step 3 - enable in rawhide". There should be only filled bugs.

Replying to [comment:26 kkofler]:

IMHO, turning this warning into an error is a horribly flawed idea. It just has way too many false positives. For example, here's the snippet it complains about in Qt 3:
{{{
QString line;
line.fill( '-', 60 );
qDebug( line.ascii() );
}}}
As you can see, the format string being passed here is provably constant. The code just avoids spelling out 60 dashes.

It seems that KDE folks are actively fixing such (and similar) cases themselves. Please see [http://lists.kde.org/?l=kde-commits&m=126756678217346 this] URL for details.

Note -Werror=format-security vs -Wformat-security was mentioned obliquely at the 2013-11-20 fesco meeting where we approved this:

18:11:08 <abadger1999> mitr: We've seen higher breakage from updating gcc than this.

18:12:51 <mitr> abadger1999: but we haven't been the only distribution that refuses to build the software. "But it works and will continue to work on Ubuntu, wontfix"

18:13:42 <mitr> ... I suppose if this is still limited to RPMs, I'm fine with my original proposal. But I'm noticeably less enthusiastic.

18:14:37 <abadger1999> mitr: from the ticket: "At this point in time, turning on this flag should be relatively safe, since many FTBFS issues have been found (and fixed) already by Debian and Ubuntu folks. This means that all the hard work has already been done for us ;)"

18:15:04 <mitr> abadger1999: but Ubuntu doesn't have Werror by default

http://meetbot.fedoraproject.org/teams/fesco/fesco.2013-11-20-17.59.log.html

Side note to that exchange, Ubuntu now has -Werror=format-security by default: https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-Wformat_-Wformat-security

So... I'm still +1 to turning on -Werror=format-security in rawhide.

It seems that KDE folks are actively fixing such (and similar) cases themselves.

But KDE upstream last cared about Qt 3 more than 5 years ago.

Replying to [comment:32 kkofler]:

It seems that KDE folks are actively fixing such (and similar) cases themselves.

But KDE upstream last cared about Qt 3 more than 5 years ago.

Can we afford the "risk" involved in shipping such software? Sure, we can do it if we are willing to do the work involved in the process.

What we can't really do is to tell others to stop "moving forward" only because we are too far behind (and obsolete). This last line is my personal opinion (more of a rant, sorry).

Also it seems that this "qt3" stuff was removed from Debian some time back. See [https://wiki.debian.org/qt3-x11-freeRemoval this URL] for more information.

I believe that Debian is well known for shipping stale (i.e. "super stable") software but even they have moved on. We surely can do better as a bleeding-edge distribution.

FESCo affirmed this change for F21 at today's meeting.

( AGREED: We add -Werror=format-security by default. (A -1 indicates that we will add -Wformat-security instead) (+7,-2)

Can we afford the "risk" involved in shipping such software? Sure, we can do it if we are
willing to do the work involved in the process.

Uh, I am very much willing to fix '''actual''' security issues in Qt 3! In fact, I backported the fix for CVE-2013-4549 from Qt 4:
http://pkgs.fedoraproject.org/cgit/qt3.git/commit/?id=437bc95f6a4c62030d1e4d2859448b3a235e756b
and I can tell you it was not trivial to do so. I also tested it hard and tried it even in GDB to make sure the fix is doing what it should. The reason the update is not yet pushed is, I have some concerns about the correctness of the upstream fix, it is breaking existing KDE XML files (also in Qt 4 and 5):
https://bugreports.qt-project.org/browse/QTBUG-35459
As you can see, I really '''do''' care about keeping qt3 working.

What I have a beef with is having to "fix" "security" '''non'''-issues such as that -Werror=format-security '''false positive''' I posted here!

I find it really sad that FESCo is forcing that silly plan through.

(And by the way, the QTBUG-35459 regression was actually found '''because''' of my work on backporting the fix to Qt 3 and testing it there, so I am helping improve Qt 4 and 5 that way, too!)

An example of upstream being super stubborn is [http://www.info-zip.org/phpBB3/viewtopic.php?f=7&t=426 at this URL] ;)

I fully support the reaction of the unzip upstream, they make perfectly valid points (in fact, ones I have already made). "Fixing" false positives from broken compiler warnings is a waste of everyone's time.

Speaking only as myself here:

  • I definitely do appreciate the work you do maintaining qt3 and all the other packages you do. Thank you.

  • While in some cases this check is a false positive and you have to make (minor) changes just to pass the check, IMHO this check is very much worth it overall. Please look beyond your single cases to the distribution as a whole. While your case is a false positive and you have to add a few lines of code, overall it IS showing exploitable cases and issues in other packages. So, on a whole, as a distribution it's still worth it, even if it causes inconvience for some small % of packages/maintainers/upstreams.

  • The amount of time and effort to simply adjust to accomodate the check (as it's enabled in a bunch of other distros as well) seems small to me next to the amount of time writing up long posts about why someone shouldn't want to.

Finally, FESCo has already decided this... twice. If you have some new information to support your case or have some reason for us to revisit it, great. If not, please adjust your code and move on?

Bugzilla ticket (for the required redhat-rpm-config change) is at https://bugzilla.redhat.com/show_bug.cgi?id=1043495.

All the required changes are in place. Can we please go ahead and do a mass rebuild (soon enough)?

We typically try and do only one mass rebuild per cycle. So, we need to wait here and find out what other changes need a mass rebuild and when they will be ready, then schedule it. ;)

If all the required change does is enable errors on some warnings, what good does a mass rebuild do? It doesn't change the exposure of bad code.

Login to comment on this ticket.

Metadata