Opened 3 years ago

Closed 5 months ago

Last modified 5 weeks ago

#16010 closed task (fixed)

Get a working content process sandbox for Tor Browser on Windows

Reported by: gk Owned by: gk
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: ff52-esr, tbb-e10s, tbb-security, GeorgKoppen201709, TorBrowserTeam201709R, tbb-not-backported
Cc: mikeperry, mcs, arlolra, sukhbir, tom, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor4

Description

We are about to compile Tor Browser for Windows with --disable-sandbox as it breaks with mingw-w64 otherwise (https://bugzilla.mozilla.org/show_bug.cgi?id=1042426). One of the main problems is that SEH is not available in GCC mainly due to patent issues (See: https://gcc.gnu.org/wiki/WindowsGCCImprovements section Structured Exception Handling (SEH)). According to Jacek the patent expired but still there has no one written the proper code for GCC yet.

We might want to think about ways to get that fixed for us by third parties I guess.

Child Tickets

Attachments (2)

content-sandbox.patch (27.6 KB) - added by gk 12 months ago.
0001-Bug-16010-Enabling-the-sandbox-on-Windows.patch (859 bytes) - added by gk 5 months ago.

Download all attachments as: .zip

Change History (73)

comment:1 Changed 3 years ago by mcs

Cc: mcs added

comment:2 Changed 2 years ago by gk

Sponsor: SponsorU

These are SponsorU tickets

comment:3 Changed 2 years ago by arlolra

Cc: arlolra added

comment:4 Changed 2 years ago by sukhbir

Cc: sukhbir added

comment:5 Changed 2 years ago by gk

Severity: Normal

As an update: Jacek wrote a WIP patch that should be able to do the trick for us and I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1230910 to track the process of getting the problem fixed in Mozilla itself.

comment:6 Changed 2 years ago by gk

Keywords: ff52-esr added; ff45-esr removed
Sponsor: SponsorUNone

This won't be needed for esr45 as e10s is not shipped enabled. Postponing for esr52.

comment:7 Changed 14 months ago by gk

Parent ID: #21147

comment:8 Changed 14 months ago by tom

Cc: tom added

comment:9 Changed 13 months ago by gk

Keywords: TorBrowserTeam201702 added

We should start banging our head against this wall.

comment:10 Changed 13 months ago by gk

Sponsor: NoneSponsor4

This is Sponsor4 work

comment:11 in reply to:  5 Changed 12 months ago by gk

Replying to gk:

As an update: Jacek wrote a WIP patch that should be able to do the trick for us and I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1230910 to track the process of getting the problem fixed in Mozilla itself.

I adapted the WIP and attached a version which let me build with sandboxing enabled in ESR52. Now, we just need to get it work. :)

Changed 12 months ago by gk

Attachment: content-sandbox.patch added

comment:12 Changed 12 months ago by gk

I am still waiting of Jacek's but it seems to me the two obvious blockers are SEH support and getting that assembly code in SmartSidestepResolverThunk::SmartStub() ported. Not sure what to do about the assembly but https://gcc.gnu.org/wiki/WindowsGCCImprovements mentions Casper Hornstrup having created an initial implementation that did not make it into GCC mainline. Code not being in GCC mainline is no issue for us in general, so maybe we should try to dig that code up and see whether we can get it to work with GCC 5? Or even better getting Casper to make it work?

comment:13 Changed 12 months ago by gk

Keywords: tbb-7.0-must added

comment:14 Changed 12 months ago by gk

Keywords: TorBrowserTeam201703 added; TorBrowserTeam201702 removed

Moving tickets to March

comment:15 Changed 12 months ago by gk

Keywords: GeorgKoppen201703 added

Adding to my plate.

comment:16 Changed 11 months ago by gk

The backport of https://bugzilla.mozilla.org/show_bug.cgi?id=1321724 breaks our build again:

/home/firefox/win52/gecko-dev/security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc:61:32: error: ‘WinLocalAccountAndAdministratorSid’ was not declared in this scope
       deny_only_sids.push_back(WinLocalAccountAndAdministratorSid);

comment:17 Changed 11 months ago by cypherpunks

According to https://dxr.mozilla.org/mozilla-esr52/search?limit=100&redirect=false&q=__except%20path%3Asecurity/sandbox%2F you can use #12425 as an easy-fix/dirty-haxx just to get it working like #12113, but it's not safe, however.

comment:18 in reply to:  17 ; Changed 11 months ago by tom

Replying to cypherpunks:

According to https://dxr.mozilla.org/mozilla-esr52/search?limit=100&redirect=false&q=__except%20path%3Asecurity/sandbox%2F you can use #12425 as an easy-fix/dirty-haxx just to get it working like #12113, but it's not safe, however.

I'm pretty sure we cannot. try {} except {} can be replaced with setjmp/longjmp but try / except are a special MSVC extension that catches what would otherwise be a segfault.

Right now we're looking at a few options:
1) Rip out all try / except and just hope we don't hit an access violation in normal usage
2) MinGW's try1 / except1 construct
3) libseh from here: http://www.programmingunlimited.net/siteexec/content.cgi?page=mingw-seh

Preliminary testing of both 1 and 2 indicate these probably don't work. But we don't know exactly why yet.

comment:19 in reply to:  18 Changed 11 months ago by cypherpunks

Replying to tom:

Replying to cypherpunks:

According to https://dxr.mozilla.org/mozilla-esr52/search?limit=100&redirect=false&q=__except%20path%3Asecurity/sandbox%2F you can use #12425 as an easy-fix/dirty-haxx just to get it working like #12113, but it's not safe, however.

I'm pretty sure we cannot. try {} except {} can be replaced with setjmp/longjmp but try / except are a special MSVC extension that catches what would otherwise be a segfault.

See the link: sandbox doesn't catch segfaults, it seems. But if it does, sjlj is no op. (try/except? Maybe, C++ try/catch? __try/__except are for system SEH, Clang 3.7 claims to support that)

Right now we're looking at a few options:
1) Rip out all try /except and just hope we don't hit an access violation in normal usage
2) MinGW's try1 / except1 construct
3) libseh from here: http://www.programmingunlimited.net/siteexec/content.cgi?page=mingw-seh

Preliminary testing of both 1 and 2 indicate these probably don't work. But we don't know exactly why yet.

It's not a problem to use SEH, it's a huge problem to use it safely. MS uses version 4 (SEH4) or later of its implementation. There are a lot of undocumented tricks, hopefully, we don't need C++ EH stuff. But also without it, there are a lot of things to do.
(some code from the net: https://gist.github.com/kikairoya/1710310)

comment:20 Changed 11 months ago by gk

For posterity from Monday's meeting:

18:19:03 <tjr> I successfully built and ran a mingw build built off mozilla's esr52 with GeKo's patches from bug_21240 and the moz changeset used in that branch; then I did it again with an updated esr52 branch (which merged in the RegisterIdlePeriod commit). I had to add a single line patch to add an include to the up to date branch for some reason.
18:19:10 <tjr> Then I took the same branch and compiled it with the sandbox. I used all the non-__try patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1230910 (including the one that just annihilates SmartStub) AND I commented out all __try blocks so it should just crash immediately.
18:19:16 <tjr> It turns out e10s was not enabled, so sandboxing (I believe) wasn't doing anything. youtube.com showed some messed up rendering on the homepage even without e10s/sbox though: http://imgur.com/a/7X8ZP
18:19:30 <tjr> When I enabled it, the browser did not work at all. So something is breaking with regards to the sandbox. The debug build is blocked on the std::__throw issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1332747). I will investigate a fix for that.
18:19:39 <tjr> I also made a build trying out mingw's __try1 construct. This does not have the youtube artifact! But when I enable e10s it also doesn't work.

comment:21 Changed 11 months ago by gk

Keywords: TorBrowserTeam201704 added; TorBrowserTeam201703 removed

Moving tickets over to April

comment:22 Changed 11 months ago by gk

Parent ID: #21147

Not strictly needed for getting Tor Browser based on ESR52 compiled.

comment:23 Changed 11 months ago by gk

Keywords: tbb-7.0-must-alpha added; tbb-7.0-must removed

Getting this on our radar for alpha release in less than two weeks.

comment:24 Changed 11 months ago by cypherpunks

FWIW: is this the right place to discuss SEH? If so, it seems better to start with https://en.wikipedia.org/wiki/Microsoft-specific_exception_handling_mechanisms and go UCRT way.

comment:25 Changed 11 months ago by gk

Keywords: GeorgKoppen201704 added; GeorgKoppen201703 removed

Moving my tickets to April

comment:26 in reply to:  20 Changed 11 months ago by gk

Replying to gk:

For posterity from Monday's meeting:

18:19:10 <tjr> Then I took the same branch and compiled it with the sandbox. I used all the non-__try patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1230910 (including the one that just annihilates SmartStub) AND I commented out all __try blocks so it should just crash immediately.
18:19:16 <tjr> It turns out e10s was not enabled, so sandboxing (I believe) wasn't doing anything. youtube.com showed some messed up rendering on the homepage even without e10s/sbox though: http://imgur.com/a/7X8ZP

FWIW: I tested a build using the attached patch + the small fixup needed. And the build worked fine for me until I tried to enable e10s. I wonder whether that helps to explain your Youtube issues. Or does my build

https://people.torproject.org/~gk/testbuilds/firefox52-sandbox.zip
https://people.torproject.org/~gk/testbuilds/firefox52-sandbox.zip.asc

cause the same effects on your testing machine?

comment:27 Changed 11 months ago by cypherpunks

Detailed description of why SEH4 and SEHOP were created http://www.uninformed.org/?v=5&a=2&t=txt

comment:28 Changed 10 months ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201704 removed

Moving our tickets to May 2017.

comment:29 Changed 10 months ago by gk

Keywords: GeorgKoppen201705 added; GeorgKoppen201704 removed

Moving my tickets to May.

comment:30 Changed 9 months ago by gk

Keywords: tbb-7.0-must-alpha removed

That's important but won't make it into 7.0, alas.

comment:31 Changed 9 months ago by gk

Keywords: tbb-7.0-must added

We are beyond the alpha testing. Moving tickets for tbb-7.0-must.

comment:32 Changed 9 months ago by gk

Keywords: tbb-7.0-must removed

comment:33 Changed 9 months ago by cypherpunks

Keywords: tbb-e10s added

comment:34 Changed 9 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201705 removed

Moving our tickets to June.

comment:35 Changed 9 months ago by gk

Priority: HighVery High
Severity: NormalMajor

comment:36 Changed 9 months ago by gk

Keywords: GeorgKoppen201706 added; GeorgKoppen201705 removed

Moving my tickets to June

comment:37 Changed 8 months ago by cypherpunks

Georg, which way did you decide to go?
GCC's attempts seem to be finished with https://gcc.gnu.org/wiki/WindowsGCCImprovementsGSoC2008

comment:38 in reply to:  37 ; Changed 8 months ago by gk

Replying to cypherpunks:

Georg, which way did you decide to go?

Not sure yet. I ripped out all the problematic parts and got it compiled and running without exploding in my face. I'll post a build for others to test tomorrow probably. I hope to find cases where that build is exploding while normal Firefox works fine to fill then the missing gaps with something smart. :)

GCC's attempts seem to be finished with https://gcc.gnu.org/wiki/WindowsGCCImprovementsGSoC2008

Well, thanks for this page but looking at the SEH Suppport section it seems there was still a bunch of stuff to do back at that time.

comment:39 in reply to:  38 Changed 8 months ago by cypherpunks

Replying to gk:

Replying to cypherpunks:

Georg, which way did you decide to go?

Not sure yet. I ripped out all the problematic parts and got it compiled and running without exploding in my face.

You've removed sandbox protections, right? :)

I'll post a build for others to test tomorrow probably. I hope to find cases where that build is exploding while normal Firefox works fine to fill then the missing gaps with something smart. :)

Then you'll need to attack the chrome process through IPC, see https://dxr.mozilla.org/mozilla-esr52/source/security/sandbox/chromium/sandbox/win/src/crosscall_server.cc#131

GCC's attempts seem to be finished with https://gcc.gnu.org/wiki/WindowsGCCImprovementsGSoC2008

Well, thanks for this page but looking at the SEH Suppport section it seems there was still a bunch of stuff to do back at that time.

Yes, this page is about that they have a lot of things partially completed/investigated.

comment:40 Changed 8 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706 removed

Moving Tickets to July 2017.

comment:41 Changed 8 months ago by gk

Keywords: GeorgKoppen201707 added; GeorgKoppen201706 removed

Moving my tickets to July 2017.

comment:42 Changed 7 months ago by arthuredelstein

Cc: arthuredelstein added

comment:43 Changed 7 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:44 Changed 7 months ago by gk

Keywords: GeorgKoppen201708 added; GeorgKoppen201707 removed

Moving my tickets to August.

comment:45 Changed 6 months ago by gk

Status: newneeds_information

I've made some test builds enabling content sandboxing. Please test the following one and report back in case things explode for you or you encounter weird bugs:

https://people.torproject.org/~gk/testbuilds/torbrowser-install-16010-cs0_en-US.exe
https://people.torproject.org/~gk/testbuilds/torbrowser-install-16010-cs0_en-US.exe.asc

The branch this build is based upon is https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v2

It has the patch in this bug applied and makes sure the sandbox is compiled and its level is set to 0. Having it on level 0 does not do much to make the browsing experience safer, but it's a good starting point for making sure Tor Browser is running at all with sandboxing enabled adnd for tightening it towards level 1 (and higher) step-by-step while fixing issues as they come up.

The plan is to get as much of the level 1 mitigations enabled as fast as possible isolating the problematic ones to get them fixed easier.

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

comment:46 Changed 6 months ago by cypherpunks

Not sure what to test here...

Tor Browser is running at all with sandboxing enabled

Level 0 means it is disabled.
And at Level 1 it can't even start:

NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIScriptSecurityManager.getLoadContextCodebasePrincipal]  remote-browser.xml:376
TypeError: frameLoader.tabParent is null[Learn More]  remote-browser.xml:256:13
NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]  remote-browser.xml:44

comment:47 in reply to:  46 Changed 6 months ago by gk

Owner: changed from tbb-team to gk
Status: needs_informationassigned

Replying to cypherpunks:

Not sure what to test here...

Tor Browser is running at all with sandboxing enabled

Level 0 means it is disabled.

Yes, content sandboxing is disabled since Firefox 49 that way (even though https://wiki.mozilla.org/Security/Sandbox is currently suggesting otherwise and I found that out later). What I meant was to test whether it runs with the big patch applied and the configure switch removed that disabled the sandbox explicitly (and, so I assumed, with the things enabled which the wiki page claimed to be on level 0). But it seems it does, good.

And at Level 1 it can't even start:

NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIScriptSecurityManager.getLoadContextCodebasePrincipal]  remote-browser.xml:376
TypeError: frameLoader.tabParent is null[Learn More]  remote-browser.xml:256:13
NS_ERROR_NOT_INITI ALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]  remote-browser.xml:44

Yes, that's what I am seeing as well. Thanks for confirming.

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

comment:48 Changed 6 months ago by gk

Keywords: GeorgKoppen201709 added; GeorgKoppen201708 removed

Moving my tickets to the new month.

comment:49 Changed 6 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:50 Changed 6 months ago by gk

Keywords: tbb-rbm added; tbb-gitian removed

Moving over to rbm

comment:51 Changed 6 months ago by gk

Keywords: tbb-rbm removed

There does not seem to be any rbm/tor-browser-build work invovled right now, removing the keyword.

comment:52 in reply to:  51 ; Changed 5 months ago by cypherpunks

Replying to gk:

There does not seem to be any rbm/tor-browser-build work invovled right now, removing the keyword.

And include it in the next alpha as

it runs with the big patch applied and the configure switch removed

?

As for

the list of exported names is in the wrong order (this is a binary search), they also seem to be missing the underscores

The order and the number of symbols are correct. "function name not ordinal" means "it doesn't have an underscore" in this case. This is a violation of decorated naming convention.

comment:53 in reply to:  52 ; Changed 5 months ago by gk

Replying to cypherpunks:

Replying to gk:

There does not seem to be any rbm/tor-browser-build work invovled right now, removing the keyword.

And include it in the next alpha as

it runs with the big patch applied and the configure switch removed

?

I thought there are no specific tor-browser-build changes needed as tor-browser patches should take care of all the things (it turns out I was only partly right because the .mozconfig file is handled directly in tor-browser-build).

As for

the list of exported names is in the wrong order (this is a binary search), they also seem to be missing the underscores

The order and the number of symbols are correct. "function name not ordinal" means "it doesn't have an underscore" in this case. This is a violation of decorated naming convention.

What is a violation (or who is violating it)? Do you have a link with some more information about that?

That said, here is a build with sandboxing enabled that is working for me. Please test and report back:

https://people.torproject.org/~gk/testbuilds/torbrowser-install-16010-cs1_en-US.exe
https://people.torproject.org/~gk/testbuilds/torbrowser-install-16010-cs1_en-US.exe.asc

It is based on https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4.

comment:54 Changed 5 months ago by gk

Keywords: TorBrowserTeam201709R added; TorBrowserTeam201709 removed
Status: assignedneeds_review

bug_16010_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4) has the tor-browser patches for review. https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 has the additional code changes I needed to apply.

Attached is a fix for the tor-browser-build site as well as this is still needed due to different .mozconfig handlings.

comment:55 in reply to:  53 Changed 5 months ago by cypherpunks

Replying to gk:

What is a violation (or who is violating it)? Do you have a link with some more information about that?

"it doesn't have an underscore" is a violation of https://docs.microsoft.com/en-us/cpp/build/reference/decorated-names
According to unixwiz.net/techtips/win32-callconv.html, it is required only for direct linking to a third-party dll. So your way 2) should work, but it's better to fix it properly. Given that "Chrome doesn't use the SANDBOX_EXPORTS parts of the code, because they use the same EXE for the child", we are on terra incognita here for mingw-w64. Please, ask Jacek to find out what's going on.

Last edited 5 months ago by cypherpunks (previous) (diff)

comment:56 Changed 5 months ago by cypherpunks

Okay, as Bob Owen mentioned, chromium sandbox requires level 20, but crashes at startup with

Process Sandbox BLOCKED: NtCreateFile for : \??\pipe\chrome.3552.35.48659324
Stack Trace:
--#01: ???[C:\Windows\AppPatch\EMET.DLL +0x27089]
Process Sandbox Broker ALLOWED: NtCreateFile for : \??\pipe\chrome.3552.35.48659324
Process Sandbox BLOCKED: NtOpenThread
Stack Trace:
--#01: ???[C:\Windows\system32\KERNELBASE.dll +0x9e57]
Process Sandbox Broker ALLOWED: NtOpenThread
Process Sandbox BLOCKED: NtCreateFile for : \??\pipe\chrome.3552.36.121071419
Stack Trace:
--#01: ???[C:\Windows\AppPatch\EMET.DLL +0x27089]
Process Sandbox Broker ALLOWED: NtCreateFile for : \??\pipe\chrome.3552.36.121071419
Process Sandbox BLOCKED: NtCreateFile for : \??\pipe\chrome.3552.37.59242143
Stack Trace:
--#01: ???[C:\Windows\AppPatch\EMET.DLL +0x27089]
Process Sandbox Broker ALLOWED: NtCreateFile for : \??\pipe\chrome.3552.37.59242143
Process Sandbox BLOCKED: NtOpenKey for : \REGISTRY\MACHINE
Stack Trace:
--#01: CreateThread[C:\Windows\system32\kernel32.dll +0x4df59]
Process Sandbox BLOCKED: NtOpenKey for : \REGISTRY\USER
Stack Trace:
--#01: SetFileAttributesW[C:\Windows\system32\kernel32.dll +0x3b593]
Process Sandbox BLOCKED: NtOpenProcessToken
Stack Trace:
--#01: ???[C:\Windows\system32\KERNELBASE.dll +0x128e2]
Process Sandbox Broker ALLOWED: NtOpenProcessToken
Process Sandbox BLOCKED: NtOpenKeyEx for : \Registry\Machine\Software\Classes\CLSID\{BCDE0395-E52F-467C-8E3D-C4579291692E}
Stack Trace:
--#01: ReleaseActCtx[C:\Windows\system32\kernel32.dll +0x47692]
Process Sandbox BLOCKED: NtCreateFile for : \??\pipe\chrome.3552.38.63916814
Stack Trace:
--#01: ???[C:\Windows\AppPatch\EMET.DLL +0x27089]
Process Sandbox Broker ALLOWED: NtCreateFile for : \??\pipe\chrome.3552.38.63916814
Process Sandbox BLOCKED: NtOpenThread
Stack Trace:
--#01: ???[C:\Windows\system32\KERNELBASE.dll +0x9e57]
Process Sandbox Broker ALLOWED: NtOpenThread
Process Sandbox BLOCKED: NtOpenKey for : \Registry\Machine\Software\Microsoft\Windows\Tablet PC\
Stack Trace:
--#01: GetUserObjectInformationA[C:\Windows\system32\USER32.dll +0x7418]
Process Sandbox BLOCKED: NtOpenKeyEx for : \Registry\Machine\Software\Classes\CLSID\{E77CC89B-7401-4C04-8CED-149DB35ADD04}
Stack Trace:
--#01: ReleaseActCtx[C:\Windows\system32\kernel32.dll +0x47692]
Process Sandbox BLOCKED: NtCreateFile for : \??\pipe\chrome.3784.0.37962629
Stack Trace:
--#01: ???[C:\Windows\AppPatch\EMET.DLL +0x27089]
Process Sandbox Broker ALLOWED: NtCreateFile for : \??\pipe\chrome.3784.0.37962629
Process Sandbox BLOCKED: NtOpenKey for : \Registry\Machine\Software\Microsoft\Windows\Tablet PC\
Stack Trace:
--#01: GetUserObjectInformationA[C:\Windows\system32\USER32.dll +0x7418]

Then "level 10 might be acceptable for many people" is not true: it can't even display a context menu and

Process Sandbox BLOCKED: NtQueryAttributesFile for : \??\C:\Tor Browser\Browser\softokn3.dll
Stack Trace:
--#01: RtlExpandEnvironmentStrings[C:\Windows\SYSTEM32\ntdll.dll +0x60db1]
Process Sandbox BLOCKED: NtQueryAttributesFile for : \??\C:\Tor Browser\Browser\mozavutil.dll
Stack Trace:
--#01: RtlExpandEnvironmentStrings[C:\Windows\SYSTEM32\ntdll.dll +0x60db1]
Process Sandbox BLOCKED: NtQueryAttributesFile for : \??\C:\Tor Browser\Browser\mozavcodec.dll
Stack Trace:
--#01: RtlExpandEnvironmentStrings[C:\Windows\SYSTEM32\ntdll.dll +0x60db1]

Lower levels are acceptable. However, they give

08:21:12.871 NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindowID] 1 WebNavigationContent.js:158	
08:47:41.428 NS_BINDING_ABORTED: Component returned failure code: 0x804b0002 (NS_BINDING_ABORTED) [nsIStreamListener.onDataAvailable] 1 WebRequest.jsm:355	
09:53:28.611 [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.suspend]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/WebRequest.jsm :: maybeSuspend :: line 601"  data: no] 1 (unknown)	
	maybeSuspend resource://gre/modules/WebRequest.jsm:601:7
	HttpObserverManager.applyChanges< resource://gre/modules/WebRequest.jsm:749:24
	next self-hosted:1120:9
	TaskImpl_run resource://gre/modules/Task.jsm:319:42
	TaskImpl resource://gre/modules/Task.jsm:277:3
	createAsyncFunction/asyncFunction resource://gre/modules/Task.jsm:252:14
	runChannelListener resource://gre/modules/WebRequest.jsm:738:12
	observe resource://gre/modules/WebRequest.jsm:504:9

which may need further investigations.

To be on par with Mozilla, level 4 is suitable for the alphas. It could help to collect users' opinions about all the changes (and then downgrade if needed).

Very important side issue is that the sandboxing feature adds security.sandbox.content.tempDirSuffix pref which is a 128-bit GUID that allows to uniquely identify your copy of Tor Browser. It is persistent and leaves unique traces on every machine you use in user's %TEMP% folder.

Last edited 5 months ago by cypherpunks (previous) (diff)

comment:57 in reply to:  54 ; Changed 5 months ago by cypherpunks

Replying to gk:

bug_16010_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4) has the tor-browser patches for review. https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 has the additional code changes I needed to apply.

This makes NPAPI and GMP to fail silently. Gaining a sandbox for plugins seems to be worth fixing the problem with underscores.

comment:58 in reply to:  57 ; Changed 5 months ago by gk

Replying to cypherpunks:

Replying to gk:

bug_16010_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4) has the tor-browser patches for review. https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 has the additional code changes I needed to apply.

This makes NPAPI and GMP to fail silently. Gaining a sandbox for plugins seems to be worth fixing the problem with underscores.

Actually not NPAPI which would not be much of a problem. So, currently this would only affect GMPs but that's a thing we don't support out-of-the-box either. We don't want to have DRM GMPs and the OpenH264 one is not useful as we are disabling WebRTC.

That said Mozilla wants to get away from SANDBOX_EXPORTS as well, so this seems not a bad direction to move in and allows us to use our scarce resources somewhere else.

If anybody wants to investigate the underscores issue, please do. I'd like to know what's up with that one.

comment:59 in reply to:  58 Changed 5 months ago by cypherpunks

Replying to gk:

Replying to cypherpunks:

Replying to gk:

bug_16010_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4) has the tor-browser patches for review. https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 has the additional code changes I needed to apply.

This makes NPAPI and GMP to fail silently. Gaining a sandbox for plugins seems to be worth fixing the problem with underscores.

Actually not NPAPI which would not be much of a problem.

Some mozillians may think NPAPI was removed, and Flash is working through some magic. But that's not the case, and sandbox for it is really needed.

So, currently this would only affect GMPs but that's a thing we don't support out-of-the-box either. We don't want to have DRM GMPs and the OpenH264 one is not useful as we are disabling WebRTC.

WebRTC is needed too (but without holes).

That said Mozilla wants to get away from SANDBOX_EXPORTS as well, so this seems not a bad direction to move in and allows us to use our scarce resources somewhere else.

Do they want to rename all their processes to firefox.exe? (Previous attempt failed.)

If anybody wants to investigate the underscores issue, please do. I'd like to know what's up with that one.

It seemed Jacek liked such issues :) If so, ni him again.
But, doesn't it work without underscores with patching https://dxr.mozilla.org/mozilla-esr52/source/security/sandbox/chromium/sandbox/win/src/interception.h#238?

comment:60 Changed 5 months ago by mcs

Kathy and I reviewed the proposed changes. They seem fine although we admit that we do not understand this stuff very well. We do have a couple of questions about the "Bug 16010: Fixing sandbox compile issues" patch:

  1. For security/sandbox/chromium/sandbox/win/src/sidestep/mini_disassembler_types.h: is there a missing int in the declaration of flag_aux_? This probably does not matter because types default to int anyway.
  1. For security/sandbox/chromium/sandbox/win/src/sidestep_resolver.cc: do we know what the implications are for the FIXME (making SmartSidestepResolverThunk::SmartStub() a NOOP)?

comment:61 in reply to:  60 Changed 5 months ago by gk

Replying to mcs:

Kathy and I reviewed the proposed changes. They seem fine although we admit that we do not understand this stuff very well. We do have a couple of questions about the "Bug 16010: Fixing sandbox compile issues" patch:

  1. For security/sandbox/chromium/sandbox/win/src/sidestep/mini_disassembler_types.h: is there a missing int in the declaration of flag_aux_? This probably does not matter because types default to int anyway.

I think you are right, good catch. I'll make a fixup for that.

  1. For security/sandbox/chromium/sandbox/win/src/sidestep_resolver.cc: do we know what the implications are for the FIXME (making SmartSidestepResolverThunk::SmartStub() a NOOP)?

I trust bobowen when he says this one does not get used right now: https://bugzilla.mozilla.org/show_bug.cgi?id=1230910#c28. So, we should be fine.

comment:62 Changed 5 months ago by cypherpunks

Why not to use MOZ_SEH_TRY and MOZ_SEH_EXCEPT(expr) instead of #define __try if(true) and #define __except(x) else (they are 64-bit compatible)?

Last edited 5 months ago by cypherpunks (previous) (diff)

comment:63 in reply to:  54 ; Changed 5 months ago by arthuredelstein

Replying to gk:

bug_16010_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4) has the tor-browser patches for review. https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 has the additional code changes I needed to apply.

Attached is a fix for the tor-browser-build site as well as this is still needed due to different .mozconfig handlings.

I also had a look and didn't find any obvious errors, though I too am not familiar with the chromium sandbox code. I think it might be useful to briefly document the compile issues fixed in the first patch, either as comments or in the commit message.

One thing that puzzled me is the section here:
https://gitweb.torproject.org/user/gk/tor-browser.git/diff/security/sandbox/chromium-shim/base/win/sdkdecls.h?h=bug_16010_v4&id=4f613829fdcbf6dba4e80e8df1d356cb1c0a7de7
You have changed one constant integer to the uLL suffix, but the others remain ui64. I'm wondering if there's a reason for that.

Last edited 5 months ago by arthuredelstein (previous) (diff)

comment:64 Changed 5 months ago by cypherpunks

If you modify the declaration of flag_aux_ (and others) in Opcode struct, then why not in SpecificOpcode struct too?

comment:65 Changed 5 months ago by cypherpunks

FIXME !! for __declspec(naked) until GCC 8 is released (underscore bug is also mentioned). Can pospeselr do that?

comment:66 in reply to:  65 Changed 5 months ago by gk

Replying to cypherpunks:

FIXME !! for __declspec(naked) until GCC 8 is released (underscore bug is also mentioned). Can pospeselr do that?

We might not need that: https://bugzilla.mozilla.org/show_bug.cgi?id=1230910#c28.

comment:67 in reply to:  64 Changed 5 months ago by gk

Replying to cypherpunks:

If you modify the declaration of flag_aux_ (and others) in Opcode struct, then why not in SpecificOpcode struct too?

We don't need that at the moment it seems. But having a clean-up would be good to have, yes. See: #23659.

comment:68 in reply to:  63 Changed 5 months ago by gk

Replying to arthuredelstein:

Replying to gk:

bug_16010_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_16010_v4) has the tor-browser patches for review. https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 has the additional code changes I needed to apply.

Attached is a fix for the tor-browser-build site as well as this is still needed due to different .mozconfig handlings.

I also had a look and didn't find any obvious errors, though I too am not familiar with the chromium sandbox code. I think it might be useful to briefly document the compile issues fixed in the first patch, either as comments or in the commit message.

One thing that puzzled me is the section here:
https://gitweb.torproject.org/user/gk/tor-browser.git/diff/security/sandbox/chromium-shim/base/win/sdkdecls.h?h=bug_16010_v4&id=4f613829fdcbf6dba4e80e8df1d356cb1c0a7de7
You have changed one constant integer to the uLL suffix, but the others remain ui64. I'm wondering if there's a reason for that.

There is no reason for that. It's mainly Jacek PoC he whipped up to get the code compiled. It certainly needs some clean-up. We have #23659 for that.

comment:69 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, I made two fixups:

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.3.0esr-7.5-2&id=2354d122644d82df54d655ece5b42bdfa4cf38f8
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.4.0esr-7.5-1&id=0a9793458e9ddd5c7742d3ceb250125c52e8bf86

in addition to the patches that were up for review. (see: comment:54 for those). The ones for review got applied to tor-browser-52.3.0esr-75.-2 before rebasing:

commit 99e8c2c94986940de47d5f50a4b863cb6127df3d
commit 0b5dfeae5d09168e020acd2f630c5352674075ab
commit 6cafd21960e74a78317330c8559f643e4beac165.

The tor-browser-build change that landed is commit 682966b5b0faf438e69f61092487a6ea99534525. We are done here. I created the follow-up parent ticket #23658 to improve on the code we have right now and four child tickets. #23659, #23660, #23661, and #23664 addressing open issues like code clean-up and the GUID of the temp folder. Thanks all!

comment:70 Changed 5 months ago by gk

Keywords: tbb-backport added

comment:71 Changed 5 weeks ago by gk

Keywords: tbb-not-backported added; tbb-backport removed

Not backported but will be available in 7.5.

Note: See TracTickets for help on using tickets.