1230910 - Get sandbox compiled with mingw-w64

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Georg Koppen]

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.

Comment 1

[Jacek Caban]

Attachment: WIP

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.

Comment 2

[Bob Owen (:bobowen)]

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

[Jacek Caban]

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.

Comment 4

[Bob Owen (:bobowen)]

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

Comment 5

[Georg Koppen]

Attachment: content-sandbox patches for esr52

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 6

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Allow MinGW and silence warning about Windows SDK version

On MinGW, these typedefs are the same, and mingw complains about duplicate instantiations.
Rather than use -fpermissive, just comment out the second instantiation.

Review commit: https://reviewboard.mozilla.org/r/118678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118678/

Comment 7

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Remove some __try blocks that are unneeded in our use case

Review commit: https://reviewboard.mozilla.org/r/118684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118684/

Comment 8

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix casing of include files so MinGW can compile

Review commit: https://reviewboard.mozilla.org/r/118686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118686/

Comment 9

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix constants so MinGW can compile

Review commit: https://reviewboard.mozilla.org/r/118688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118688/

Comment 10

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix 'error: stray '\' in program'

Review commit: https://reviewboard.mozilla.org/r/118690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118690/

Comment 11

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Remove some __try blocks and crash immediately

Review commit: https://reviewboard.mozilla.org/r/118692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118692/

Comment 12

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Cast GetProcAddress to (void*)

error: invalid conversion from 'FARPROC {aka int (__attribute__((__stdcall__)) *)()}' to 'void*' [-fpermissive]

According to http://stackoverflow.com/questions/13958081/, msvc does the fixuo
for itself automatically, while for mingw we need to do it manually.

Review commit: https://reviewboard.mozilla.org/r/118694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118694/

Comment 13

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Create a universal exception handler filter for mingw

Review commit: https://reviewboard.mozilla.org/r/118696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118696/

Comment 14

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Add interceptors.h as an include in interception_internal.h TODO - WHY?

Review commit: https://reviewboard.mozilla.org/r/118698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118698/

Comment 15

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 interception.h declares InterceptorId, but in some cases it is used without interceptors.h being included.

Review commit: https://reviewboard.mozilla.org/r/118700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118700/

Comment 16

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Map _Copy_s to copy for basic_string compatibility on MinGW

Review commit: https://reviewboard.mozilla.org/r/118702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118702/

Comment 17

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Declare operator new [](size_t, sandbox::AllocationType, void*)

Review commit: https://reviewboard.mozilla.org/r/118704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118704/

Comment 18

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Port simple __try blocks to __try1

If __try1 works at all, it should work for these.

Review commit: https://reviewboard.mozilla.org/r/118706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118706/

Comment 19

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Port complicated __try blocks to __try1

Not sure if __try1 will be able to handle these or not...

Review commit: https://reviewboard.mozilla.org/r/118708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118708/

Comment 20

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix 'error: narrowing conversion of '2148073472u' from 'unsigned int' to 'int' inside { }'

Review commit: https://reviewboard.mozilla.org/r/118710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118710/

Comment 21

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Cast address of function to void*

Review commit: https://reviewboard.mozilla.org/r/118712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118712/

Comment 22

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Ignore comment-based gcc warnings

Review commit: https://reviewboard.mozilla.org/r/118714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118714/

Comment 23

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Obliterate SmartStub FIXME TODO

Review commit: https://reviewboard.mozilla.org/r/118716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118716/

Comment 24

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Change ERROR_INVALID_TOKEN to GetLastError()

Review commit: https://reviewboard.mozilla.org/r/118718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118718/

Comment 25

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Replace GetExceptionCode() with a constant choice of EXCEPTION_ACCESS_VIOLATION.

While MinGW appears to define the macro (mapping it to the function _exception_code) there is no definition of the function, so it results in a linker error.

Review commit: https://reviewboard.mozilla.org/r/118720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118720/

Comment 26

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Use gcc intrinsic for return address on MinGW

Review commit: https://reviewboard.mozilla.org/r/118722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118722/

Comment 27

[Tom Ritter [:tjr]]

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

[Bob Owen (:bobowen)]

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.

Comment 29

[Tom Ritter [:tjr]]

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.

Comment 30

[Bob Owen (:bobowen)]

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

Comment 31

[Georg Koppen]

(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

[Jacek Caban]

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

Comment 33

[Jacek Caban]

It's in mingw-w64 Git now.

Comment 34

[Georg Koppen]

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. :/

Comment 35

[Bob Owen (:bobowen)]

(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 36

[Tom Ritter [:tjr]]

Comment on attachment 8845566 [details]
Bug 1230910 Allow MinGW and silence warning about Windows SDK version

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118678/diff/1-2/

Comment 37

[Tom Ritter [:tjr]]

Comment on attachment 8845566 [details]
Bug 1230910 Allow MinGW and silence warning about Windows SDK version

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118678/diff/2-3/

Comment 38

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Enable the sandbox for MinGW

On MinGW, these typedefs are the same, and mingw complains about duplicate instantiations.
Rather than use -fpermissive, just comment out the second instantiation.

Review commit: https://reviewboard.mozilla.org/r/174480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174480/

Comment 39

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Remove some __try blocks that are unneeded in our use case

Review commit: https://reviewboard.mozilla.org/r/174482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174482/

Comment 40

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix casing of include files so MinGW can compile

Review commit: https://reviewboard.mozilla.org/r/174484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174484/

Comment 41

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix constants so MinGW can compile

Review commit: https://reviewboard.mozilla.org/r/174486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174486/

Comment 42

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix 'error: stray '\' in program'

Review commit: https://reviewboard.mozilla.org/r/174488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174488/

Comment 43

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Remove some __try blocks and crash immediately

Review commit: https://reviewboard.mozilla.org/r/174490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174490/

Comment 44

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Cast GetProcAddress to (void*)

error: invalid conversion from 'FARPROC {aka int (__attribute__((__stdcall__)) *)()}' to 'void*' [-fpermissive]

According to http://stackoverflow.com/questions/13958081/, msvc does the fixup
for itself automatically, while for mingw we need to do it manually.

Review commit: https://reviewboard.mozilla.org/r/174492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174492/

Comment 45

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Create a universal exception handler filter for mingw

Review commit: https://reviewboard.mozilla.org/r/174494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174494/

Comment 46

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Add interceptors.h as an include in interception_internal.h TODO - WHY?

Review commit: https://reviewboard.mozilla.org/r/174496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174496/

Comment 47

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 interception.h declares InterceptorId, but in some cases it is used without interceptors.h being included.

Review commit: https://reviewboard.mozilla.org/r/174498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174498/

Comment 48

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Map _Copy_s to copy for basic_string compatibility on MinGW

Review commit: https://reviewboard.mozilla.org/r/174500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174500/

Comment 49

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Declare operator new [](size_t, sandbox::AllocationType, void*)

Review commit: https://reviewboard.mozilla.org/r/174502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174502/

Comment 50

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Port simple __try blocks to __try1

Review commit: https://reviewboard.mozilla.org/r/174504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174504/

Comment 51

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Port complicated __try blocks to __try1

Not sure if __try1 will be able to handle these or not...

Review commit: https://reviewboard.mozilla.org/r/174506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174506/

Comment 52

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Fix 'error: narrowing conversion of '2148073472u' from 'unsigned int' to 'int' inside { }'

Review commit: https://reviewboard.mozilla.org/r/174508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174508/

Comment 53

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Cast address of function to void*

Review commit: https://reviewboard.mozilla.org/r/174510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174510/

Comment 54

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Ignore comment-based gcc warnings

Review commit: https://reviewboard.mozilla.org/r/174512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174512/

Comment 55

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Obliterate SmartStub FIXME TODO

Review commit: https://reviewboard.mozilla.org/r/174514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174514/

Comment 56

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Horrible hack because ERROR_INVALID_TOKEN is not defined

Review commit: https://reviewboard.mozilla.org/r/174516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174516/

Comment 57

[Tom Ritter [:tjr]]

Attachment: Bug 1230910 Replace GetExceptionCode() with a constant choice of EXCEPTION_ACCESS_VIOLATION.

While MinGW appears to define the macro (mapping it to the function _exception_code) there is no definition of the function, so it results in a linker error.

Review commit: https://reviewboard.mozilla.org/r/174518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174518/

Comment 58

[Tom Ritter [:tjr]]

(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

Comment 59

[Tom Ritter [:tjr]]

That try run failed, this one succeeded: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ce79eb371e41346a6f2fdfc2974616cf1b10bc2

Comment 60

[Georg Koppen]

(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?

Comment 61

[Georg Koppen]

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

Comment 62

[Bob Owen (:bobowen)]

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

Comment 63

[Bob Owen (:bobowen)]

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

Comment 64

[Georg Koppen]

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

Comment 65

[Georg Koppen]

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;

Comment 66

[Georg Koppen]

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?

Comment 67

[Bob Owen (:bobowen)]

(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?

Comment 68

[Georg Koppen]

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

Comment 69

[Georg Koppen]

I meant "The `LdrGetDllHandle` part looks good".

Comment 70

[Bob Owen (:bobowen)]

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

Comment 71

[Georg Koppen]

(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?

Comment 72

[Georg Koppen]

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.

Comment 73

[Bob Owen (:bobowen)]

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

Comment 74

[Georg Koppen]

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

Comment 75

[Bob Owen (:bobowen)]

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.

Comment 76

[Georg Koppen]

Attachment: 0001-fixup-Bug-16010-Fixing-sandbox-compile-issues_ESR52.patch

Comment 77

[Georg Koppen]

Attachment: 0001-fixup-Bug-16010-Fixing-sandbox-compile-issues_2_ESR52.patch

I added two patches (against ESR 52) we use that fix minor issues in Jacek's PoC.

Comment 78

[Tom Ritter [:tjr]]

Comment on attachment 8902807 [details]
Bug 1230910 Enable the sandbox for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174480/diff/1-2/

Comment 79

[Bob Owen (:bobowen)]

Comment on attachment 8902807 [details]
Bug 1230910 Enable the sandbox for MinGW

https://reviewboard.mozilla.org/r/174480/#review221890

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d91302395a3
Enable the sandbox for MinGW r=bobowen

Comment 81

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6d91302395a3