1503589 - Use the --enable-hardening configure flag for Linux builds

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Gian-Carlo Pascutto [:gcp]]

This bug is to cover to turning on the --enable-hardening flag on Linux builds. 

Turning this on is going to include the changes to set the flag and also performance testing to determine performance impacts.

Our understanding is that Ubuntu is shipping this configuration for their builds already.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Reference:
https://hg.mozilla.org/try/rev/cb0e6ff86d984a19bf88c630436c4b69cde50b47
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb0e6ff86d984a19bf88c630436c4b69cde50b47

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=cb0e6ff86d984a19bf88c630436c4b69cde50b47

Comment 2

[Gian-Carlo Pascutto [:gcp]]

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

Comment 3

[Gian-Carlo Pascutto [:gcp]]

--enable-hardening build:
https://hg.mozilla.org/try/rev/d507ede4ceaf2c8338589a16eb8a6229e810da5b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d507ede4ceaf2c8338589a16eb8a6229e810da5b

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=d507ede4ceaf2c8338589a16eb8a6229e810da5b

Comment 4

[Gian-Carlo Pascutto [:gcp]]

https://phabricator.services.mozilla.com/D10381

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Exception: Unknown platform(s) [linux64-st-an] specified for try

Looks like a trychooser bug then?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

2nd attempt:

Reference:
https://hg.mozilla.org/try/rev/057673fdf6638fc0ef376dd231b8f15df22e1436
https://treeherder.mozilla.org/#/jobs?repo=try&revision=057673fdf6638fc0ef376dd231b8f15df22e1436
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=057673fdf6638fc0ef376dd231b8f15df22e1436

hardened:
https://hg.mozilla.org/try/rev/daf6304c25fe237c3992e2e5834a43151caac5cc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf6304c25fe237c3992e2e5834a43151caac5cc
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=daf6304c25fe237c3992e2e5834a43151caac5cc

Comment 7

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=057673fdf6638fc0ef376dd231b8f15df22e1436&newProject=try&newRevision=daf6304c25fe237c3992e2e5834a43151caac5cc&framework=1&showOnlyConfident=1

Pref regression is typically <1%, except for

perf_reftest_singletons pgo e10s stylo   2.6%
a11yr opt e10s stylo                     4.5%

We said before we're willing to take a 2-3% performance hit for this, so this looks good. Digging into the a11yr result might be useful as it's possible some snipped of code suffers from the stack checks for some reason.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 1503589 - Use the --enable-hardening configure flag for Linux builds.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Context for reviewers: many of the blockers for enabling hardening (and things like PIE) have been eliminated in the last few years. There was a request to not enable hardening (and any perf hit) before the first Quantum release. This has passed now, so we did a new Talos run. The regressions there are very small, and probably do not outweigh the security benefit.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Much of the original discussion is in: https://bugzilla.mozilla.org/show_bug.cgi?id=620058

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 1503589 - Enable strong stack protector by default. r?glandium

Comment 12

[Gian-Carlo Pascutto [:gcp]]

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

Comment 13

[Gian-Carlo Pascutto [:gcp]]

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

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40d197664391
Enable strong stack protector by default. r=glandium

Comment 15

[Cosmin Sabou [:CosminS]]

Backed out for causing windows MinGW build bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=busted&fromchange=40d19766439121cd6d231dbee225243d14939db0&tochange=235c2ef758a1e21f0520c5fed9cc3d4564f56c57&selectedJob=211925195

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211925195&repo=autoland&lineNumber=4055

Backout link: https://hg.mozilla.org/integration/autoland/rev/235c2ef758a1e21f0520c5fed9cc3d4564f56c57

Comment 16

[Gian-Carlo Pascutto [:gcp]]

The build that failed doesn't even define -fstack-protector-strong. What's going on?

[task 2018-11-15T10:25:57.194Z] 10:25:57     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/x86_64-w64-mingw32-clang++ -mwindows -o StoreBuffer.o -c  -DDEBUG=1 -DENABLE_BINARYDATA -DENABLE_WASM_BULKMEM_OPS -DENABLE_WASM_GC -DENABLE_WASM_GENERALIZED_TABLES -DWASM_PRIVATE_REFTYPES -DWASM_HUGE_MEMORY -DJS_CACHEIR_SPEW -DENABLE_SHARED_ARRAY_BUFFER -DENABLE_WASM_THREAD_OPS -DJS_HAS_CTYPES -DFFI_BUILDING -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/builds/worker/workspace/build/src/js/src/gc -I/builds/worker/workspace/build/src/obj-firefox/js/src/gc -I/builds/worker/workspace/build/src/obj-firefox/js/src -I/builds/worker/workspace/build/src/js/src -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/js/src/js-confdefs.h -Qunused-arguments -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-unknown-pragmas -Wno-unused-function -Wno-conversion-null -Wno-switch -Wno-enum-compare -Wno-gnu-zero-variadic-macro-arguments -Wno-noexcept-type -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fms-extensions -mms-bitfields -Wno-incompatible-ms-struct -fno-rtti -ffunction-sections -fdata-sections -Wa,-mbig-obj -fno-exceptions -fno-math-errno -pipe -g -gcodeview -fno-omit-frame-pointer -funwind-tables -fno-strict-aliasing -Werror=format -Wno-shadow  -MD -MP -MF .deps/StoreBuffer.o.pp   /builds/worker/workspace/build/src/js/src/gc/StoreBuffer.cpp

Comment 17

[Gian-Carlo Pascutto [:gcp]]

tjr pointed out that some configure checks, including the one for _thread, give a different result. I noticed it even changes whether truncate64 support is detected, which is just very odd.

Comment 18

[Tom Ritter [:tjr]]

The problem seems to be that in our toolchain, we have lib/libssp.a and lib/libssp_nonshared.a but we look for libraries in x86_64-w64-mingw32/lib/

Symlinking these two libraries into x86_64-w64-mingw32/lib/ makes the configure tests pass, which should resolve the build error.  I just want to double check that this makes sense and is safe.  These libraries are also in lib32/ and symlinking those versions _also_ works. Which version should be use for x64? The same files are present in the same place for x86; which version should we use there?

Comment 19

[Jacek Caban]

Probably we shouldn't use any of them. Those are host libraries, we'd need something for cross target. Here is how llvm-mingw toolchain builds it:
https://github.com/mstorsjo/llvm-mingw/blob/master/build-libssp.sh
We could do the same in our toolchain build script (we don't need .dll version, just static library, so it's a bit simpler than tat) or even use llvm-mingw script for that.


Another way to look at it is that this bug is about Linux build. mingw builds are made on Linux, but those are Windows builds. Thus it's not obvious to me that enabling it on mingw clang is intentional as far as this bug is considered.

Comment 20

[Tom Ritter [:tjr]]

gcp: I'm not going to be able to dig into that in the short term; we should disable stack protector for mingw-clang, and file a followup bug blocking 1330608.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8d320ccbfbc
Enable strong stack protector by default. r=glandium

Comment 23

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b8d320ccbfbc

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Relnote (together with bug 1492917):

"Firefox on Linux and macOS now enables stronger stack smashing protection by default."

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Amended release note:

"Firefox on Linux, Android and macOS now enables stronger stack smashing protection by default."