Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add a --enable-hardening flag to enable stack protection and enhanced buffer overflow protection #2027

Open
Nomis101 opened this issue Apr 8, 2019 · 11 comments

Comments

5 participants
@Nomis101
Copy link
Contributor

commented Apr 8, 2019

This feature request is to add a flag "--enable-hardening" for all platforms which will include -fstack-protector-strong and -D_FORTIFY_SOURCE=2 to CFLAGS, CPPFLAGS and CXXFLAGS to enable a buffer overflow protection. See (1) and (2) for details. This idea is adopted from Mozilla where this is enabled for Firefox and Thunderbird (3, 4) by default.

I don't know for Win and Linux, but on macOS, the weaker -fstack-protector and -D_FORTIFY_SOURCE=1 is enabled by default (5).

My plan was to come up with a working patch. But I'm a bit stuck. I found out, adding --enable-hardening as a grp.add_option to configure.py is the first step. And -fstack-protector-strong and -D_FORTIFY_SOURCE=2 can be added to gcc.defs. But I don't know how to link both.
But you need to tell me, if you like that idea in general...

(1) https://en.wikipedia.org/wiki/Buffer_overflow_protection
(2) https://access.redhat.com/blogs/766093/posts/1976213
(3) https://bugzilla.mozilla.org/show_bug.cgi?id=620058
(4) https://bugzilla.mozilla.org/show_bug.cgi?id=1359908
(5) https://developer.apple.com/library/archive/documentation/Security/Conceptual/SecureCodingGuide/Articles/BufferOverflows.html#//apple_ref/doc/uid/TP40002577-SW26

@jstebbins

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

You can look at make/include/main.defs to see how we handle other similar things. For example:

ifeq (1,$(FEATURE.qsv))
    MODULES += contrib/libmfx
endif

The FEATURE.* variables are written to build/GNUMakefile during configure. There is also a collection of BUILD.* variables that get written, I would make your new build option something like BUILD.harden.

Then in make/include/gcc.defs you need to add the hardening options to GCC.args.extra when BUILD.harden is 1.

@Nomis101

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Thanks, will try that. :-)

@Nomis101

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

How do I add the attribute 'harden' to the object 'BuildAction'? I'm always getting

Traceback (most recent call last):
File "./make/configure.py", line 1835, in
doc.add( 'BUILD.harden', build.harden )
AttributeError: 'BuildAction' object has no attribute 'harden'

I'm not very skilled in python. My current effort looks like

diff --git a/make/configure.py b/make/configure.py
index d25400e38..8e41bea9c 100644
--- a/make/configure.py
+++ b/make/configure.py
@@ -1360,6 +1360,8 @@ def createCLI():
     h = IfHost( 'specify Mac OS X deployment target for Xcode builds', '*-*-darwin*', none=optparse.SUPPRESS_HELP ).value
     grp.add_option( '--minver', default=None, action='store', metavar='VER',
         help=h )
+    h = IfHost( 'enable buffer overflow protection', '*-*-*', none=optparse.SUPPRESS_HELP ).value
+    grp.add_option( '--enable-hardening', dest="enable_build_harden", default=None, action='store_false', help=h )
     cli.add_option_group( grp )
 
     ## add Xcode options
@@ -1830,6 +1832,7 @@ int main()
     doc.addBlank()
     doc.add( 'BUILD.spec',    build.spec )
     doc.add( 'BUILD.machine', build.machine )
+    doc.add( 'BUILD.harden',  build.harden )
     doc.add( 'BUILD.vendor',  build.vendor )
     doc.add( 'BUILD.system',  build.system )
     doc.add( 'BUILD.systemf', build.systemf )
diff --git a/make/include/gcc.defs b/make/include/gcc.defs
index 83b26aafa..8101d28f5 100644
--- a/make/include/gcc.defs
+++ b/make/include/gcc.defs
@@ -73,11 +73,16 @@ GCC.args.L         = -L$(1)
 GCC.args.l         = -l$(1)
 GCC.args.end       = -Wl,--end-group
 
ifeq ($(BUILD.machine),$(filter $(BUILD.machine),i686 x86_64))
    GCC.args.extra = $(CFLAGS) $(CPPFLAGS) -mfpmath=sse -msse2
 else
     GCC.args.extra = $(CFLAGS) $(CPPFLAGS)
 endif
+
+ifeq (1,$(BUILD.harden))
+    GCC.args.extra = $(CFLAGS) $(CXXFLAGS) $(CPPFLAGS) -fstack-protector-strong -D_FORTIFY_SOURCE=2
+endif
+
 GCC.args.extra.h_o     =
 GCC.args.extra.c_o     =
 GCC.args.extra.dylib   = $(LDFLAGS)

Something is missing somewhere...

@jstebbins

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

You don't need to modify the build BuildAction object. Just use the options object like the other FEATURES.* options do, but write a BUILD.harden assignment to GNUMakefile. e.g.

doc.add( 'BUILD.harden', options.harden )

Something similar is done for BUILD.cross

@Nomis101

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

I have a working patch now :-)
Nomis101@6fbe879

But I think for the PR I will wait after # 2021 landed and I have converted my patch accordingly.

@bradleysepos

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Are there drawbacks to consider, performance or otherwise? If negligible, perhaps we simply enable by default, no option necessary.

@Nomis101

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Are there drawbacks to consider, performance or otherwise?

I don't know any that would affect Handbrake. At least not on Mac. I'm using this for quite some time now for my own builds. But I haven't tested this on any other OS than macOS. I know, that Chromium (1, 2) is using this per default (on Mac) and Mozilla (3) for all platforms (Mac, Windows, Linux, on Android for js), also Gentoo (4) seems to use it. I've read, it should be disabled if ASAN is used.
It should also be noted, that -D_FORTIFY_SOURCE=2 is only fully enabled if the source is compiled with -O1 or higher.

(1) https://bugs.chromium.org/p/chromium/issues/detail?id=93615
(2) https://bugs.chromium.org/p/chromium/issues/detail?id=196719
(3) https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#1604
(4) https://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo/src/patchsets/gcc/5.4.0/gentoo/10_all_default-fortify-source.patch?view=markup

@cehoyos

This comment has been minimized.

Copy link

commented Apr 12, 2019

There is definitely a performance impact.

@Nomis101

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I was always told -fstack-protector-all gives a performance impact and that's why -fstack-protector-strong was introduced (1). Correct me if I'm wrong. Do you know any numbers? I couldn't find any.
For -D_FORTIFY_SOURCE=2 what I know is, that it should have no measurable performance impact (2). Also correct me here if I'm wrong. I could make some benchmarking tests with and without that flags, if that helps.

(1, at 6.3.1) https://nvlpubs.nist.gov/nistpubs/TechnicalNotes/NIST.TN.1860.pdf
(2) https://wiki.debian.org/Hardening#gcc_-D_FORTIFY_SOURCE.3D2_-O1

@cehoyos

This comment has been minimized.

Copy link

commented Apr 12, 2019

Why would checking the canary before returning from the function be free?
From an FFmpeg point-of-view many functions are not written in C, they cannot be protected by the compiler and do not cause any speed penalty. For other functions - that may or may not be speed-relevant - there can be a penalty.

@cehoyos

This comment has been minimized.

Copy link

commented Apr 12, 2019

I measure about 1% penalty for -fstack-protector-strong alone, more for -fstack-protector-all.

@sr55 sr55 added the Enhancement label Apr 14, 2019

@sr55 sr55 added this to the 1.3.0 milestone Apr 14, 2019

Nomis101 added a commit to Nomis101/HandBrake that referenced this issue Apr 22, 2019

Add a --enable-hardening flag to enable stack protection and enhanced…
… buffer overflow protection

This is the PR for HandBrake#2027. It adds a --enable-hardening flag. It prints the hardening status on global init to the log. Was PR HandBrake#2040.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.