Opened 10 months ago

Closed 5 weeks ago

#29013 closed enhancement (fixed)

Provide stack smashing protection for mingw-clang builds

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, GeorgKoppen201910, tbb-9.0, TorBrowserTeam201910R
Cc: tom, boklm Actual Points: 1.5
Parent ID: Points: 1
Reviewer: Sponsor:

Description

Currently with our mingw-w64/gcc builds we provide among other things stack smashing protection (SSP). We want to do the same for mingw-w64/clang builds.

https://bugzilla.mozilla.org/show_bug.cgi?id=1511073 is the upstream bug. https://bugzilla.mozilla.org/show_bug.cgi?id=1503589#c19 has a good summary to get us started (pointing e.g. to https://github.com/mstorsjo/llvm-mingw/blob/master/build-libssp.sh)

Child Tickets

Change History (36)

comment:1 Changed 9 months ago by gk

Keywords: GeorgKoppen201902 added; GeorgKoppen201901 removed

Moving my tickets to Feb

comment:2 Changed 9 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:3 Changed 9 months ago by gk

Priority: MediumHigh

comment:4 Changed 8 months ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving my tickets to March.

comment:5 Changed 8 months ago by gk

Keywords: GeorgKoppen201903 added; GeorgKoppen201902 removed

Now for my keyword.

comment:6 Changed 7 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

comment:7 Changed 7 months ago by gk

Keywords: GeorgKoppen201904 added; GeorgKoppen201903 removed

Moving my tickets for April

comment:8 Changed 6 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:9 Changed 6 months ago by gk

Keywords: GeorgKoppen201905 added; GeorgKoppen201904 removed

Move my tickets.

comment:10 Changed 5 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:11 Changed 5 months ago by gk

Keywords: GeorgKoppen201906 added; GeorgKoppen201905 removed

Moving my tickets to June

comment:12 Changed 5 months ago by gk

Keywords: GeorgKoppen201907 added; GeorgKoppen201906 removed

Moving my tickets to July.

comment:13 Changed 4 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:14 Changed 3 months ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201907 removed

Moving more tickets to August

comment:15 Changed 3 months ago by gk

Keywords: GeorgKoppen201908 added; GeorgKoppen201907 removed

Move my tickets.

comment:16 Changed 3 months ago by gk

Parent ID: #28238#30322

comment:17 Changed 2 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201908 removed

Moving tickets to September

comment:18 Changed 2 months ago by gk

Keywords: tbb-9.0-must-alpha added

comment:19 Changed 2 months ago by pili

Points: 1

comment:21 Changed 7 weeks ago by gk

I guess what we can do, instead of building libssp with build-libssp.sh in a separate step, is to just use the static libraries we build for mingw-w64 anyway.

comment:22 Changed 7 weeks ago by cypherpunks

FWIW, starting from GCC 8, there is --disable-libssp configure time option, because "On many targets library support is provided by the C library instead."

comment:23 in reply to:  21 ; Changed 7 weeks ago by gk

Replying to gk:

I guess what we can do, instead of building libssp with build-libssp.sh in a separate step, is to just use the static libraries we build for mingw-w64 anyway.

That approach is not working very well it seems. I used my bug_29013 branch for tor-browser and bug_29013_v2 for tor-browser-build.

First of all for some reason we need the *.dll as well in the Browser dir (I thought ssp got statically linked in) but copying it over results in crashes both on 32bit and 64bit versions during start-up.

More digging is needed here.

comment:24 Changed 7 weeks ago by cypherpunks

o_0 and libcxx?

comment:25 in reply to:  23 Changed 6 weeks ago by gk

Replying to gk:

Replying to gk:

I guess what we can do, instead of building libssp with build-libssp.sh in a separate step, is to just use the static libraries we build for mingw-w64 anyway.

That approach is not working very well it seems. I used my bug_29013 branch for tor-browser and bug_29013_v2 for tor-browser-build.

First of all for some reason we need the *.dll as well in the Browser dir (I thought ssp got statically linked in) but copying it over results in crashes both on 32bit and 64bit versions during start-up.

More digging is needed here.

Using the llvm-mingw approach results in the same problems. I guess I need to look closer in a debugger what's crashing here. Might help to solve #31546 first, though.

comment:26 Changed 6 weeks ago by gk

Keywords: tbb-9.0 added; tbb-9.0-must-alpha removed
Parent ID: #30322

I might not get to that in time...

comment:27 Changed 6 weeks ago by gk

So, not adding libssp.dll.a solves actually both issues I had (the dynamically linking and the crashes). I might be tempted to pick this up to get it still into 9.0 or 9.5a1 if the former is too risky. :) (thanks again, Martin)

Last edited 6 weeks ago by gk (previous) (diff)

comment:28 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201910 added

comment:29 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201909 removed

comment:30 Changed 6 weeks ago by gk

Keywords: GeorgKoppen201910 added; GeorgKoppen201908 removed

comment:31 in reply to:  27 Changed 6 weeks ago by gk

Actual Points: 1.5
Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

Replying to gk:

So, not adding libssp.dll.a solves actually both issues I had (the dynamically linking and the crashes). I might be tempted to pick this up to get it still into 9.0 or 9.5a1 if the former is too risky. :) (thanks again, Martin)

Here we are. bug_29013_v5 (https://gitweb.torproject.org/user/gk/tor-browser-build.git/commit/?h=bug_29013_v5&id=562fe8f1df0de8912c9eaf7ff8c3b4d989d4aa01) in my tor-browser-build repo and bug_29013_v2 (https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_29013_v2&id=cb5ccc9e3b50e9d37a4c7a34a0c81418df38adfe) in my tor-browser one.

The first patch is using our own mingw-w64 which we build anyway and is just copying the .a libs over. The tor-browser patch essentially backs out the special treatment of mingw-w64-clang.

comment:32 Changed 6 weeks ago by gk

FWIW: I am not really sure how to test that for Windows stack canaries are embedded in the binaries but I can see that the linker is at least loading libssp.a for ___stack_chk_fail during linking.

comment:33 Changed 6 weeks ago by cypherpunks

Is there any advantage of this approach vs. SSPStrong (via UCRT?) enabled by default in clang-cl?

comment:34 in reply to:  33 Changed 6 weeks ago by gk

Replying to cypherpunks:

Is there any advantage of this approach vs. SSPStrong (via UCRT?) enabled by default in clang-cl?

I don't know to be honest. I feel we are on the safer side following llvm-mingw here as that approach seems to be working. I'd be happy looking over a patch for your approach, though.

comment:35 Changed 6 weeks ago by cypherpunks

How does it work for e.g. libcxx if it is not enabled globally?
llvm-mingw seems to be made for UCRT explicitly, so it can follow clang-cl's defaults and avoid dependency on GCC. Talk to Martin about it.

comment:36 Changed 5 weeks ago by boklm

Resolution: fixed
Status: needs_reviewclosed

The two patches look good to me. I also checked that a 32bit and 64bit build with those two patches are still working for me on a Windows 7 VM.

I cherry-picked the tor-browser commit as 4b8a33af9610111d87dc5a901d06bcc20f1cc7b0, and merged the tor-browser-build commit with effde40cc5643080d45670d889a2fd81a6d39c67.

Note: See TracTickets for help on using tickets.