Closed Bug 1359908 Opened 3 years ago Closed 3 years ago

Add FORTIFY_SOURCE security flag

Categories

(Core :: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: Nomis101)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file, 3 obsolete files)

This bug was created as a clone of Bug 620058 which has more context.
Attached patch Add FORTIFY_SOURCE security flag (obsolete) — Splinter Review
This patch adds -D_FORTIFY_SOURCE=2 to the set of hardening flags. Four different cases are possible, see: https://gcc.gnu.org/ml/gcc-patches/2004-09/msg02055.html
Most used is level 2, seems also the best choice here.
On macOS level 1 is enabled by default when compiling for Mac OS X v10.6 and later:  https://developer.apple.com/library/content/documentation/Security/Conceptual/SecureCodingGuide/Articles/BufferOverflows.html#//apple_ref/doc/uid/TP40002577-SW26
Assignee: nobody → Nomis101
Attachment #8864320 - Flags: review?(nfroyd)
Thanks for taking a look at this one!

Before we add this to the hardening flags, we should test to see if there's a negative performance impact to enabling this; if not it should probably be on by default.
OK. I do not have access to the try server. I've just checked locally that it builds properly.
Comment on attachment 8864320 [details] [diff] [review]
Add FORTIFY_SOURCE security flag

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

Thanks, r=me with the change below assuming that it builds correctly.

Note that as Alex has said, we'll need a try run to sort out any performance issues.

::: build/moz.configure/toolchain.configure
@@ +1004,5 @@
>  
>  @depends('--enable-hardening', c_compiler)
>  def security_hardening_cflags(value, c_compiler):
>      if value and c_compiler.type in ['gcc', 'clang']:
> +        return '-fstack-protector-strong -D_FORTIFY_SOURCE=2'

We want to make this a list now that we have multiple options.  So:

return ['-fstack-protector-strong', '-D_FORTIFY_SOURCE=2']
Attachment #8864320 - Flags: review?(nfroyd) → review+
Comment on attachment 8864320 [details] [diff] [review]
Add FORTIFY_SOURCE security flag

># HG changeset patch
># User Nomis101 <Nomis101@web.de>
># Date 1493850400 -7200
>#      Thu May 04 00:26:40 2017 +0200
># Node ID 62f613eb634feef3ae7e3de24aed2f4fb760883d
># Parent  b25ad0674afd563e888dc07981baa626e8d794db
>Bug 1359908 - Add FORTIFY_SOURCE security flag
>
>diff --git a/build/moz.configure/toolchain.configure b/build/moz.configure/toolchain.configure
>--- a/build/moz.configure/toolchain.configure
>+++ b/build/moz.configure/toolchain.configure
>@@ -1000,12 +1000,12 @@ include('windows.configure', when=is_win
> # ==============================================================
> 
> option('--enable-hardening', env='MOZ_SECURITY_HARDENING',
>        help='Enables security hardening compiler options')
> 
> @depends('--enable-hardening', c_compiler)
> def security_hardening_cflags(value, c_compiler):
>     if value and c_compiler.type in ['gcc', 'clang']:
>-        return '-fstack-protector-strong'
>+        return ['-fstack-protector-strong', '-D_FORTIFY_SOURCE=2']
> 
> add_old_configure_assignment('HARDENING_CFLAGS', security_hardening_cflags)
> imply_option('--enable-pie', depends_if('--enable-hardening')(lambda v: v))
OK, seems I need to upload a new patch. I was hoping I just can edit it.
Attached patch Patch with review comments (obsolete) — Splinter Review
OK, I've applied the review comments. Could somebody put this on try run to sort out any performance issues, please? I don't have access to the try server. But I checked local that it builds (on macOS with Apple Clang).
Priority: -- → P2
Tom: is comment 7 directed at you, or could you redirect it?
Severity: normal → enhancement
Flags: needinfo?(tom)
Whiteboard: [sg:want]
(In reply to Tom Ritter [:tjr] from comment #9)
> Try run with full unit tests to see if this breaks anything:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f688cf575f02f3d543f03c3ba060ec148ac85716
> 
> Try run for talos:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=33d3a4b99d1713461795808c819293693f751091
> Talos:
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=33d3a4b99d1713461795808c819293693f7
> 51091

Thank you so much! Looks good, does it?
Thanks for clearing up the warnings. Now that Bug 1406687 has checked in, is it OK if I set the checkin-needed flag?
(In reply to Nomis101 from comment #13)
> Thanks for clearing up the warnings. Now that Bug 1406687 has checked in, is
> it OK if I set the checkin-needed flag?

I'd like to try and get someone from the performance team to look at the Talos results. If we can land this by default (and not hidden behind a flag) that would be better than putting it behind a flag we don't use on shipped releases.
adding myself/:igoldan here as there could be perf changes when this lands :)
OSX is still coming in, hopefully will be in when you check tomorrow, but the new linux results look... like there's barely any change. :)

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16
Flags: needinfo?(jmaher)
overall this looks to have no significant or measurable changes.

while many of the data points show changes, they are low confidence, or some are medium confidence- this means they have outliers or a higher stddev and overlap in large portions with the data points from mozilla-central.

A few looked sort of interesting:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16&framework=1&filter=responsiveness%20linux%20pgo&selectedTimeRange=172800
* as those are pgo specific, there isn't much to do other than change the pgo profiling script or remove certain blocks of code from the profiling script

tp6 youtube on osx:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16&framework=1&filter=tp6%20youtube%20osx&selectedTimeRange=172800
* this might be a real regression

tp6 amazon opt: linux e10s, osx stylo-disabled e10s:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16&framework=1&filter=tp6%20amazon%20opt&selectedTimeRange=172800
* these have a higher chance of being a regression.


to be honest nothing is jumping out as a must look at given the noise levels.
Flags: needinfo?(jmaher)
I realized I had omitted it from js/

I have a run here I was hoping you could look at also:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=482d2433fe40ca1a618270a19a876ce1a762c590&selectedJob=138110086
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=482d2433fe40ca1a618270a19a876ce1a762c590&framework=1&showOnlyImportant=1&selectedTimeRange=172800

I will prepare a patch that turns this on by default for everything except js/ (and maybe for js/ too depending on what you think) and flag you for review.
Flags: needinfo?(jmaher)
the green results (improvements) are all looking valid, I am more concerned with a11y and speedometer as those look to be real regressions.  Speedometer is one of the Firefox 57 key benchmarks we tracked, so there might be more concerns with regressing that so much- but look at the subtests there- if you can figure out why the deletingitems/async subtests are causing regressions- there are a lot of other improvements.

I do like a lot of the improvements we see overall, maybe a little bit of work for big gains.
Flags: needinfo?(jmaher)
wait, I probably spoke too soon, speedometer recently (2 days ago) improved for linux64-qr and the base of the try push is 3 days old.  So speedometer is posting at parity with the previous data.
Attachment #8920018 - Flags: review?(jmaher)
Attachment #8920018 - Flags: review?(core-build-config-reviews)
Comment on attachment 8920018 [details]
Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default

I am not really qualified to review this- I am not familiar with those files which are changed- I suspect the build team will be better qualified- but the talos numbers seem to be stable or improved :)
Attachment #8920018 - Flags: review?(jmaher)
Comment on attachment 8920018 [details]
Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default

https://reviewboard.mozilla.org/r/190996/#review197294

Assuming this is talos-neutral, and maybe even if it's not, this is fine.
Attachment #8920018 - Flags: review+
Comment on attachment 8920018 [details]
Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default

Sigh, mozreview.
Attachment #8920018 - Flags: review?(core-build-config-reviews) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/057166cb35fe
Add FORTIFY_SOURCE to gcc and clang builds by default r=froydnj
Keywords: checkin-needed
Thanks Nomis101 for your work on this!
https://hg.mozilla.org/mozilla-central/rev/057166cb35fe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Tom Ritter [:tjr] from comment #28)
> Thanks Nomis101 for your work on this!

You are welcome. Thanks for moving it forward.
The following line gives an error in Windows/mozilla-build environments.
https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/old-configure.in#503

> INFO -  z:/build/build/src/old-configure: line 5045: test: too many arguments

The configure continues after this and build succeeds, but we should probably fix this.

I believe the change needed is: s/-o test/-o/
Flags: needinfo?(tom)
Depends on: 1414367
No longer depends on: 1414367
Depends on: 1417452
Whiteboard: [adv-main58-]
You need to log in before you can comment on or make changes to this bug.