Opened 2 years ago

Last modified 4 weeks 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, tbb-rbm
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 (14)

comment:1 Changed 2 years 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/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.

Version 0, edited 2 years ago by arthuredelstein (next)

comment:2 Changed 2 years ago by arthuredelstein

Keywords: TorBrowserTeam20170724R added
Status: newneeds_review

comment:3 Changed 2 years ago by arthuredelstein

Keywords: TorBrowserTeam201707R added; TorBrowserTeam20170724R removed

comment:4 Changed 2 years 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 2 years 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 2 years 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 2 years ago by cypherpunks (previous) (diff)

comment:7 Changed 2 years ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:8 Changed 2 years ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:9 Changed 2 years ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:10 Changed 2 years ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:11 Changed 6 weeks ago by gk

Keywords: tbb-rbm added

comment:12 Changed 6 weeks ago by tom

Werror=format-security - enabled on non-Windows, https://bugzilla.mozilla.org/show_bug.cgi?id=1578619

_FORTIFY_SOURCE - enabled but mingw-clang doesn't support it (yet. it's a WIP)

-fno-delete-null-pointer-checks https://bugzilla.mozilla.org/show_bug.cgi?id=1578617

-Wl,--enable-reloc-section - AFAICT unneeded for mingw-clang, since ASLR is working

--image-base - AFAICT not needed on modern systems

large-address-aware - enabled for mingw-clang

--forceinteg - not applicablt to clang/lld

--no-seh - set by lld automatically https://reviews.llvm.org/D41252 (but this would be good to confirm manually

--tsaware - I'm not sure but I really hope that this is completely unneeded by now.

comment:13 Changed 4 weeks ago by cypherpunks

What about --icf=all automatically? https://github.com/llvm/llvm-project/blob/d0f63f83e7c5c6fc11e964f848d1496234695182/lld/MinGW/Driver.cpp#L265

--forceinteg - not applicablt to clang/lld

What do you mean? Just disabled by default: https://github.com/llvm/llvm-project/blob/ee6fbebbaff5af0a0fbe58a0e33ef191340223ea/lld/COFF/Driver.cpp#L1507

--no-seh - set by lld automatically ​https://reviews.llvm.org/D41252 (but this would be good to confirm manually

What about --safeseh automatically? https://github.com/llvm/llvm-project/blob/ee6fbebbaff5af0a0fbe58a0e33ef191340223ea/lld/COFF/Driver.cpp#L1617

--tsaware - I'm not sure but I really hope that this is completely unneeded by now.

Because it is enabled and should be enabled by default, you mean? https://github.com/llvm/llvm-project/blob/ee6fbebbaff5af0a0fbe58a0e33ef191340223ea/lld/COFF/Driver.cpp#L1513

comment:14 in reply to:  13 Changed 4 weeks ago by tom

Replying to cypherpunks:

What about --icf=all automatically? https://github.com/llvm/llvm-project/blob/d0f63f83e7c5c6fc11e964f848d1496234695182/lld/MinGW/Driver.cpp#L265

Haven't heard of it; but https://clang.llvm.org/docs/UsersManual.html says that the arguements needed for ICF to work (-faddrsig) are ELF only...

--forceinteg - not applicablt to clang/lld

What do you mean? Just disabled by default: https://github.com/llvm/llvm-project/blob/ee6fbebbaff5af0a0fbe58a0e33ef191340223ea/lld/COFF/Driver.cpp#L1507

Ahhah; I was wrong. So it looks like this sets IMAGE_DLLCHARACTERISTICS_FORCE_INTEGRITY which requires a file be signed before it's loaded.

Frankly it seems kind of useless to me, an attacker who can modify the dll would invalidate the signature; but they could just strip the signature and the unset the flag. But if it cost nothing, I'd say sure, flip it: but I'm not sure which Tor Browser releases we Authenticode sign; which this would require.

--no-seh - set by lld automatically ​https://reviews.llvm.org/D41252 (but this would be good to confirm manually

What about --safeseh automatically? https://github.com/llvm/llvm-project/blob/ee6fbebbaff5af0a0fbe58a0e33ef191340223ea/lld/COFF/Driver.cpp#L1617

Oh good catch: on by default except for MinGW. We should investigate why that is and if we can enable it.

--tsaware - I'm not sure but I really hope that this is completely unneeded by now.

Because it is enabled and should be enabled by default, you mean? https://github.com/llvm/llvm-project/blob/ee6fbebbaff5af0a0fbe58a0e33ef191340223ea/lld/COFF/Driver.cpp#L1513

https://docs.microsoft.com/en-us/cpp/build/reference/tsaware-create-terminal-server-aware-application?view=vs-2019 "When an application is not Terminal Server aware (also known as a legacy application), Terminal Server makes certain modifications to the legacy application to make it work properly in a multiuser environment. For example, Terminal Server will create a virtual Windows folder, such that each user gets a Windows folder instead of getting the system's Windows directory. This gives users access to their own INI files. In addition, Terminal Server makes some adjustments to the registry for a legacy application. These modifications slow the loading of the legacy application on Terminal Server."

I had hoped that all this nonsense was not needed/performed in Windows 10 or at least the compiler set the flag automatically. The code makes it seem like it does not; but I can't find the flag in Firefox's code, which implies that it would not be setting it either...

More investigation needed, specifically what Firefox sets and if this has any effect on Windows 7+

Note: See TracTickets for help on using tickets.