1359908 - Add FORTIFY_SOURCE security flag

Status: RESOLVED FIXED

(Core :: Security, enhancement, P2)

Description

[Tom Ritter [:tjr]]

This bug was created as a clone of Bug 620058 which has more context.

Comment 1

[Nomis101]

Attachment: Add FORTIFY_SOURCE security flag

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

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 3

[Nomis101]

OK. I do not have access to the try server. I've just checked locally that it builds properly.

Comment 4

[Nathan Froyd [:froydnj]]

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']

Comment 5

[Nomis101]

Comment on attachment 8864320 [details] [diff] [review]
Add FORTIFY_SOURCE security flag

># HG changeset patch
># User Nomis101 <[e-mail]>
># 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))

Comment 6

[Nomis101]

OK, seems I need to upload a new patch. I was hoping I just can edit it.

Comment 7

[Nomis101]

Attachment: Patch with review comments

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

Comment 8

[Daniel Veditz [:dveditz]]

Tom: is comment 7 directed at you, or could you redirect it?

Comment 9

[Tom Ritter [:tjr]]

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=33d3a4b99d1713461795808c819293693f751091

Comment 10

[Nomis101]

(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 11

[Tom Ritter [:tjr]]

(In reply to Nomis101 from comment #10)
> Thank you so much! Looks good, does it?

I uh, screwed up the patch. (Well the patch was fine, but I didn't turn on --enable-hardening, so we weren't testing anything.)

I hardcoded it:

Unit Test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86162255645ff9243fe871ae9ff674e8c339a6bb

Talos:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dca8e59ba7b5734791e89cce6d45119ce5c509d3
 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=dca8e59ba7b5734791e89cce6d45119ce5c509d3

Comment 12

[Tom Ritter [:tjr]]

Okay, I think I cleared up the warnings-as-errors, third time's a charm?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=735960651681145410715aaa2b9baa3f0a99f370
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b6e61e33c57c84bb6d3a1ccb3d2f0455742f896
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=3b6e61e33c57c84bb6d3a1ccb3d2f0455742f896

Comment 13

[Nomis101]

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

Comment 14

[Tom Ritter [:tjr]]

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

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

adding myself/:igoldan here as there could be perf changes when this lands :)

Comment 16

[Tom Ritter [:tjr]]

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

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 18

[Tom Ritter [:tjr]]

Attachment: Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default

Review commit: https://reviewboard.mozilla.org/r/190996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190996/

Comment 19

[Tom Ritter [:tjr]]

Attachment: Bug 1359908 Add FORTIFY_SOURCE=2 to js/ also

Review commit: https://reviewboard.mozilla.org/r/190998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190998/

Comment 20

[Tom Ritter [:tjr]]

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.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 8920018 [details]
Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190996/diff/1-2/

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 25

[Nathan Froyd [:froydnj]]

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.

Comment 26

[Nathan Froyd [:froydnj]]

Comment on attachment 8920018 [details]
Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default

Sigh, mozreview.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/057166cb35fe
Add FORTIFY_SOURCE to gcc and clang builds by default r=froydnj

Comment 28

[Tom Ritter [:tjr]]

Thanks Nomis101 for your work on this!

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/057166cb35fe

Comment 30

[Nomis101]

(In reply to Tom Ritter [:tjr] from comment #28)
> Thanks Nomis101 for your work on this!

You are welcome. Thanks for moving it forward.

Comment 31

[Ted Campbell [:tcampbell]]

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/

Comment 32

[Tom Ritter [:tjr]]

Filed as Bug 1414067