Opened 2 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#22563 closed defect (fixed)

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

Reported by: arthuredelstein Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: windows tor-client win32 tor-relay security hardening 031-backport, TorBrowserTeam201707R
Cc: tom, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (14)

comment:1 Changed 2 months ago by nickm

Cc: tjr added

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

Adding tjr to cc

comment:2 Changed 2 months ago by nickm

Keywords: windows tor-client win32 tor-relay security hardening 031-backport added
Milestone: Tor: 0.3.2.x-final

comment:3 Changed 8 weeks ago by tom

Cc: tom added; tjr removed

comment:4 Changed 6 weeks ago by arthuredelstein

Owner: set to arthuredelstein
Status: newaccepted

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) as well.

comment:5 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201707 added

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

comment:6 Changed 5 weeks ago by arthuredelstein

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201707 removed
Status: acceptedneeds_review

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.)

comment:7 Changed 5 weeks ago by gk

Component: Core Tor/TorApplications/Tor Browser
Keywords: TorBrowserTeam201707 added; TorBrowserTeam201707R removed
Milestone: Tor: 0.3.2.x-final
Status: needs_reviewneeds_revision

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.

comment:8 Changed 5 weeks ago by 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.
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 ;)

comment:9 Changed 5 weeks ago by mcs

Cc: mcs added

comment:10 in reply to:  8 ; Changed 5 weeks ago by arthuredelstein

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 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? :)

comment:11 in reply to:  10 Changed 5 weeks ago by cypherpunks

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? :)

comment:12 in reply to:  7 Changed 3 weeks ago by arthuredelstein

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201707 removed
Status: needs_revisionneeds_review

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.

comment:13 Changed 3 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

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).

comment:14 Changed 3 weeks ago by boklm

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

Note: See TracTickets for help on using tickets.