Opened 22 months ago

Last modified 19 months ago

#23024 needs_revision defect

Flags to increase hardening on Windows

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201711
Cc: boklm Actual Points:
Parent ID: #21448 Points:
Reviewer: Sponsor:

Description

We can add -fstack-protector-all and -D_FORTIFY_SOURCE=2 to our Windows build for some extra protection.

Child Tickets

Change History (10)

comment:1 Changed 22 months ago by arthuredelstein

Here's my patch for review. I would suggest we adding it after we have transitioned to rbm builds:
https://github.com/arthuredelstein/tor-browser-build/commit/23024

I tried building TBB with -fstack-protector-strong on mingw-w64 but so far ran into errors. However, a Windows Tor Browser built with this patch (using -fstack-protector-all) doesn't seem subjectively slower to me, so I would suggest trying this on the alpha, at least until we have a solution for -fstack-protector-strong on mingw-w64.

Last edited 22 months ago by arthuredelstein (previous) (diff)

comment:2 Changed 22 months ago by arthuredelstein

Keywords: TorBrowserTeam20170724R added
Status: newneeds_review

comment:3 Changed 22 months ago by arthuredelstein

Keywords: TorBrowserTeam201707R added; TorBrowserTeam20170724R removed

comment:4 Changed 22 months ago by gk

Cc: boklm added
Keywords: TorBrowserTeam201707 added; TorBrowserTeam201707R removed
Status: needs_reviewneeds_revision

I tested -fstack-protector-strong on top of the latest tor-browser-bundle commit. And the compilation worked as expected. Is that a tor-browser-build issue? Or maybe the GCC version bump (tor 5.4.0) resolved this problem?

Regarding fortify source: Have you checked whether the _chk part is actually there after compiling with -D_FORTIFY_SOURCE=2? Because it does not seem to be the case. Doing a

i686-w64-mingw32-nm -C firefox.exe | grep strcpy

after compiling with the flags in your patch does only give ma a

0041b3f4 I _imp__strcpy
00413320 T strcpy

(Note: In order to check it the way I did you need to compile the browser part with --disable-strip and --disable-install-strip)

Assuming I am not mistaken then the likely root cause of this problem is a GCC bug which the RedHat people are tracking in https://bugzilla.redhat.com/show_bug.cgi?id=1324759.

comment:5 in reply to:  4 Changed 22 months ago by cypherpunks

Replying to arthuredelstein:

However, a Windows Tor Browser built with this patch (using -fstack-protector-all) doesn't seem subjectively slower to me, so I would suggest trying this on the alpha, at least until we have a solution for -fstack-protector-strong on mingw-w64.

Also you can copy https://dxr.mozilla.org/mozilla-esr52/source/old-configure.in#957 to *-mingw*) section to gain parity with Linux.

Replying to gk:

I tested -fstack-protector-strong on top of the latest tor-browser-bundle commit. And the compilation worked as expected. Is that a tor-browser-build issue? Or maybe the GCC version bump (tor 5.4.0) resolved this problem?

tor 5.4.0 from 2540 :) Try with --disable-auto-import for fun :)

Regarding fortify source: Have you checked whether the _chk part is actually there after compiling with -D_FORTIFY_SOURCE=2? Because it does not seem to be the case. Doing a

i686-w64-mingw32-nm -C firefox.exe | grep strcpy

after compiling with the flags in your patch does only give ma a

0041b3f4 I _imp__strcpy
00413320 T strcpy

(Note: In order to check it the way I did you need to compile the browser part with --disable-strip and --disable-install-strip)

Assuming I am not mistaken then the likely root cause of this problem is a GCC bug which the RedHat people are tracking in https://bugzilla.redhat.com/show_bug.cgi?id=1324759.

This is https://bugzilla.mozilla.org/show_bug.cgi?id=1359908

You also need something to:

  1. check your flags passed and applied properly
  2. check features compiled properly
  3. check features works properly

comment:6 in reply to:  1 Changed 22 months ago by cypherpunks

Replying to arthuredelstein:

Here's my patch for review. I would suggest we adding it after we have transitioned to rbm builds:
https://github.com/arthuredelstein/tor-browser-build/commit/23024

Some kind of wrappers... whether they applied to all TBB parts?
What to include (by order):
$COMPILER_WARNINGS (like -Werror=format-security)
$COMPILER_OPTIONS (like -D_FORTIFY_SOURCE=2)
$COMPILER_OPTIMIZATIONS (like -fno-delete-null-pointer-checks)
$LINKER_FLAGS
-Wl,--enable-reloc-section - its name is awful (as awful as that this bug is still present). There should be no such flag as this is a part of -Wl,--dynamicbase by meaning. Firefox has export table, but TBB hasn't. There should be a better way to make the toolchain work properly, than a specific hack which can't get upstreamed.

Other flags:
-Wl,--image-base,0x10000000 to force relocations.
-Wl,--large-address-aware is always set by the compiler driver (e.g. Cygwin gcc). MinGW too (usually yes)? If so, no need for #22477.
-Wl,--forceinteg - Better than nothing. Code integrity checking, while no signatures.
-Wl,--no-seh - that's the only part of SafeSEH GCC supports, see ticket:20322#comment:3. Upstream to https://dxr.mozilla.org/mozilla-esr52/source/old-configure.in#1218
(-Wl,--tsaware - Terminal Server aware - is for upstream only.)

Last edited 21 months ago by cypherpunks (previous) (diff)

comment:7 Changed 22 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:8 Changed 21 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:9 Changed 20 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:10 Changed 19 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

Note: See TracTickets for help on using tickets.