Get sandbox compiled with mingw-w64

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: gk, Assigned: tjr)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla60
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [tor 16010][tor 23658][tor-standalone], sb-)

Attachments

(1 attachment, 44 obsolete attachments)

59 bytes, text/x-review-board-request
bobowen
: review+
Details
(Reporter)

Description

3 years ago
In bug 1042426 a switch to disable sandbox compilation got implemented allowing mingw-w64 to avoid that part of the code. For projects (like the Tor Browser) that use mingw-w64 for cross-compiling and that want to benefit from the enhanced security which sandboxing provides, this is no viable workaround. This bug is for fixing the underlying issues and getting a working sandbox in mingw-w64 builds.
(Reporter)

Updated

3 years ago
Whiteboard: [tor]

Comment 1

3 years ago
Posted patch WIP (obsolete) — Splinter Review
Here is a WIP patch. It's by no means ready for review nor to work. It lacks SmartSidestepResolverThunk::SmartStub port to GCC, testing and proper solutions for a few bugs and lack of SEH support. It, however, gets m-c to build on mingw-w64 with sandboxing enabled, so it's a good starting point for anyone interested.
Component: Security → Security: Process Sandboxing
I'm a little concerned about the number of changes to the imported Chromium code here.
I suspect they might cause maintenance headaches in the future.

I don't suppose there is a chance we could try to get any of the changes upstreamed?

Comment 3

3 years ago
Most of those changes are trivial and Chromium took similar changes for other areas from me in the past, so I think most of it should be possible to be upstreamed.
(In reply to Jacek Caban from comment #3)
> Most of those changes are trivial and Chromium took similar changes for
> other areas from me in the past, so I think most of it should be possible to
> be upstreamed.

That would be great.
I'm happy to help, if I can ... when taking updates, even trivial changes can be quite painful when spread across multiple files.

However, even with that said, I don't think we should block on it, as making sandboxing available for the Tor Browser is far more important than merging pain.

Updated

3 years ago
Whiteboard: [tor] → [tor], sb-
Priority: -- → P3
Blocks: 1260929
Whiteboard: [tor], sb- → [tor][tor-standalone], sb-
(Assignee)

Updated

2 years ago
Blocks: 1330608
(Reporter)

Comment 5

2 years ago
I attached a WIP based on Jacek's previous one which "works" with ESR 52. It is working around the same issues to a large extend.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 years ago
I've broken out the attached patchfiles by topic to make it easier to understand. 

The two biggest concerns are porting __try to __try1 (or removing the try/except handler entirely) and rewriting SmartStub (https://bugzilla.mozilla.org/attachment.cgi?id=8845583)

These patches complete the compilation but unfortunately the browser doesn't run.

Comment 28

2 years ago
mozreview-review
Comment on attachment 8845583 [details]
Bug 1230910 Obliterate SmartStub FIXME TODO

https://reviewboard.mozilla.org/r/118716/#review122106

::: security/sandbox/chromium/sandbox/win/src/sidestep_resolver.cc:153
(Diff revision 1)
>  //  [xxx]                             [saved ebx]                 [xxx]
>  //  [xxx]                             [saved ecx]                 [xxx]
>  //  [xxx]                             [saved edx]                 [xxx]
> +#if 0
> -__declspec(naked)
> +  __declspec(naked)
>  void SmartSidestepResolverThunk::SmartStub() {

As far as I can tell, I don't think the type of resolver gets used at all.
(Assignee)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8845580 [details]
Bug 1230910 Fix 'error: narrowing conversion of '2148073472u' from 'unsigned int' to 'int' inside { }'

https://reviewboard.mozilla.org/r/118710/#review126882

::: security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc:61
(Diff revision 1)
>        deny_only_sids.push_back(WinAccountCertAdminsSid);
>        deny_only_sids.push_back(WinAccountSchemaAdminsSid);
>        deny_only_sids.push_back(WinAccountEnterpriseAdminsSid);
>        deny_only_sids.push_back(WinAccountPolicyAdminsSid);
>        deny_only_sids.push_back(WinBuiltinHyperVAdminsSid);
> -      deny_only_sids.push_back(WinLocalAccountAndAdministratorSid);
> +      //deny_only_sids.push_back(WinLocalAccountAndAdministratorSid);

This needs breaking out to a separate commit.
By the way, assuming it sticks, you will almost certainly get bitrotted from the patches in bug 1337331.
Sorry about that, but I guess there's a chance it might fix more issues than it adds ...
(Reporter)

Comment 31

2 years ago
(In reply to Tom Ritter [:tjr] from comment #29)
> Comment on attachment 8845580 [details]
> Bug 1230910 Fix 'error: narrowing conversion of '2148073472u' from 'unsigned
> int' to 'int' inside { }'
> 
> https://reviewboard.mozilla.org/r/118710/#review126882
> 
> ::: security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc:61
> (Diff revision 1)
> >        deny_only_sids.push_back(WinAccountCertAdminsSid);
> >        deny_only_sids.push_back(WinAccountSchemaAdminsSid);
> >        deny_only_sids.push_back(WinAccountEnterpriseAdminsSid);
> >        deny_only_sids.push_back(WinAccountPolicyAdminsSid);
> >        deny_only_sids.push_back(WinBuiltinHyperVAdminsSid);
> > -      deny_only_sids.push_back(WinLocalAccountAndAdministratorSid);
> > +      //deny_only_sids.push_back(WinLocalAccountAndAdministratorSid);
> 
> This needs breaking out to a separate commit.

I think that should be fixed on the mingw-w64 side by adding the respective SID to WELL_KNOWN_SID_TYPE which should not be that complicated.

Comment 32

2 years ago
Yes, I sent a patch to mingw-w64. It should be in their Git soon.

Comment 33

2 years ago
It's in mingw-w64 Git now.
(Reporter)

Comment 34

2 years ago
Just to state where we are here right now (and maybe anyone has a debugging idea): I am using the patch in comment:5. With content sandbox level set to "0" I get a working Tor Browser (which is not overly surprising). However, with the level set to "1" I get what looks like child process crashes early on. I experience the behavior and log output mentioned in bug 1372222, although there is no update in my setup involved.

Any ideas on how to debug this are greatly appreciated: I somehow don't trigger this issue in my debug  build (but a bunch of others instead :) ) which makes it a bit hard for me to find out more. :/
(In reply to Georg Koppen from comment #34)
> Just to state where we are here right now (and maybe anyone has a debugging
> idea): I am using the patch in comment:5. With content sandbox level set to
> "0" I get a working Tor Browser (which is not overly surprising). However,
> with the level set to "1" I get what looks like child process crashes early
> on. I experience the behavior and log output mentioned in bug 1372222,
> although there is no update in my setup involved.
> 
> Any ideas on how to debug this are greatly appreciated: I somehow don't
> trigger this issue in my debug  build (but a bunch of others instead :) )
> which makes it a bit hard for me to find out more. :/

I see that bug 1372222 was duped to one that was about issues with GetProcessBaseAddress.

On Saturday I landed a patch for bug 1385928 that takes a new implementation of GetProcessBaseAddress from chromium.
It looks like it will be much more reliable and certainly won't have the path comparison issues that plagued the old version in some circumstances.

Have you tried with a version of mozilla-central that has this patch?
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8845567 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845568 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845569 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845570 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845571 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845572 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845573 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845574 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845575 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845576 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845577 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845578 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845579 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845580 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845581 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845582 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845583 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845584 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845585 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8845586 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 58

2 years ago
(In reply to Bob Owen (:bobowen) from comment #35)
> (In reply to Georg Koppen from comment #34)
> > Just to state where we are here right now (and maybe anyone has a debugging
> > idea): I am using the patch in comment:5. With content sandbox level set to
> > "0" I get a working Tor Browser (which is not overly surprising). However,
> > with the level set to "1" I get what looks like child process crashes early
> > on. I experience the behavior and log output mentioned in bug 1372222,
> > although there is no update in my setup involved.
> > 
> > Any ideas on how to debug this are greatly appreciated: I somehow don't
> > trigger this issue in my debug  build (but a bunch of others instead :) )
> > which makes it a bit hard for me to find out more. :/
> 
> I see that bug 1372222 was duped to one that was about issues with
> GetProcessBaseAddress.
> 
> On Saturday I landed a patch for bug 1385928 that takes a new implementation
> of GetProcessBaseAddress from chromium.
> It looks like it will be much more reliable and certainly won't have the
> path comparison issues that plagued the old version in some circumstances.
> 
> Have you tried with a version of mozilla-central that has this patch?

-central needs a lot of work to build (and run) with MinGW. 

I've got the building part mostly done, and rebased my individual patches and attached them (messing up the mozreview history which I was trying to avoid but oh well.)

I have a try run going here about them rebased onto central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c15d28fbebeaf854b4dbd67817a785fce173a56
(Reporter)

Comment 60

2 years ago
(In reply to Bob Owen (:bobowen) from comment #35)
> (In reply to Georg Koppen from comment #34)
> > Just to state where we are here right now (and maybe anyone has a debugging
> > idea): I am using the patch in comment:5. With content sandbox level set to
> > "0" I get a working Tor Browser (which is not overly surprising). However,
> > with the level set to "1" I get what looks like child process crashes early
> > on. I experience the behavior and log output mentioned in bug 1372222,
> > although there is no update in my setup involved.
> > 
> > Any ideas on how to debug this are greatly appreciated: I somehow don't
> > trigger this issue in my debug  build (but a bunch of others instead :) )
> > which makes it a bit hard for me to find out more. :/
> 
> I see that bug 1372222 was duped to one that was about issues with
> GetProcessBaseAddress.
> 
> On Saturday I landed a patch for bug 1385928 that takes a new implementation
> of GetProcessBaseAddress from chromium.
> It looks like it will be much more reliable and certainly won't have the
> path comparison issues that plagued the old version in some circumstances.
> 
> Have you tried with a version of mozilla-central that has this patch?

Interesting. No, not yet. As Tom said getting m-c built with mingw-w64 is rather tricky right now. I looked a bit at backporting this to esr52 to try it on our tree but it seems that's not easily doable.

I tried to take Tom's build but that does not run. :(

So, I dug further into the issue I am experiencing. What breaks is the SpawnTask call in sandboxBroker.cpp:

result = sBrokerService->SpawnTarget(aPath, aArguments, mPolicy, &targetInfo);
if (sandbox::SBOX_ALL_OK != result) {
  return false;
}

result is 1, so SBOX_ERROR_GENERIC. Does that ring a bell?
(Reporter)

Comment 61

2 years ago
(In reply to Georg Koppen from comment #60)
> result is 1, so SBOX_ERROR_GENERIC. Does that ring a bell?

Following the advice to call GetLastError() to get the underlying error just gives me "0" back. So, it seems finding out something more that way is not going to work...
Ah this is ESR, the GetBaseAddress function was different still there and didn't involve path comparisons, so I suspect that isn't your issue.
You're possibly just getting similar results to bug 1372222 just because the child process isn't starting in both cases.

(In reply to Georg Koppen from comment #61)
> (In reply to Georg Koppen from comment #60)
> > result is 1, so SBOX_ERROR_GENERIC. Does that ring a bell?
> 
> Following the advice to call GetLastError() to get the underlying error just
> gives me "0" back. So, it seems finding out something more that way is not
> going to work...

Unfortunately as the name suggest SBOX_ERROR_GENERIC gets used in a number of places.

However digging through the the ESR52 instances of it I _think_ that it could only be returned from SpawnTarget from one place (with the policy being used) [1].

This is called before the process is actually created, so if you're seeing a child process then I must have missed something.

[1] https://dxr.mozilla.org/mozilla-esr52/rev/93c675fd8c3b52be09138305bd5ea97f355d699c/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc#463
(Reporter)

Comment 64

2 years ago
(In reply to Bob Owen (:bobowen) from comment #63)
> Looks like we could get it from SpawnCleanup here [2] as well if AddTarget
> fails.
> 
> [2]
> https://dxr.mozilla.org/mozilla-esr52/rev/
> 93c675fd8c3b52be09138305bd5ea97f355d699c/security/sandbox/chromium/sandbox/
> win/src/broker_services.cc#446

Yeah, that's the one I am hitting. I am still digging and trying to wrap my head around what is actually going wrong...
(Reporter)

Comment 65

2 years ago
I am still not at the bottom but it feels close. The issue comes from InterceptionManager::PatchClientFunctions(). And there we are failing when doing

if (it != interceptions_.end())
  return false;
(Reporter)

Comment 66

2 years ago
Okay, we are finally failing in GetProcAddress(). The particular code we hit (in ResolveTarget()) is:

*address = module_image.GetProcAddress(function_name);

if (NULL == *address) {
  NOTREACHED_NT();
  return STATUS_UNSUCCESSFUL;
}

*address being NULL.

Bob, any ideas how this can happen? What could be helpful further steps in understanding this issue?
Flags: needinfo?(bobowencode)
(In reply to Georg Koppen from comment #66)
...
> Bob, any ideas how this can happen? What could be helpful further steps in
> understanding this issue?

We used to get a crash in PEImage::GetProcAddress (bug 1271890), but that was fixed by an update to the sandbox back in Fx51.

I assume it's actaully PEImage::GetExportEntry that is failing and from there PEImage::GetExportDirectory or PEImage::GetProcOrdinal.

Do you know which of these is failing?
Flags: needinfo?(bobowencode)
(Reporter)

Comment 68

2 years ago
(In reply to Bob Owen (:bobowen) from comment #67)
> (In reply to Georg Koppen from comment #66)
> ...
> > Bob, any ideas how this can happen? What could be helpful further steps in
> > understanding this issue?
> 
> We used to get a crash in PEImage::GetProcAddress (bug 1271890), but that
> was fixed by an update to the sandbox back in Fx51.
> 
> I assume it's actaully PEImage::GetExportEntry that is failing and from
> there PEImage::GetExportDirectory or PEImage::GetProcOrdinal.
> 
> Do you know which of these is failing?

Yes, it is PEImage::GetProcOrdinal. More specifically we are failing at

if (cmp != 0)
  return false;

in the while loop. Enabling some logging gives me the following result 

function name not ordinal
function names are LdrGetDllHandle (function_name) and RtlIsThreadWithinLoaderCallout (name)
function names are LdrGetDllHandle (function_name) and NtSetInformationWorkerFactory (name)
function names are LdrGetDllHandle (function_name) and NtCreateThreadEx (name)
function names are LdrGetDllHandle (function_name) and LdrProcessRelocationBlock (name)
function names are LdrGetDllHandle (function_name) and EtwEventWriteStartScenario (name)
function names are LdrGetDllHandle (function_name) and KiRaiseUserExceptionDispatcher (name)
function names are LdrGetDllHandle (function_name) and LdrGetDllHandleByMapping (name)
function names are LdrGetDllHandle (function_name) and LdrEnumResources (name)
function names are LdrGetDllHandle (function_name) and LdrFindResourceEx_U (name)
function names are LdrGetDllHandle (function_name) and LdrFlushAlternateResourceModules (name)
function names are LdrGetDllHandle (function_name) and LdrGetDllHandle (name)
function name not ordinal
function names are _TargetNtCreateFile@48 (function_name) and TargetNtOpenThread@20 (name)
function names are _TargetNtCreateFile@48 (function_name) and g_handles_to_close (name)
function names are _TargetNtCreateFile@48 (function_name) and TargetNtSetInformationFile@24 (name)
function names are _TargetNtCreateFile@48 (function_name) and TargetNtUnmapViewOfSection@12 (name)
function names are _TargetNtCreateFile@48 (function_name) and TargetRegisterClassW@8 (name)
cmp is 11

The `LdrGetDllHandle` while the `_TargetNtCreateFile@48` case fails it seems.
(Reporter)

Comment 69

2 years ago
I meant "The `LdrGetDllHandle` part looks good".
(In reply to Georg Koppen from comment #68)
...
> function name not ordinal
> function names are _TargetNtCreateFile@48 (function_name) and
> TargetNtOpenThread@20 (name)
> function names are _TargetNtCreateFile@48 (function_name) and
> g_handles_to_close (name)
> function names are _TargetNtCreateFile@48 (function_name) and
> TargetNtSetInformationFile@24 (name)
> function names are _TargetNtCreateFile@48 (function_name) and
> TargetNtUnmapViewOfSection@12 (name)
> function names are _TargetNtCreateFile@48 (function_name) and
> TargetRegisterClassW@8 (name)
> cmp is 11

For my version of ESR52 I get _TargetNtOpenThread@20, _TargetNtCreateKey@32, _TargetCreateProcessW@44, _TargetNtCreateEvent@24 then _TargetNtCreateFile@48.

So, it looks like the list of exported names is in the wrong order (this is a binary search), they also seem to be missing the underscores.
(Reporter)

Comment 71

2 years ago
(In reply to Bob Owen (:bobowen) from comment #70)
> (In reply to Georg Koppen from comment #68)
> ...
> > function name not ordinal
> > function names are _TargetNtCreateFile@48 (function_name) and
> > TargetNtOpenThread@20 (name)
> > function names are _TargetNtCreateFile@48 (function_name) and
> > g_handles_to_close (name)
> > function names are _TargetNtCreateFile@48 (function_name) and
> > TargetNtSetInformationFile@24 (name)
> > function names are _TargetNtCreateFile@48 (function_name) and
> > TargetNtUnmapViewOfSection@12 (name)
> > function names are _TargetNtCreateFile@48 (function_name) and
> > TargetRegisterClassW@8 (name)
> > cmp is 11
> 
> For my version of ESR52 I get _TargetNtOpenThread@20, _TargetNtCreateKey@32,
> _TargetCreateProcessW@44, _TargetNtCreateEvent@24 then
> _TargetNtCreateFile@48.
> 
> So, it looks like the list of exported names is in the wrong order (this is
> a binary search), they also seem to be missing the underscores.

Thanks. Jacek: Any idea what could cause the missing underscores and the wrong order of exported names? Is that a known issue with workarounds?
Flags: needinfo?(jacek)
(Reporter)

Comment 72

2 years ago
Okay, some updates here. bobowen looked a bit more into this issue and there are two ways forward we might want to try:

1) We hit the problematic code paths as `SANDBOX_EXPORTS` gets defined. We can, however, try to avoid that as it is only needed for NPAPI and GMPs which we don't have (and don't recommend) in Tor Browser right now. Removing the entry in  security/sandbox/moz.build results in a broken compilation, though:

/home/firefox/win52/tor-browser/security/sandbox/chromium/sandbox/win/src/filesystem_dispatcher.cc: In member function ‘virtual bool sandbox::FilesystemDispatcher::SetupService(sandbox::InterceptionManager*, int)’:
/home/firefox/win52/tor-browser/security/sandbox/chromium/sandbox/win/src/interception.h:271:55: error: invalid conversion from ‘NTSTATUS (__attribute__((__stdcall__)) *)(NtCreateFileFunction, PHANDLE, ACCESS_MASK, POBJECT_ATTRIBUTES, PIO_STATUS_BLOCK, PLARGE_INTEGER, ULONG, ULONG, ULONG, ULONG, PVOID, ULONG) {aka long int (__attribute__((__stdcall__)) *)(long int (__attribute__((__stdcall__)) *)(void**, long unsigned int, _OBJECT_ATTRIBUTES*, _IO_STATUS_BLOCK*, _LARGE_INTEGER*, long unsigned int, long unsigned int, long unsigned int, long unsigned int, void*, long unsigned int), void**, long unsigned int, _OBJECT_ATTRIBUTES*, _IO_STATUS_BLOCK*, _LARGE_INTEGER*, long unsigned int, long unsigned int, long unsigned int, long unsigned int, void*, long unsigned int)}’ to ‘const void*’ [-fpermissive]
                         MAKE_SERVICE_NAME(service), id)
                                                       ^
/home/firefox/win52/tor-browser/security/sandbox/chromium/sandbox/win/src/interception.h:274:12: note: in expansion of macro ‘ADD_NT_INTERCEPTION’
   manager->ADD_NT_INTERCEPTION(service, id, num_params)
            ^
/home/firefox/win52/tor-browser/security/sandbox/chromium/sandbox/win/src/filesystem_dispatcher.cc:68:14: note: in expansion of macro ‘INTERCEPT_NT’
       return INTERCEPT_NT(manager, NtCreateFile, CREATE_FILE_ID, 48);
              ^
/home/firefox/win52/tor-browser/security/sandbox/chromium/sandbox/win/src/interception.h:108:8: note:   initializing argument 4 of ‘bool sandbox::InterceptionManager::AddToPatchedFunctions(const wchar_t*, const char*, sandbox::InterceptionType, const void*, sandbox::InterceptorId)’
   bool AddToPatchedFunctions(const wchar_t* dll_name,
        ^

The mingw-w64 related patches need probably an amendment to cover that issue as well

2) Another idea which Bob had is tackling the issue with the underscore. It seems to come from

https://dxr.mozilla.org/mozilla-esr52/source/security/sandbox/chromium/sandbox/win/src/interception.h#235 ff.

We could try to patch line 238 to make the code not look for the "_" (in addition to https://dxr.mozilla.org/mozilla-esr52/source/security/sandbox/chromium/sandbox/win/src/interception.cc#387 f.). That might be all that's needed.
(In reply to Georg Koppen from comment #72)
...
> 1) We hit the problematic code paths as `SANDBOX_EXPORTS` gets defined. We
> can, however, try to avoid that as it is only needed for NPAPI and GMPs
> which we don't have (and don't recommend) in Tor Browser right now. Removing

Just realised that we don't use our sandbox for 32-bit NPAPI, so for 32-bit it would only affect GMPs.
(Reporter)

Comment 74

2 years ago
I went ahead with 1) and it seems to work. I can browse the web without the browser exploding and `about:support` shows that the content sandbox level is set to `1`. I am optimistic that we get our patch set included into our upcoming alpha to get a broader testing of the approach. Thanks to bobowen for help.

https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_16010_v4&id=03833cf4c2a833f6e5420e92368ac2dae1b99c70 is the additional patch I am using and https://trac.torproject.org/projects/tor/ticket/16010#comment:53 has links to a test bundle
Flags: needinfo?(jacek)
I've tried that bundle and it works for me on Windows 7.
I can confirm that the content process is running at low integrity.

Just for information ... the levels we have defined were pretty arbitrary pegs to hang various policy settings onto, mainly for testing and roll-out.
In 52 there are also levels:
* 2 - doesn't gain you much, but probably doesn't break much if anything

* 10 - removes user's SID from access token (so blocks read access normally gained from that e.g. user's own files), also uses alternate desktop and separate job to remove other privileges
** general web browsing will probably be OK
** file:// URIs blocked (unless everyone can read)
** some styling issues
** probably some add-on problems

* 20 - similar to chromium renderer sandbox when it was added - currently breaks everything

That's probably not a complete list of issues, but using level 10 might be acceptable for many people.
(Reporter)

Comment 77

2 years ago
I added two patches (against ESR 52) we use that fix minor issues in Jacek's PoC.
Whiteboard: [tor][tor-standalone], sb- → [tor 16010][tor 23658][tor-standalone], sb-
(Assignee)

Updated

a year ago
Depends on: 1431621
(Assignee)

Updated

a year ago
Depends on: 1431797
(Assignee)

Updated

a year ago
Depends on: 1431801
(Assignee)

Updated

a year ago
Depends on: 1431803
(Assignee)

Updated

a year ago
Depends on: 1431807
(Assignee)

Updated

a year ago
Depends on: 1431809
(Assignee)

Updated

a year ago
Depends on: 1431825
(Assignee)

Updated

a year ago
Depends on: 1432239
(Assignee)

Updated

a year ago
Depends on: 1432295
(Assignee)

Updated

a year ago
Depends on: 1432313
(Assignee)

Updated

a year ago
Depends on: 1432319
(Assignee)

Updated

a year ago
Depends on: 1432381
(Assignee)

Updated

a year ago
Depends on: 1432505
(Assignee)

Updated

a year ago
Assignee: nobody → tom
Priority: P3 → P1
(Assignee)

Updated

a year ago
Depends on: 1432790
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8902808 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902809 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902810 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902811 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902812 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902813 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902814 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902815 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902816 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902817 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902818 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902819 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902820 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902821 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902822 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902823 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902824 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902825 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8902826 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8845566 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8912200 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8912199 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841521 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8696483 - Attachment is obsolete: true

Comment 79

a year ago
mozreview-review
Comment on attachment 8902807 [details]
Bug 1230910 Enable the sandbox for MinGW

https://reviewboard.mozilla.org/r/174480/#review221890
Attachment #8902807 - Flags: review?(bobowencode) → review+
(Assignee)

Updated

a year ago
Depends on: 1434316
(Assignee)

Updated

a year ago
No longer depends on: 1434316
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 80

a year ago
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d91302395a3
Enable the sandbox for MinGW r=bobowen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d91302395a3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
status-firefox45: affected → ---
(Assignee)

Updated

5 months ago
Blocks: 1498670
See Also: → bug 1498670
You need to log in before you can comment on or make changes to this bug.