Closed Bug 620058 Opened 14 years ago Closed 7 years ago

Add --enable-hardening configure flag for hardened gcc and clang builds

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: db.pub.mail, Assigned: Alex_Gaynor)

References

(Blocks 5 open bugs)

Details

(Keywords: sec-want, Whiteboard: [sg:want][adv-main55-])

Attachments

(1 file, 3 obsolete files)

Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101210 4
Build Identifier: Magic ponies! 

use hardened gcc build flags:
"Hi um could the default compile flags also include: -D_FORTIFY_SOURCE=2
-Wformat -Wformat-security  -fstack-protector

Also --> -pie -fPIE 

See http://wiki.debian.org/Hardening (among other sites) for as to why these
should be added."
...
"For the default firefox - from http://www.mozilla.com/en-US/firefox/ (32bit)

 Position Independent Executable: no, normal executable!
 Stack protected: no, not found!
 Fortify Source functions: no, not found!
 Read-only relocations: no, not found!
 Immediate binding: no, not found!

Here is firefox compiled on amd64 with hardening flags enabled:
 Position Independent Executable: yes
 Stack protected: yes
 Fortify Source functions: yes
 Read-only relocations: yes
 Immediate binding: yes"
...

Reproducible: Always




https://bugzilla.mozilla.org/show_bug.cgi?id=590181#c113
Component: General → Build Config
Product: Firefox → Core
QA Contact: general → build-config
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: major → normal
OS: Other → Linux
Version: unspecified → Trunk
(In reply to comment #0)
>  Read-only relocations: yes

This needs -Wl,-z,relro

>  Immediate binding: yes"

This needs -Wl,-z,now

I expect these ones to have a dramatic effect on startup performance. Plus, it does sound weird to keep using a PLT when its effect is made moot, since everything is bound at startup. The only upside of the PLT then is the fact that several calls to the same library function only require one relocation, but making a function call with a read and a jump remains, which is pretty much a pointless overhead with these 2 options applied.
Sure. However, I compiled firefox like this:

.mozconfig
...
export CFLAG=" -fstack-protector -fPIE -D_FORTIFY_SOURCE=2"
export CPPFLAG=" -fstack-protector -fPIE -D_FORTIFY_SOURCE=2"

and I also have this in my .bashrc
export DEB_BUILD_HARDENING=1
(In reply to comment #2)
> and I also have this in my .bashrc
> export DEB_BUILD_HARDENING=1

AIUI, that alone will enforce all flags, including -z relro and -z now.
(In reply to comment #3)
> AIUI, that alone will enforce all flags, including -z relro and -z now.

(but it's not portable, obviously)
main	sessionRestored	firstPaint	version	appBuildID
26.33	1268.00	1541.33	5.0	20110703145655
22	871	1150	5.0	20110703145655
28	1043	1319	5.0	20110703145655
29	1890	2155	5.0	20110703145655
49.67	1271.00	1651.00	5.0	20110702193025
33	1046	1422	5.0	20110702193025
32	1305	1677	5.0	20110702193025
84	1462	1854	5.0	20110702193025
319.00	3638.13	4140.75	5.0	20110701125147
33	1148	1522	5.0	20110701125147
35	1571	1924	5.0	20110701125147
36	1088	1459	5.0	20110701125147

Data from about:startup extension.
20110701125147 is 0.5-2 currently in experimental.
20110702193025 is 0.5-2 rebuilt with all options but -z relro and -z now.
20110703145655 is 0.5-2 rebuilt with all hardening options enabled.
it would seem that this is possibly applicable to OSX as well while we continue to build with gcc there
Depends on: 671426
it sounds like chrome nightly builds now are building with -fPIE on OSX now that Lion has better ASLR support.
Whiteboard: [sg:want]
(In reply to comment #7)
> it sounds like chrome nightly builds now are building with -fPIE on OSX now
> that Lion has better ASLR support.

It doesn't matter much for firefox, actually, because the program itself is an almost empty shell (except on linux where it contains jemalloc).
Blocks: exploit-mitigation
I build all of my TB and FF builds on OSX with -fstack-protector-all, without any issues. Today I've tried to build on Lion with -fPIE instead of -fPIC (changed all -fPIC  to -fPIE in the necessary files), and it worked (it builds fine and doesn't crash, the DMG is 1MB bigger). I've used Apples Clang for that. Can we introduce a --enable-hardening flag, so everybody who wants to build a hardened build can enable hardening (on every OS)?

TOR uses the following for a hardened build:
[if test x$enableval = xyes; then
    CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2 -fstack-protector-all"
    CFLAGS+=" -fwrapv -fPIE -Wstack-protector -Wformat -Wformat-security"
    CFLAGS+=" -Wpointer-sign"
    LDFLAGS+=" -pie"
fi])
(In reply to Nomis101 from comment #9)
> TOR uses the following for a hardened build:
> [if test x$enableval = xyes; then
>     CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2 -fstack-protector-all"
>     CFLAGS+=" -fwrapv -fPIE -Wstack-protector -Wformat -Wformat-security"
>     CFLAGS+=" -Wpointer-sign"
>     LDFLAGS+=" -pie"
> fi])

No sorry, this was the old one. It currently uses:
[if test x$enableval = xyes; then
    CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2 -fstack-protector-all"
    CFLAGS="$CFLAGS -fwrapv -fPIE -Wstack-protector"
    CFLAGS="$CFLAGS --param ssp-buffer-size=1"
    LDFLAGS="$LDFLAGS -pie"
fi])
Depends on: 725941
Attached patch Allow to enable a set of hardening flags (obsolete) — — Splinter Review 
This patch adds the same --enable-hardening option as in Bug 725941.
Depends on: 752139
No longer depends on: 752139
Depends on: 752293
No longer depends on: 752293
These hardening options negatively affect the performance, don't they? We should test how much Firefox would be slower with these patches to check if the game is worth the candle.
Attached file hardened flags (obsolete) — 
I'm running the attached patch personally.
that's relro, bind now, ssp, no pie. a pie patch may also be interesting (for ASLR) however (-fPIE -pie)
also, fixed indent from attachment 596081 [details] [diff] [review]

That's the output of checksec:
RELRO           STACK CANARY      NX            PIE             RPATH      RUNPATH      FILE
Full RELRO      Canary found      NX enabled    No PIE          No RPATH   No RUNPATH   ./obj-x86_64-unknown-linux-gnu/dist/bin/firefox

It had to put those options in .mozconfig:
ac_add_options --enable-hardening
ac_add_options --disable-elf-hack
Attached patch hardened flags (obsolete) — — Splinter Review 
Noticed I was forcing nx, while its not necessary. Basically in comparison to 596081 this adds bind now
Attachment #641281 - Attachment is obsolete: true
Comment on attachment 641295 [details] [diff] [review]
hardened flags

hi ted, I was told that you may be able to review this :)
Attachment #641295 - Flags: review?(ted.mielczarek)
Comment on attachment 641295 [details] [diff] [review]
hardened flags

You don't want -z now and -z relro.
Comment on attachment 641295 [details] [diff] [review]
hardened flags

I think glandium would be a better reviewer for this, actually.
Attachment #641295 - Flags: review?(ted.mielczarek) → review?(mh+mozilla)
Attachment #641295 - Attachment is patch: true
Comment on attachment 641295 [details] [diff] [review]
hardened flags

Review of attachment 641295 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +7612,5 @@
> +              AC_MSG_ERROR([--enable-hardening is not supported on non-GNU toolchain-defaults])
> +          fi
> +          CFLAGS="-fstack-protector --param=ssp-buffer-size=4 -Wformat -Wformat-security -Werror=format-security $CFLAGS"
> +          CPPFLAGS="-D_FORTIFY_SOURCE=2 $CPPFLAGS"
> +          LDFLAGS="-Wl,-z,now -Wl,-z,relro $LDFLAGS"

You should only add these linker flags if elfhack is disabled.
Attachment #641295 - Flags: review?(mh+mozilla) → review-
If I make it exclude the linker flags during configure and put a warning, I fear that many people will actually miss it, and believe they have a build using relro and bind now while they would not.

Since the elfhack option is default, and hardening is optional (must manually add the configure flag), I would suggest that, instead, I would make it die with a message explaining that you need to disable elfhack in order to have --hardening.

Another alternative I have in mind is to have a configure flag for SSP *and* a flag for relro bind now, so you could still have elfhack + ssp if you wanted. (the later would also stop configure with a message).

Which one would you prefer? (and if you still prefer the first solution, why?)

(note: one compilation option that is absent from all this and would be added via later patch is PIE, that would be -pie -fPIE and replacing some -fPIC by -fPIE..)
:glandium i need your reply to comment #19 to continue :)
Here's an idea: only allow --enable-hardening with an explicit choice for elhack. with --enable-elf-hack, you disable relro, with --disable-elf-hack, you enable it. With neither, you error out.
By the way, without those LDFLAGS this builds also just fine on Mac.
Chromium requires immediate binding since the linker would be unable to perform lazy binding from within the sandbox. Enabling *partial* RELRO (-Wl,-z,relro) is low overhead, and since there's immediate binding for another reason it gets full RELRO for free. They're also enabling PIE by default, and that's certainly important because even a few seemingly harmless functions can be very useful for ROP.
Is this on anybodies radar? Some try build & QA would be nice to assess performance impact (maybe with different "hardening levels"?). Judging by the comments the work required to set this up is minimal so it's not such a waste of time if we find the performance impact is too high.
Component: Build Config → Security
OS: Linux → All
Summary: use hardened gcc build flags → use hardened gcc & clang build flags
The performance impact of these flags has been looked into in depth by various Linux distributions for the default CFLAGS. There's a minimal impact from enabling RELRO and PIE. Turning on -fstack-protector (or better, -fstack-protector-strong like Chromium) has a 1-5% performance hit and a 1-10% code size increase but prevents many buffer overflow exploits.

Using immediate binding will be a start-up time regression, but it's a prerequisite for a real sandbox. It's not possible for the linker to do lazy binding from within an isolated environment.
On Mac I'm using -fPIE instead of -fPIC (changed all instances of -fPIC in the code to -fPIE) for my own builds for 1-2 years now. And -fstack-protector-all for CXXFLAGS and CFLAGS and -D_FORTIFY_SOURCE=2 for CPPFLAGS. This builds and works fine, no drawbacks for now.
About PIE:
https://bugzilla.mozilla.org/show_bug.cgi?id=857628#c1

(In reply to Daniel Micay from comment #25)
> Using immediate binding will be a start-up time regression, but it's a
> prerequisite for a real sandbox. It's not possible for the linker to do lazy
> binding from within an isolated environment.

Do you have a link explaining that? Because I don't see why, since the linker doesn't need system calls to do lazy binding.
(In reply to Nomis101 from comment #26)
> On Mac I'm using -fPIE instead of -fPIC (changed all instances of -fPIC in
> the code to -fPIE) for my own builds for 1-2 years now. And
> -fstack-protector-all for CXXFLAGS and CFLAGS and -D_FORTIFY_SOURCE=2 for
> CPPFLAGS. This builds and works fine, no drawbacks for now.

You need to link the binary with -pie for PIE, -fPIE is just to generate the position independent code for it and it's likely not much different (if at all?) from -fPIC.
(In reply to Mike Hommey [:glandium] from comment #27)
> About PIE:
> https://bugzilla.mozilla.org/show_bug.cgi?id=857628#c1
> 
> (In reply to Daniel Micay from comment #25)
> > Using immediate binding will be a start-up time regression, but it's a
> > prerequisite for a real sandbox. It's not possible for the linker to do lazy
> > binding from within an isolated environment.
> 
> Do you have a link explaining that? Because I don't see why, since the
> linker doesn't need system calls to do lazy binding.

The immediate binding provides full RELRO to eliminate classes of exploits involving mutation of those global tables and other ELF data. I thought it was also required to avoid breaking the sandbox rules, but it seems I misinterpreted some conversations about the weird linker they use on Android.[1]

[1] https://android.googlesource.com/platform/ndk/+/master/sources/android/crazy_linker/README.TXT
(In reply to Daniel Micay from comment #28)
> (In reply to Nomis101 from comment #26)
> > On Mac I'm using -fPIE instead of -fPIC (changed all instances of -fPIC in
> > the code to -fPIE) for my own builds for 1-2 years now. And
> > -fstack-protector-all for CXXFLAGS and CFLAGS and -D_FORTIFY_SOURCE=2 for
> > CPPFLAGS. This builds and works fine, no drawbacks for now.
> 
> You need to link the binary with -pie for PIE, -fPIE is just to generate the
> position independent code for it and it's likely not much different (if at
> all?) from -fPIC.

I also do -fpie for LDFLAGS, do you mean that?
(In reply to Nomis101 from comment #30)
> (In reply to Daniel Micay from comment #28)
> > (In reply to Nomis101 from comment #26)
> > > On Mac I'm using -fPIE instead of -fPIC (changed all instances of -fPIC in
> > > the code to -fPIE) for my own builds for 1-2 years now. And
> > > -fstack-protector-all for CXXFLAGS and CFLAGS and -D_FORTIFY_SOURCE=2 for
> > > CPPFLAGS. This builds and works fine, no drawbacks for now.
> > 
> > You need to link the binary with -pie for PIE, -fPIE is just to generate the
> > position independent code for it and it's likely not much different (if at
> > all?) from -fPIC.
> 
> I also do -fpie for LDFLAGS, do you mean that?

No, PIE is enabled by passing -pie (not -fpie/-fPIE) while linking. Building with -fPIE is just used to generate position independent code, much like -fPIC. The difference between -fPIC and -fPIE is likely very small with `gcc`, and I don't think there *is* a difference beyond the defined macros with `clang`.
(In reply to Daniel Micay from comment #31)
> (In reply to Nomis101 from comment #30)
> > (In reply to Daniel Micay from comment #28)
> > > (In reply to Nomis101 from comment #26)
> > > > On Mac I'm using -fPIE instead of -fPIC (changed all instances of -fPIC in
> > > > the code to -fPIE) for my own builds for 1-2 years now. And
> > > > -fstack-protector-all for CXXFLAGS and CFLAGS and -D_FORTIFY_SOURCE=2 for
> > > > CPPFLAGS. This builds and works fine, no drawbacks for now.
> > > 
> > > You need to link the binary with -pie for PIE, -fPIE is just to generate the
> > > position independent code for it and it's likely not much different (if at
> > > all?) from -fPIC.
> > 
> > I also do -fpie for LDFLAGS, do you mean that?
> 
> No, PIE is enabled by passing -pie (not -fpie/-fPIE) while linking. Building
> with -fPIE is just used to generate position independent code, much like
> -fPIC. The difference between -fPIC and -fPIE is likely very small with
> `gcc`, and I don't think there *is* a difference beyond the defined macros
> with `clang`.

OK, thanks. I've tried that, works on Mac too.
(In reply to Mike Hommey [:glandium] from comment #27)
> About PIE:
> https://bugzilla.mozilla.org/show_bug.cgi?id=857628#c1
> 
> (In reply to Daniel Micay from comment #25)
> > Using immediate binding will be a start-up time regression, but it's a
> > prerequisite for a real sandbox. It's not possible for the linker to do lazy
> > binding from within an isolated environment.
> 
> Do you have a link explaining that? Because I don't see why, since the
> linker doesn't need system calls to do lazy binding.

To do lazy binding you need a writable GOT... which is exactly what you don't want security-wise.

Immediate binding is useless if you don't have a read-only GOT... So step1 is relro, step2 is bindnow

The following article explains what GOT overwrites are about:
https://isisblogs.poly.edu/2011/06/01/relro-relocation-read-only/
I'd like to see this ticket move forward... Can we at least enable the options no one objects about? (FORTIFY, -fstack-protector-strong, format-warnings, ...)

Can we consider enabling options on a per-achitecture basis? ASLR/PIE is dirt-cheap on amd64... and that's what most people use (http://seclists.org/oss-sec/2014/q1/319)
What actually needs to happen to get the Florent's subset merged?
(In reply to jvoisin from comment #35)
> What actually needs to happen to get the Florent's subset merged?

I think Florent's approach of enabling the options to which there are no objections (so far) is probably the best one, however whoever chooses to take on that task will almost certainly have to provide performance testing results to be able to quantify any performance hit and enable discussions of whether the hit is acceptable or not. So someone would have to write that patch (or take one of the attached patches and address all the previous feedback in the bug), conduct the performance testing and attach results in this bug, and then drive it through review with an appropriate reviewer (probably :glandium based on previous comments/reviews) and then get it landed. 

In fact, whoever wants to take this on should probably file another bug and mark this bug dependent on it that specifically calls out which theoretically not-objected-to options it's trying to enable. One of the reasons this bug has stalled out multiple times is probably that 'hardened build flags' is pretty vague and hence ripe for debate over WHICH flags and what their values should be. Perfect is the enemy of the good and all that.
Chromium uses RELRO, immediate binding (for full RELRO), PIE (for ASLR) and -fstack-protector-strong. The only other flag that's really relevant is -D_FORTIFY_SOURCE=2. I realize that Firefox doesn't have a focus on security (no JIT hardening, no sandboxing) but it's embarrassing to lack ASLR in 2014.
Well, reading the bug comments back, the only options I've ever seen objections to, are the linker ones: specifically "-z relro and -z bindnow".

I suggest that the following are enabled *by default* where supported:
CFLAGS= -fstack-protector-strong -fPIE -D_FORTIFY_SOURCE=2 -fwrapv -Wstack-protector -Wformat -Wformat-security -Wpointer-sign
LDFLAGS+= -pie

and when -fstack-protector-strong is unavailable (old GCC), it'd be replaced by
CFLAGS+= -fstack-protector --param ssp-buffer-size=4

...

I also suggest that depending on the elfhack flag, we do or do not enable the following contentious linker-hardening options (as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=620058#c21)
LDFLAGS+= -Wl,-z,now -Wl,-z,relro

According to Mike's own benchmarks (https://bugzilla.mozilla.org/show_bug.cgi?id=606145#c40), elfhack wins ~200ms on startup time, so any cost incurred adds up to it... I know that I prefer my firefox to have linker-hardening options than having it start faster, but I respect that it might not be the case for everyone... and that the default behavior is is a policy decision rather than anything else.

Is everyone happy with the above? Can we get away with *NOT* having an option for disabling hardening flags?
(In reply to Florent Daigniere from comment #38)
 > and when -fstack-protector-strong is unavailable (old GCC), it'd be replaced

Does Clang have support for -fstack-protector-strong?
(In reply to Nomis101 from comment #39)
> (In reply to Florent Daigniere from comment #38)
>  > and when -fstack-protector-strong is unavailable (old GCC), it'd be
> replaced
> 
> Does Clang have support for -fstack-protector-strong?

Not yet. It requires GCC 4.9+
(In reply to Florent Daigniere from comment #40)
> (In reply to Nomis101 from comment #39)
> > (In reply to Florent Daigniere from comment #38)
> >  > and when -fstack-protector-strong is unavailable (old GCC), it'd be
> > replaced
> > 
> > Does Clang have support for -fstack-protector-strong?
> 
> Not yet. It requires GCC 4.9+

Err, since 3.5 it does.
http://lists.freedesktop.org/archives/mesa-dev/2014-May/059224.html
You could check for -fstack-protector-strong with an autoconf test and fall back to -fstack-protector.

Using PIE is the single most important change to get ASLR for the sbrk base (although jemalloc doesn't use it by default), code in the binary and ELF data (GOT, PLT). There's next to no benefit from ASLR without PIE. Using RELRO would make the PLT/GOT data read-only, but there's still more than enough non-randomized functions to get remote code execution.

It has the same performance impact as moving code to dynamic libraries. On x86_64, that's negligible, and on i386 it can range in impact. However, since Firefox has nearly everything in dynamic libraries, it's already paying this price without reaping the benefits of full ASLR.

Using *partial* RELRO has a negligible performance impact but it's not really valuable without immediate binding, and that's where you get a start-up time hit.
(In reply to Daniel Micay from comment #42)
> Using PIE is the single most important change to get ASLR for the sbrk base
> (although jemalloc doesn't use it by default), code in the binary and ELF
> data (GOT, PLT). There's next to no benefit from ASLR without PIE. Using
> RELRO would make the PLT/GOT data read-only, but there's still more than
> enough non-randomized functions to get remote code execution.

Firefox is not, like Chrome, a single executable binary. It's a small executable, with dynamic libraries. So effectively, 98% of Firefox is already ASLRed. Only the small "firefox" executable is not. And PIE for that executable is bug 857628. Please read comment 1 there.
> Firefox is not, like Chrome, a single executable binary. It's a small executable, with dynamic libraries. So effectively, 98% of Firefox is already ASLRed. Only the small "firefox" executable is not. And PIE for that executable is bug 857628. Please read comment 1 there.

Partial ASLR that doesn't have any value. All of the code in the binary (including jemalloc) can be used as ROP gadgets. Since Firefox doesn't use RELRO, the GOT is a mutable table of function pointers in a known location too. So a lot of the function addresses for those dynamic libraries are at known locations and are *mutable* function pointers.
Here's a simple example:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void local() {}

int main(void) {
    printf("%p %p %p\n", strdup, free, local);
    return 0;
}

If -fPIE or -fPIC isn't used, the free and strdup functions end up at known static locations. Randomizing the address of `local` requires the addition of `-pie`, and that applies to plenty of code in the Firefox executable.
> Partial ASLR that doesn't have any value.

You know, complete ASLR doesn't have as much value as you would like it to have. That said, debating this over and over is not going to change the current state of affairs, which is that enabling PIE *breaks* Firefox. So until someone actually figures what the hell is going on, there is no changing that. Or who knows, maybe the problem is gone by time passing by (and code changing), in which case we'd better figure what the problem was anyways so as to ensure it won't happen again.
See Also: → 857628, 1018210
(In reply to Mike Hommey [:glandium] from comment #46)
> > Partial ASLR that doesn't have any value.
> 
> You know, complete ASLR doesn't have as much value as you would like it to
> have. That said, debating this over and over is not going to change the
> current state of affairs, which is that enabling PIE *breaks* Firefox. So
> until someone actually figures what the hell is going on, there is no
> changing that. Or who knows, maybe the problem is gone by time passing by
> (and code changing), in which case we'd better figure what the problem was
> anyways so as to ensure it won't happen again.

I don't understand what you're trying to convey here, ASLR is certainly a useful exploit mitigation. It makes finding ROP gadgets significantly harder. Partial ASLR has next to no security value at all, especially since the GOT is completely mutable without RELRO so you can find and redirect many functions from dynamic libraries.

Ubuntu and Hardened Gentoo build Firefox as PIE, among other distributions, and it compiles just fine as a position independent executable when I do it locally.
(In reply to Daniel Micay from comment #47)
> Ubuntu and Hardened Gentoo build Firefox as PIE, among other distributions,
> and it compiles just fine as a position independent executable when I do it
> locally.

Go read bug 857628 comment 1.
Now that there is an option (https://hg.mozilla.org/mozilla-central/rev/d38de091ced0) to build Firefox with pie support, can we please have a version compiled with hardening flags enabled?
Enabling PIE leads to bug 1076892, which has not yet been resolved in file/libmagic.
Bug 1079662 will track re-enabling once a workaround has been found or upstream distributes a fix.

To enable other hardening flags, someone would apparently have to incorporate glandiums review comment 18/21 and "-fstack-protector-strong"+configure check into the latest patch, get it in the tree and provide performance & functionality data using try-runs.
Now that the mozilla builds use gcc 4.9 for firefox aurora and nightly -fstack-protector-strong can easily be used for official builds.
I've gone ahead and uploaded a review for the -fstack-protector-strong portion of this. Here's some Talos data: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=df0936e00966&newProject=try&newRevision=8a8c1c09c4d036bd18612e39efbb1acb9e2c3e4e&framework=1&showOnlyImportant=0

A thing I (very happily) learned while putting this together is that Ubuntu's GCC (at least on Xenial) enables -fstack-protector-strong for everything it builds.
Attachment #8858300 - Flags: review?(tom)
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review133354

::: old-configure.in:557
(Diff revision 1)
>  if test -n "$COMPILE_ENVIRONMENT"; then
>     MOZ_CONFIG_SANITIZE
>  fi
>  
>  dnl ========================================================
> +dnl Clang/GCC specific defaults

Are you sure you don't need this in js/src/old-configure.in also?
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review133354

> Are you sure you don't need this in js/src/old-configure.in also?

I think it probably is necessary get everything; however `js/src/old-configure.in` currently has `-fno-stack-protector`, dating back to https://hg.mozilla.org/mozilla-central/rev/663a3afb238e. Ideally, I'd like to disentangle getting this enabled for most things with tracking down whether the commit message there is still applicable.
I see there are quite a few performance regressions here:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=df0936e00966&newProject=try&newRevision=8a8c1c09c4d0&framework=1

this is comparing osx10.10 and linux64 (ubuntu 12.04).  OSX has a dromaeo regression and a11y.  

Linux has many regressions, the larger ones are tp5 (regular page load, also the responsiveness measure here), a11y, tart (tab animation).  Please consider getting more people to sign off on these performance differences.
I'm not totally sure who the right folks to sign off (or not :P) on the balance of concerns here - I'll talk to Selena and a few people, but if you (or anyone else) has someone in mind it'd be great if you could ni? or r? them.

In terms of techniques that might be used to reduce the performance impact, we can whitelist individual functions to not have stack-cookie-checks inserted if, for example, Quantum Flow profiling showed pain points. Doing this requires pretty deep knowledge of which functions merit the performance improvement / our confidence in their safety.

I'll also flag (pun intended) that the "firefox" one gets from the downstream Ubuntu repo comes with -fstack-protector-strong, so that segment of the user base won't be impacted by this.

As for why this is important, linear stack-based buffer overwrites are relatively straightforward to turn into exploits, by overwriting the return pointer on the stack. Stack cookies significantly increases the difficulty of turning one of these bugs into an exploit.

Using this quick search of bugzilla: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20stack%20keywords%3Acsectype-bounds&list_id=13540309 we have 12 public bugs touched in the past year that show up as a bounds issue with "stack", and 6 this calendar year. So this mitigation targets a real class of vulnerability.

RAND's recent report on 0-day vulnerability exploitation (https://www.rand.org/pubs/research_reports/RR1751.html) asserts that the shift from exploits targeting stack buffers to heap buffers was driven by exploit mitigations like this one.
:ritu, would you know who we could chat with about getting more information on accepting this regression?  Possibly looking at our linux users and seeing what percentage is not on Ubuntu would be a good way to reduce the scope here.
Flags: needinfo?(rkothari)
We don't need -D_FORTIFY_SOURCE=2 in this patch?
My goal was to make this patch as narrow as possible -- there can be follow up patches for each of the other hardening flags, but IMO putting them all together makes it harder to review since each individual hardening option might have different tradeoffs or impacts. As various comments here discuss, there are a range of different hardening flags we probably want to at least consider enabling.
(In reply to Joel Maher ( :jmaher) from comment #59)
> :ritu, would you know who we could chat with about getting more information
> on accepting this regression?  Possibly looking at our linux users and
> seeing what percentage is not on Ubuntu would be a good way to reduce the
> scope here.

Redirect to Sylvestre. Thanks!

I know that we are establishing release criteria for 57/Quantum and those specifically include "no regressions on TP5". Are the regressions in comment 57 also affecting windows?
Flags: needinfo?(rkothari) → needinfo?(sledru)
Can we have this discussion for 58?
I don't think we should spend time on this right now in the quantum context. 
It has been in this state for a while, I don't think we should work on that right now.

FYI, Debian builds Firefox with -fstack-protector-strong & -D_FORTIFY_SOURCE=2 (but Debian does NOT care about performances of the package generated).
Flags: needinfo?(sledru)
Until 58, we can probably a new configure file like "--enable-hardening" (disabled by default) for people who wants to use it

FYI bis, if Firefox is built on Ubuntu using the gcc of the distro, -fstack-protector & -D_FORTIFY_SOURCE=2 are enabled in gcc by default (but probably not with clang)
No, this does not affect Windows (unless you use clang or gcc on Windows, but I don't think that's really a thing people do).

It sounds like the right thing for me to do is put these behind a |--enable-hardening| flag for now, and we can revisit making it the default post-58?
Yes, please!
Attachment #8858300 - Flags: review?(sledru)
I am not an owner but I think we want this kind of changes to be in the python configure now.
Attachment #8858300 - Flags: review?(sledru)
Assignee: nobody → agaynor
(In reply to Sylvestre Ledru [:sylvestre] from comment #68)
> I am not an owner but I think we want this kind of changes to be in the
> python configure now.

We do...but we also don't have a great way of adding things to CFLAGS/CXXFLAGS from Python configure.

One thing that might work is to define HARDENING_CFLAGS from Python configure, and then add it into CFLAGS in old-configure.in, probably somewhere around:

http://dxr.mozilla.org/mozilla-central/source/old-configure.in#5180

so you'd get most of the win of Python configure.  You'd have to be careful to ensure that you wound up with the hardened CFLAGS everywhere that matters--I'm assuming we don't want to compile just Firefox with said flags, but also NSS, NSPR, ICU, etc.

Not sure which way is really better at this point.

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #65)
> No, this does not affect Windows (unless you use clang or gcc on Windows,
> but I don't think that's really a thing people do).

People do indeed use GCC on Windows for mingw targets (e.g. Tor), and we have clang builds running in automation.  But declaring such configurations out-of-bounds for this initial work is probably OK, though I suspect the Tor people may want mingw builds to use --enable-hardening...
Attachment #8858300 - Flags: review?(nfroyd)
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review135794

I think `build/moz.configure/toolchain.configure` might be a better place for this, as that would address the artifact build problems.

::: moz.configure:376
(Diff revision 3)
> +option('--enable-hardening', env='MOZ_SECURITY_HARDENING',
> +       help='Enables security hardening compiler options')

Do you want this to only be defined for Firefox proper, or do you want it to be available for a standalone JS build as well?  The latter would require making this `js_option` as well as possibly moving the checks.

::: moz.configure:379
(Diff revision 3)
> +@depends('--enable-hardening', c_compiler)
> +def security_hardening_cflags(value, c_compiler):

Putting this here isn't going to work for artifact builds, which don't check for compilers and therefore don't define `c_compiler`.

::: moz.configure:381
(Diff revision 3)
> +    if value and c_compiler.type in ['gcc', 'clang', 'clang-cl']:
> +        return '-fstack-protector-strong'

Please be sure to run this through try; `clang-cl` ought to accept `-fstack-protector-strong`, but it might not.
Attachment #8858300 - Flags: review?(nfroyd) → review-
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review135796

::: moz.configure:383
(Diff revision 3)
> +
> +@depends('--enable-hardening', c_compiler)
> +def security_hardening_cflags(value, c_compiler):
> +    if value and c_compiler.type in ['gcc', 'clang', 'clang-cl']:
> +        return '-fstack-protector-strong'
> +    else:

The else is useless ;)

::: old-configure.in:556
(Diff revision 3)
>  
>  if test -n "$COMPILE_ENVIRONMENT"; then
>     MOZ_CONFIG_SANITIZE
>  fi
>  
> +# Add the hardening flags from moz.configure

Maybe it would be better in toolchain.configure
Attachment #8858300 - Flags: review?(sledru) → review-
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review135794

> Do you want this to only be defined for Firefox proper, or do you want it to be available for a standalone JS build as well?  The latter would require making this `js_option` as well as possibly moving the checks.

Eventually extending this to JS might be good, but right now js has an explicit `-fno-stack-protector` (which dates back several years, and at the time was said to have fixed a bug), so I don't want to make landing this block on disentangling that.
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review135794

> Please be sure to run this through try; `clang-cl` ought to accept `-fstack-protector-strong`, but it might not.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef64dddce7b30598dc37f32faeb34d5949761a01

Hmmm.... it doesn't appear to like the windows clang builds that try generated.
Attachment #596081 - Attachment is obsolete: true
Attachment #641295 - Attachment is obsolete: true
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #75)
> Comment on attachment 8858300 [details]
> Bug 620058 - Add a --enable-hardening flag, which compiles with
> -fstack-protector-strong on GCC and Clang
> 
> https://reviewboard.mozilla.org/r/130278/#review135794
> 
> > Please be sure to run this through try; `clang-cl` ought to accept `-fstack-protector-strong`, but it might not.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ef64dddce7b30598dc37f32faeb34d5949761a01
> 
> Hmmm.... it doesn't appear to like the windows clang builds that try
> generated.

You want win{32,64}-st-an, I think.
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

https://reviewboard.mozilla.org/r/130278/#review136346

r=me assuming the clang-cl bits get thmselves sorted out.
Attachment #8858300 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #77)
> (In reply to Alex Gaynor [:Alex_Gaynor] from comment #75)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ef64dddce7b30598dc37f32faeb34d5949761a01
> > 
> > Hmmm.... it doesn't appear to like the windows clang builds that try
> > generated.
> 
> You want win{32,64}-st-an, I think.

Ok, triggered those: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f779dec927b4c443830afde0e565f77487aaf3c they look like they're static analysis runs though, are those secretly also the cl-clang builds?
Flags: needinfo?(nfroyd)
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #79)
> Ok, triggered those:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6f779dec927b4c443830afde0e565f77487aaf3c they look
> like they're static analysis runs though, are those secretly also the
> cl-clang builds?

That's correct, those are the clang-cl builds.
Flags: needinfo?(nfroyd)
Thanks! And good call on hitting the try: "clang.exe: warning: unknown argument ignored in clang-cl: '-fstack-protector-strong' [-Wunknown-argument]" I'll go ahead and drop that from list of compilers.
Comment on attachment 8858300 [details]
Bug 620058 - Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang

:froydnj is a build peer and reviewed the change; dropping the second r?.
Attachment #8858300 - Flags: review?(sledru)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4f091d53060c
Add a --enable-hardening flag, which compiles with -fstack-protector-strong on GCC and Clang r=froydnj
Keywords: checkin-needed
Thanks for this patch! \o/
https://hg.mozilla.org/mozilla-central/rev/4f091d53060c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reopening this and giving it the leave-open keyword as there are many flags we would like to apply.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
(In reply to Tom Ritter [:tjr] from comment #88)
> Reopening this and giving it the leave-open keyword as there are many flags
> we would like to apply.

Any reason those can't be separate bugs?  We've added the infrastructure here, and keeping bugs open for indeterminate amounts of time is problematic for many reasons.
Flags: needinfo?(tom)
(In reply to Nathan Froyd [:froydnj] from comment #89)
> (In reply to Tom Ritter [:tjr] from comment #88)
> > Reopening this and giving it the leave-open keyword as there are many flags
> > we would like to apply.
> 
> Any reason those can't be separate bugs?  We've added the infrastructure
> here, and keeping bugs open for indeterminate amounts of time is problematic
> for many reasons.

No, they can be. I'll try and break them out.
Flags: needinfo?(tom)
Okay, I opened a bunch of bugs for each flag I could find. I'm flagging Alex to have him double check I did everything. In particular I didn't open one for PIE, I'm not sure what the story is there. 

If anyone wants to follow the new bugs, I did not auto-add you as a CC on them, so you have to do it yourself.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: needinfo?(agaynor)
Resolution: --- → FIXED
Summary: use hardened gcc & clang build flags → Add --enable-hardening configure flag for hardened gcc and clang builds
:tjr, why no bug for PIE, it is probably the biggest protection out of the ones that the compiler provides and definitely requires an information leak to further exploit a vulnerability to gain full code execution.
I think we should have at least have a bug to see how feasible it is.
Flags: needinfo?(tom)
We already support PIE with https://dxr.mozilla.org/mozilla-central/source/build/autoconf/compiler-opts.m4#253-280

I think --enable-hardening should imply --enable-pie (new issue for this?), that flag should support Clang (new issue?). It sounds like there are blockers to enabling it by default -- like the mentioned nautilus bug. Does anyone know if there's an upstream issue tracking that?
Flags: needinfo?(tom)
Flags: needinfo?(agaynor)
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #93)
> We already support PIE with
> https://dxr.mozilla.org/mozilla-central/source/build/autoconf/compiler-opts.
> m4#253-280
> 
> I think --enable-hardening should imply --enable-pie (new issue for this?),
> that flag should support Clang (new issue?). It sounds like there are
> blockers to enabling it by default -- like the mentioned nautilus bug. Does
> anyone know if there's an upstream issue tracking that?

https://bugzilla.gnome.org/show_bug.cgi?id=737849
Whiteboard: [sg:want] → [sg:want][adv-main55-]
Target Milestone: mozilla55 → ---
Blocks: 1492917
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: