Many memory pages in tor.exe for Windows violate W^X

A cypherpunk (ticket:21617#comment:5) has reported that the tor.exe process in the Tor Expert Bundle on Windows has many Execute/Read/Write memory pages. I also observe the same thing for Tor Browser's tor.exe process. Also, there are many Execute/Copy on Write pages, which I suspect, after reading Microsoft documentation, are also effectively W^X violations.

To reproduce on Windows:

1. Download VMMap: [https://technet.microsoft.com/en-us/sysinternals/vmmap.aspx]
2. Run Tor Browser
3. Run VMMap and select the tor.exe process
4. Select View > Expand All
5. In the bottom table of the VMMap window, examine the Protection column. Note many Execute/Read/Write and Execute/Copy on Write pages, all belonging to either tor.exe or DLLs bundled with tor.exe.

added 031-backport TorBrowserTeam201707R component::applications/tor browser hardening owner::arthuredelstein priority::medium resolution::fixed security severity::normal status::closed tbb-not-backported tor-client tor-relay type::defect win32 windows labels

So are we linking wrong? Compiling wrong? Using bogus APIs?

Adding tjr to cc

Trac:
Cc: N/A to tjr

Trac:
Keywords: N/A deleted, windows tor-client win32 tor-relay security hardening 031-backport added
Milestone: N/A to Tor: 0.3.2.x-final

See also https://trac.torproject.org/projects/tor/ticket/22584

Trac:
Cc: tjr to tom

I have tracked down the problem and posted a patch for mingw-w64.

https://sourceforge.net/p/mingw-w64/mailman/message/35935261/

If they accept the patch, we can add it to our build as well. I have confirmed with this patch that no RWX or X/COW pages remain.

This patch solves at least some of the RWX pages reported in Tor Browser for Windows (#22584 (moved)) as well.

Trac:
Owner: N/A to arthuredelstein
Status: new to accepted

Not sure how long this takes but we might want to try it in the next alpha anyway. Thus, putting it on our radar.

Trac:
Keywords: N/A deleted, TorBrowserTeam201707 added

The patch is now in mingw-w64 master. So here's a branch that pulls that in:

https://github.com/arthuredelstein/tor-browser-build/commit/22563

(If we decide we want to apply it to alpha only, I think maybe we can use rbm vars to do that.)

Trac:
Keywords: TorBrowserTeam201707 deleted, TorBrowserTeam201707R added
Status: accepted to needs_review

We are not using rbm for the alphas yet. Thus, we'd need a Gitian patch. Yes, giving what amounts to a compiler change some testing in our nightly/alpha builds first sounds like a good idea.

Moreover, it seems we get 7c90d5921bd2cb678eec09d05b10ce6fd13463bc as well with this mingw-w64 bump which allows us to get rid of one of our tor-browser patches I think. We should test that, too, while we are at it.

Trac:
Component: Core Tor/Tor to Applications/Tor Browser
Status: needs_review to needs_revision
Milestone: Tor: 0.3.2.x-final to N/A
Keywords: TorBrowserTeam201707R deleted, TorBrowserTeam201707 added

As you "have stolen" this ticket from Core Tor :), it should be noted that the right fix for this bug is, as Jonathan Yong suggested, to "Use proper dllimport/dllexport in your code to avoid auto-imports." To check that you should compile Tor with --disable-auto-import for MinGW-w64. Arthur could also make Firefox compile with --disable-auto-import (and also explain Mozillians why not to use -mnop-fun-dllimport) and get another one bounty ;) In general, MinGW-w64 should remove --enable-auto-import by default, because future releases of Windows can enforce security, and such tricks will fail. Maybe, Arthur, might explain MinGW-w64 guys that they shouldn't "fix" incompatible programs (by default at least) with this dirty hack, which Arthur made much less dirty! (Ask for bounty from all MinGW-w64-based software vendors ;)

Trac:
Cc: tom to tom, mcs

Replying to cypherpunks:

As you "have stolen" this ticket from Core Tor :), it should be noted that the right fix for this bug is, as Jonathan Yong suggested, to "Use proper dllimport/dllexport in your code to avoid auto-imports." To check that you should compile Tor with --disable-auto-import for MinGW-w64.

Thanks for the good suggestion. I tried it but I already run into an error when openssl is building. Namely:

libcrypto.a(cryptlib.o):cryptlib.c:(.text+0x9): undefined reference to `__stack_chk_guard'
libcrypto.a(cryptlib.o):cryptlib.c:(.text+0x48): undefined reference to `__stack_chk_guard'
libcrypto.a(cryptlib.o):cryptlib.c:(.text+0xd4): undefined reference to `__stack_chk_guard'
libcrypto.a(cryptlib.o):cryptlib.c:(.text+0xe4): undefined reference to `__stack_chk_guard'
libcrypto.a(cryptlib.o):cryptlib.c:(.text+0x106): undefined reference to `__stack_chk_guard'
libcrypto.a(cryptlib.o):cryptlib.c:(.text+0x264): more undefined references to `__stack_chk_guard' follow
/var/tmp/dist/mingw-w64/lib/gcc/i686-w64-mingw32/5.1.0/../../../../i686-w64-mingw32/bin/ld: libcrypto.a(cryptlib.o): bad reloc address 0x200 in section `.rdata'
collect2: error: ld returned 1 exit status
make[4]: *** [link_a.cygwin] Error 1

I did not investigate further, but my best guess is the -fstack-protector implementation (when used with mingw-w64) relies on auto-import. Looks like we would need an additional patch for gcc or mingw or someplace to fix this.

Arthur could also make Firefox compile with --disable-auto-import (and also explain Mozillians why not to use -mnop-fun-dllimport) and get another one bounty ;)

Good point. I have opened #22917 (moved) to investigate this further. In the meantime I think we should go ahead with using the bumped version of mingw-w64 because it is working at least for now.

In general, MinGW-w64 should remove --enable-auto-import by default, because future releases of Windows can enforce security, and such tricks will fail. Maybe, Arthur, might explain MinGW-w64 guys that they shouldn't "fix" incompatible programs (by default at least) with this dirty hack, which Arthur made much less dirty!

My dear cypherpunk, maybe you would like to start that debate on the mingw-w64 discussion page? :)

Replying to arthuredelstein:

My dear cypherpunk, maybe you would like to start that debate on the mingw-w64 discussion page? :) That debate has a very long history (see, e.g. https://sourceforge.net/p/mingw-w64/discussion/723797/thread/9e2995ab/). Don't you want to say some words to push the discussion forward? :)

Replying to gk:

We are not using rbm for the alphas yet. Thus, we'd need a Gitian patch. Yes, giving what amounts to a compiler change some testing in our nightly/alpha builds first sounds like a good idea.

Moreover, it seems we get 7c90d5921bd2cb678eec09d05b10ce6fd13463bc as well with this mingw-w64 bump which allows us to get rid of one of our tor-browser patches I think. We should test that, too, while we are at it.

Thanks for pointing that out. Here's a gitian patch and the corresponding tor-browser.git patch: https://github.com/arthuredelstein/tor-browser-bundle/commit/22563 https://github.com/arthuredelstein/tor-browser/commit/22563

But if we transition to rbm first, we can also use these two patches instead: https://github.com/arthuredelstein/tor-browser-build/commit/22563 https://github.com/arthuredelstein/tor-browser/commit/22563

Note I have had some trouble building the gitian build, but I will try again today. I'm pretty confident it will work as the rbm build worked for me.

Trac:
Keywords: TorBrowserTeam201707 deleted, TorBrowserTeam201707R added
Status: needs_revision to needs_review

Okay, I applied both patches (commit 2e5a0f5570f5b2ba0bf9d84cd74b6553407a0435 to tor-browser-52.2.0esr-7.5-1 and commit b42927a08821f1e1d46267156c369e3b45379758 to master). However, we still need to retain a small part of the Tor Browser patch due to https://bugzilla.mozilla.org/show_bug.cgi?id=1372959. I added that one to tor-browser-52.2.0esr-7.5-1 as well (commit 84d370aab03e45fbafef90b0fd99153e45a1b64a).

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

I applied the tor-browser-build patch as commit 373cf261bd190d69669d1545b2b99c5a09c66d15.

Trac:
Keywords: N/A deleted, tbb-backport added

Here's a patch for the maint-7.0 branch: https://github.com/arthuredelstein/tor-browser-bundle/commit/22563+1

Not backported but will be available in 7.5.

Trac:
Keywords: tbb-backport, windows tor-client win32 tor-relay security hardening 031-backport deleted, 031-backport, hardening, tbb-not-backported, win32, tor-relay, tor-client, security, windows added

closed

moved to tpo/applications/tor-browser#22563 (closed)