Ticket #1185 (closed task: fixed)

Opened 19 months ago

Last modified 15 months ago

Enable "-Werror=format-security" by default

Reported by: halfie Owned by:
Priority: major Keywords: meeting
Cc: lmacken, pnemade Blocked By:
Blocking:

Description

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

Change History

comment:1 Changed 19 months ago by mitr

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.

comment:2 Changed 19 months ago by sgallagh

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.

comment:3 Changed 19 months ago by notting

  • Keywords meeting added

comment:4 Changed 19 months ago by bressers

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.

comment:5 Changed 19 months ago by notting

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.

comment:7 Changed 19 months ago by toshio

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

comment:8 Changed 19 months ago by toshio

#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

comment:9 Changed 19 months ago by kevin

Any news on the test mass rebuild?

comment:10 Changed 19 months ago by halfie

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

comment:11 Changed 18 months ago by halfie

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

comment:12 follow-up: ↓ 13 Changed 18 months ago by sgallagh

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.

comment:13 in reply to: ↑ 12 Changed 18 months ago by halfie

Replying to 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

comment:14 Changed 18 months ago by sgallagh

  • 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)

comment:15 Changed 18 months ago by halfie

The change proposal page for this is at https://fedoraproject.org/wiki/Changes/FormatSecurity URL.

comment:16 Changed 18 months ago by lmacken

  • Cc lmacken added

As far as I can tell, 22 critical path packages FTBFS with this feature.

http://lmacken.fedorapeople.org/gcc-wall-security-critpath-ftbfs.txt

https://gist.github.com/lmacken/7652081

comment:17 Changed 18 months ago by sgallagh

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

That being said, 22 package failures are pretty manageable.

comment:18 Changed 18 months ago by mmaslano

  • Keywords meeting removed

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.

comment:19 Changed 18 months ago by kdudka

-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

comment:20 Changed 18 months ago by kvolny

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

comment:21 Changed 18 months ago by kevin

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?

comment:22 follow-ups: ↓ 23 ↓ 25 Changed 18 months ago by 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.

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.

comment:23 in reply to: ↑ 22 Changed 18 months ago by toshio

  • Cc pnemade added

Replying to 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."

Last edited 18 months ago by toshio (previous) (diff)

comment:24 Changed 18 months ago by pnemade

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.

comment:25 in reply to: ↑ 22 Changed 18 months ago by mitr

Replying to 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.

comment:26 follow-up: ↓ 30 Changed 18 months ago by 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.

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.

comment:27 Changed 18 months ago by kkofler

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.

comment:28 Changed 18 months ago by tmraz

  • Keywords meeting added

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.

comment:29 Changed 18 months ago by mmaslano

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.

comment:30 in reply to: ↑ 26 Changed 18 months ago by halfie

Replying to 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 this URL for details.

comment:31 Changed 18 months ago by toshio

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.

comment:32 follow-up: ↓ 33 Changed 18 months ago by 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.

comment:33 in reply to: ↑ 32 Changed 18 months ago by halfie

Replying to 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 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.

comment:34 Changed 18 months ago by mattdm

  • Resolution set to fixed
  • Status changed from new to closed

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)

comment:35 Changed 18 months ago by kkofler

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.

comment:36 Changed 18 months ago by kkofler

(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!)

comment:37 Changed 18 months ago by halfie

An example of upstream being super stubborn is at this URL ;)

comment:38 Changed 18 months ago by kkofler

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.

comment:39 Changed 18 months ago by kevin

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?

comment:40 Changed 17 months ago by halfie

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

comment:41 Changed 15 months ago by halfie

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

comment:42 Changed 15 months ago by kevin

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. ;)

comment:43 Changed 15 months ago by notting

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.

Note: See TracTickets for help on using tickets.