Add FORTIFY_SOURCE security flag

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tjr, Assigned: Nomis101)

Tracking

(Blocks 2 bugs, {sec-want})

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [adv-main58-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
This bug was created as a clone of Bug 620058 which has more context.
(Reporter)

Updated

2 years ago
Depends on: 1359918
(Reporter)

Updated

2 years ago
Depends on: 1359920
(Reporter)

Updated

2 years ago
Depends on: 1359926
(Reporter)

Updated

2 years ago
Depends on: 1359928
(Reporter)

Updated

2 years ago
Depends on: 1360299
(Reporter)

Updated

2 years ago
Depends on: 1360300
(Reporter)

Updated

2 years ago
Depends on: 1360301
(Reporter)

Updated

2 years ago
No longer depends on: 1359918
(Reporter)

Updated

2 years ago
No longer depends on: 1360301
(Reporter)

Updated

2 years ago
No longer depends on: 1360300
(Reporter)

Updated

2 years ago
No longer depends on: 1360299
(Reporter)

Updated

2 years ago
No longer blocks: 1359905
(Reporter)

Updated

2 years ago
No longer depends on: 1359928
(Reporter)

Updated

2 years ago
No longer depends on: 1359920
(Reporter)

Updated

2 years ago
No longer depends on: 1359926
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Updated

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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+
(Assignee)

Comment 5

2 years ago
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))
(Assignee)

Comment 6

2 years ago
OK, seems I need to upload a new patch. I was hoping I just can edit it.
(Assignee)

Comment 7

2 years ago
Posted 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]
(Assignee)

Comment 10

2 years ago
(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?
Comment hidden (obsolete)
(Reporter)

Updated

2 years ago
Depends on: 1406687
(Assignee)

Comment 13

2 years ago
Thanks for clearing up the warnings. Now that Bug 1406687 has checked in, is it OK if I set the checkin-needed flag?
(Reporter)

Comment 14

2 years ago
(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 :)
(Reporter)

Comment 16

2 years ago
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)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

2 years ago
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.
(Reporter)

Updated

2 years ago
Depends on: 1410438
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Attachment #8920019 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8920018 - Flags: review?(jmaher)
Attachment #8920018 - Flags: review?(core-build-config-reviews)
(Reporter)

Updated

2 years ago
Attachment #8864320 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8864947 - Attachment is obsolete: true
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 25

2 years ago
mozreview-review
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+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 27

2 years ago
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
(Reporter)

Comment 28

2 years ago
Thanks Nomis101 for your work on this!
https://hg.mozilla.org/mozilla-central/rev/057166cb35fe
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 30

2 years ago
(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)
(Reporter)

Updated

2 years ago
Blocks: 1414067
(Reporter)

Comment 32

2 years ago
Filed as Bug 1414067
(Reporter)

Updated

2 years ago
Flags: needinfo?(tom)
Depends on: 1414367
No longer depends on: 1414367
(Reporter)

Updated

2 years ago
Blocks: 1415595
Depends on: 1417452
Whiteboard: [adv-main58-]
You need to log in before you can comment on or make changes to this bug.