Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21514 closed task (fixed)

Deal with W^X backport bustage in upcoming ESR release

Reported by: gk Owned by: arthuredelstein
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: TorBrowserTeam201703R
Cc: arthuredelstein, brade, mcs, fdsfgs@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

https://hg.mozilla.org/releases/mozilla-esr45/rev/347c10e4d6d1 backports a security bug fix but is removing the nonWritableJitCode option we use for our W^X patch backport. It might be okay to add that part back as it seems the removal is unrelated. Jan de Mooij writes:

I also removed the nonWritableJitCode option as ESR45 predates W^X and nobody is going to enable that option there.

We should be sure about that though and make sure as well that our backported patches are still working and not causing unpredictable issues. If the risk is too high we need to back out the security improvements, alas.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by gk

Arthur: you backported the patches back then? Could you take a look at that one?

comment:2 Changed 3 years ago by gk

Description: modified (diff)

comment:3 Changed 3 years ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newaccepted

comment:4 Changed 3 years ago by gk

I've heard nothing back and needed to back the backported patches out for now to get a first build for 6.5.1 going. I think we could rebuild the firefox part if we have updated patches in time, though.

comment:5 Changed 3 years ago by arthuredelstein

Status: acceptedneeds_review

I believe I have finally got it working:

https://github.com/arthuredelstein/tor-browser/commit/21514+4

Thanks to Jan de Mooij for looking it over and helpful advice.

So far I have tested on Linux, but this will need testing on Windows and Mac at least to show that it doesn't cause a crash. I am reasonably confident the code covers all three platforms.

Jan also suggested we could examine the memory maps with and without the patch, using gdb or /proc/pid/maps on Linux and VMMap on Windows. He points out that without the patch we should hopefully observe multiple rwx regions.

comment:6 in reply to:  5 Changed 3 years ago by arthuredelstein

There are two more backported patches related to the JIT code. The first, 9979ff809363114d1435846a9331c7bcd2490a7c (Bug 1234246), I simply cherry-picked from our previous 45.7 branch. The second, 9c35223e6da4a29f48e1fba8b405aa92525d4bc2 (Bug 1238694) needed to be modified a little to be compatible with the security bug fix in 52419a78cd379b7b253d405c2ec65421210f10cb.

https://github.com/arthuredelstein/tor-browser/commits/21514+5

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:7 Changed 3 years ago by gk

Cc: brade mcs added
Keywords: TorBrowserTeam201703R added; TorBrowserTeam201702 removed

comment:9 Changed 3 years ago by mcs

Kathy and I reviewed the patches and they look okay to us. We also ran gk's test bundle on OSX and it seemed to work (we did notice that Unix domain sockets did not work for SOCKS, but that seems like a different problem... maybe a missing patch?)

We could not figure out how to use lldb to examine the memory protection level of the running process (and find the JS-related pages). Maybe if someone can describe how to do that with gdb on Linux we can do something similar on OSX.

comment:10 in reply to:  9 Changed 3 years ago by gk

Replying to mcs:

Kathy and I reviewed the patches and they look okay to us. We also ran gk's test bundle on OSX and it seemed to work (we did notice that Unix domain sockets did not work for SOCKS, but that seems like a different problem... maybe a missing patch?)

Yes, sorry. I just rebuilt the browser part of nightly builds I had around which contained torbutton master, while the browser part was based on 45.8.0esr-6.5. So, it needs probably a stable torbutton.

comment:11 in reply to:  9 Changed 3 years ago by arthuredelstein

I tested my own build of the 21514+5 branch on Linux. Using

cat /proc/$PID/maps | grep rwx

I confirmed that there were no rwx pages.

I also tried reverting the WX patch and then the same command showed several "rwxp" pages. So it looks like it is working as expected.

I'm not sure yet how to do the same thing on OS X, but the vmmap command line tool looks promising. Windows has VMMap (https://technet.microsoft.com/en-us/sysinternals/vmmap.aspx), which I am going to try next.

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

comment:12 Changed 3 years ago by arthuredelstein

I used VMMap on Windows to examine the memory protections of gk's build from comment:8. Good news and bad news. The bad news is there was one 4k page with RWX protection. The good news is that Jan helped me confirm that this block was far from (and therefore not part of) the 128 MB block allocated for the JIT. All the memory in the JIT block was RX or "Reserved".

So I think my JIT W^X patch is also working correctly on Windows. But it would be good to track down the cause of the RWX block. I opened #21617 to look at this problem.

comment:13 Changed 3 years ago by mcs

I used the vmmap command on a macOS Sierra (10.12.3) system to confirm that there are no rwx segments when the patches are present. Without them, I see several such segments (as I do with Firefox ESR 45.x). So I think this is working on OSX as well.

comment:14 Changed 3 years ago by tokotoko

Cc: fdsfgs@… added

comment:15 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Alright, thanks for the manual testing. I decided to include the new patch into the upcoming stable series as

1) we retain W^X JIT support that way
2) the patch looks good to all of us
3) the patch looks good to Jan de Mooij
4) the patch contains in large parts code that is in mozilla-central
5) we verified manually that it works on all three platforms
6) we did not encounter any crashes (which would be pretty easy to trigger (at least I'd assume so))

I kept the other two backported patches as well. Those three patches are on tor-browser-45.8.0esr-6.5-2 now as commit ad729fa2597d44b1d128a66d07b89e849e44cb80, a86798d665931a7250d44366faaad812a36d38b2, and f01f8bf4c84420e8f3538a4b9a978a7a2383a8c6.

Last edited 3 years ago by gk (previous) (diff)
Note: See TracTickets for help on using tickets.