1461421 - --enable-sandbox crashes on MinGW x64 Build

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr]]

See https://bugs.torproject.org/24197 for Tor's bug

Comment 1

[Tom Ritter [:tjr]]

Some initial debugging indicates that the parent process is fine; but the child process is aborting here: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/ipc/glue/MessageChannel.cpp#2550

Which really doesn't tell us much. My previous forray into this type of crash had me spending a week or more on trying to understand what IPC message could be causing this crash; when it was in fact the other process dying. However I'm running the parent and it stays alive (as do the other child processes) so I guess I should be looking at the child to figure this out...

Comment 2

[Tom Ritter [:tjr]]

> [4636:Chrome_ChildThread]: W/chromium /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc:226: failed to create pipe: 5
> [4636, Chrome_ChildThread] WARNING: failed to create pipe: 5: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 226
> [4636:Chrome_ChildThread]: W/chromium /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc:59: Unable to create pipe named "4532.3.1048205453\1443326339" in client mode.
> [4636, Chrome_ChildThread] WARNING: Unable to create pipe named "4532.3.1048205453\1443326339" in client mode.: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 59
> ###!!! [Child][MessageChannel] Error: (msgtype=0xFFF8,name=<unknown IPC msg name>) Closed channel: cannot send/recv

5 is ERROR_ACCESS_DENIED and this is happening here: https://dxr.mozilla.org/mozilla-esr60/source/ipc/chromium/src/chrome/common/ipc_channel_posix.cc#201

Comment 3

[Bob Owen (:bobowen)]

(In reply to Tom Ritter [:tjr] from comment #2)
...
> 5 is ERROR_ACCESS_DENIED and this is happening here:
> https://dxr.mozilla.org/mozilla-esr60/source/ipc/chromium/src/chrome/common/
> ipc_channel_posix.cc#201

I'm guessing this is actually here:
https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/ipc/chromium/src/chrome/common/ipc_channel_win.cc#221

We add rules to the sandbox to allow access to these pipe paths here:
https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#518

Is something going wrong in the path matching somewhere?
If you turn on the sandbox logging you might get something (assuming it works for the MinGW build).

For debug builds just the following env var should get it in the normal debug output (redirect to a file for this to work from the sandboxed process):
* MOZ_SANDBOX_LOGGING=1

For opt that plus env vars (you can miss out the file and get it in stderr (or maybe stdout I forget)):
* MOZ_LOG="SandboxBroker:5,SandboxTarget:5,sync"
* MOZ_LOG_FILE=<some_log_file>

Comment 4

[Tom Ritter [:tjr]]

I'm using a debug build with

MOZ_DEBUG_CHILD_PAUSE=0
MOZ_LOG=SandboxTarget:5,SandboxBroker:5,sync
MOZ_PROCESS_LOG=process_log.txt
MOZ_QUIET=1
MOZ_SANDBOX_LOGGING=1

and running firefox.exe from a console and not seeing anything in the output aside from the prior errors; so I think I'm going to need to debug things in WinDbg.  (I also tried with MOZ_LOG_FILE=log1.txt)

Comment 5

[Tom Ritter [:tjr]]

Bob traced this down. 

We fail this check: https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/security/sandbox/chromium/sandbox/win/src/crosscall_server.cc#191

It's because on MinGW, sizeof(CrossCallParam) is 0x68 here: https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/security/sandbox/chromium/sandbox/win/src/crosscall_server.cc#160 even though the members only take up 0x64. That's fine; however...

When subclassed by ActualCallParams MinGW and MSVC/clang differ.  MinGW puts the first member of the subclass at 0x64 while MSVC/clang put it at 0x68.  That offset difference results in the problem.

Using sizeof(BaseClass) to learn offsets in a subclass does not seem spec-compliant. One ought to use offset of for this. The other option is to force MinGW to make the subclass' first member 8 byte aligned.

Both of the below builds' opt builds run, but....

alignas: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eae53a60820ff73f43615c12aa2a22e6e346f34
Commit: https://hg.mozilla.org/try/rev/3bbb11b6277a

offsetof: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76ea42ebbf65683b6d6de22f163ce2dc5b9eff4b
Commit: https://hg.mozilla.org/try/rev/321e6730944f0756b2581764c64c59a077b54a7c

Frankly, it's not clear to me why offsetof compiles (or runs). First off; this build has a typo: it should be offsetof(ActualCallParams, param_info_) - not parameters_.  Secondly, you can't call offsetof specifying a template class without the template variables. (I checked on a hello.cpp)

This build uses the correct parameter: https://treeherder.mozilla.org/#/jobs?repo=try&revision=159738a78c1ebaf13945c9bec4a42438ad202a19

Testing an older opt build confirms that opt builds don't work either so... I'm going to wait for the debug builds to investigate.

Comment 6

[Bob Owen (:bobowen)]

(In reply to Tom Ritter [:tjr] from comment #5)

I don't think these builds have the sandbox enabled, which probably explains the unexpected successful compilation.

Comment 7

[Tom Ritter [:tjr]]

(In reply to Bob Owen (:bobowen) from comment #6)
> (In reply to Tom Ritter [:tjr] from comment #5)
> 
> I don't think these builds have the sandbox enabled, which probably explains
> the unexpected successful compilation.

D'oh.  Trying again with the sandbox enabled....

Comment 8

[Tom Ritter [:tjr]]

Here's a version with alignas:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08d421aa9ecdbf288901669e009b3f4ea73679c1

And a version using offsetof:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12471c0710a8200c9c1dcf488c477c960603f05f

The opt build of both still crash; however I'll want to try to debug the debug versions which will take a while to complete (so leaving the links here so I don't lose them.)

Also: I had seen some crashes that related to jemalloc, so I excluded some jemalloc patches from this build.

Comment 9

[Tom Ritter [:tjr]]

Interestingly enough the offsetof build (no jemalloc) uses an offset of 0x64.  However the alignas build uses a sizeof 0x68, so that build seems to have fixed the initial problem.

However, we still get the same 'Aborting on Channel Error' assertion so something else seems to be wrong.

Comment 10

[Bob Owen (:bobowen)]

(In reply to Tom Ritter [:tjr] from comment #9)
> Interestingly enough the offsetof build (no jemalloc) uses an offset of
> 0x64.  However the alignas build uses a sizeof 0x68, so that build seems to
> have fixed the initial problem.

Sounds, like they both would have fixed it then (assumming the alignas also moved the first member of the subclass to 0x68).
If so, personally I think the offsetof solution is better.

Comment 11

[Tom Ritter [:tjr]]

There's still something screwy with the in-memory layout.  Now we fail the INVALID_TYPE type check: https://pastebin.mozilla.org/9086827

Comment 12

[Tom Ritter [:tjr]]

Success.  https://hg.mozilla.org/try/rev/06a74ef1519cef732d2f7d31b079192b1fbc6ebb (offsetof) seems to fix it.  I'll package this up as a real patch (with patchfile).

Comment 13

[Tom Ritter [:tjr]]

Attachment: Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

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

Comment 14

[Tom Ritter [:tjr]]

Attachment: Bug 1461421 Add OffsetOf patch to chromium patch directory

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

Comment 15

[Tom Ritter [:tjr]]

Attachment: Bug 1461421 Enable the sandbox for MinGW x64

Also disable jemalloc until Bug 1466192 is fixed

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

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

https://reviewboard.mozilla.org/r/250404/#review257136

::: security/sandbox/chromium/sandbox/win/src/crosscall_server.cc:73
(Diff revision 1)
>        return 0;
>    }
>  }
>  
> +// Returns the actual size for the parameters in an IPC buffer. Returns
> +// zero if the |param_count| is zero or too big.

Did you mean is less than zero here?

::: security/sandbox/chromium/sandbox/win/src/crosscall_server.cc:77
(Diff revision 1)
> +// Returns the actual size for the parameters in an IPC buffer. Returns
> +// zero if the |param_count| is zero or too big.
> +uint32_t GetOffsetOfFirstMemberOfActualCallParams(uint32_t param_count) {
> +  switch (param_count) {
> +    case 0:
> +      return offsetof(ActualCP0, param_info_);

Given we're using all the possible versions of ActualCallParams, couldn't we do offsetof parameters_ , maybe calling it GetMinDeclaredActualCallParamsSize and lose the " + ((param_count + 1) * sizeof(ParamInfo))" bits.

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

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

Comment 18

[Tom Ritter [:tjr]]

Comment on attachment 8984532 [details]
Bug 1461421 Add OffsetOf patch to chromium patch directory

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

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 8984533 [details]
Bug 1461421 Enable the sandbox for MinGW x64

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

Comment 20

[Tom Ritter [:tjr]]

Fixed up!

Comment 21

[Bob Owen (:bobowen)]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

https://reviewboard.mozilla.org/r/250404/#review258386

::: security/sandbox/chromium/sandbox/win/src/crosscall_params.h:63
(Diff revision 2)
>  // Maximum number of IPC parameters currently supported.
>  // To increase this value, we have to:
>  //  - Add another Callback typedef to Dispatcher.
>  //  - Add another case to the switch on SharedMemIPCServer::InvokeCallback.
>  //  - Add another case to the switch in GetActualAndMaxBufferSize
> +//  - Add another case to the switch in GetOffsetOfFirstMemberOfActualCallParams

This should now be GetMinDeclaredActualCallParamsSize.

::: security/sandbox/chromium/sandbox/win/src/crosscall_params.h:96
(Diff revision 2)
>    HANDLE handle;
>    // The array of extended values.
>    MultiType extended[kExtendedReturnCount];
>  };
>  
> +uint32_t GetOffsetOfFirstMemberOfActualCallParams(uint32_t param_count);

Assuming this all compiles, the rename proves we don't need this. :-)

::: security/sandbox/chromium/sandbox/win/src/crosscall_params.h:283
(Diff revision 2)
>    ParamInfo param_info_[NUMBER_PARAMS + 1];
>    char parameters_[BLOCK_SIZE - sizeof(CrossCallParams)
>                     - sizeof(ParamInfo) * (NUMBER_PARAMS + 1)];
>    DISALLOW_COPY_AND_ASSIGN(ActualCallParams);
> +
> +  friend uint32_t GetMinDeclaredActualCallParamsSize(uint32_t param_count);

I don't think we need this either GetMinDeclaredActualCallParamsSize is just a namespaced function like GetActualBufferSize.

::: security/sandbox/chromium/sandbox/win/src/crosscall_server.cc:72
(Diff revision 2)
>      default:
>        return 0;
>    }
>  }
>  
> +// Returns the actual size for the parameters in an IPC buffer. Returns

nit: s/actual/minimum/

Comment 22

[Bob Owen (:bobowen)]

Comment on attachment 8984532 [details]
Bug 1461421 Add OffsetOf patch to chromium patch directory

https://reviewboard.mozilla.org/r/250406/#review258392

Obviously this just needs the extra changes for part 1.

Comment 23

[Bob Owen (:bobowen)]

Comment on attachment 8984533 [details]
Bug 1461421 Enable the sandbox for MinGW x64

https://reviewboard.mozilla.org/r/250408/#review258394

\o/

Comment 24

[Tom Ritter [:tjr]]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

https://reviewboard.mozilla.org/r/250404/#review258386

> I don't think we need this either GetMinDeclaredActualCallParamsSize is just a namespaced function like GetActualBufferSize.

No; this one is actually needed, I couldn't compile without it.

Comment 25

[Tom Ritter [:tjr]]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

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

Comment 26

[Tom Ritter [:tjr]]

Comment on attachment 8984532 [details]
Bug 1461421 Add OffsetOf patch to chromium patch directory

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

Comment 27

[Tom Ritter [:tjr]]

Comment on attachment 8984533 [details]
Bug 1461421 Enable the sandbox for MinGW x64

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

Comment 28

[Tom Ritter [:tjr]]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

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

Comment 29

[Tom Ritter [:tjr]]

Comment on attachment 8984532 [details]
Bug 1461421 Add OffsetOf patch to chromium patch directory

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

Comment 30

[Tom Ritter [:tjr]]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

[Approval Request Comment]

This is one of several MinGW Build patches I'd like to land in esr60 for Tor. It will prevent them from carrying their own patches for the lifetime of esr60 and will enable us to keep the MinGW build functioning and know if/when/how it was broken by new commits into esr60.

This commit affects all Windows builds and is a very subtle change. However, it's very reliable: if there is an error in it, the browser will fail to start up, so we'd know about it right away.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf350ebb3004
Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class r=bobowen
https://hg.mozilla.org/integration/autoland/rev/028265406fe7
Add OffsetOf patch to chromium patch directory r=bobowen

Comment 32

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cf350ebb3004
https://hg.mozilla.org/mozilla-central/rev/028265406fe7

Comment 33

[Jacek Caban]

Here is try push with clang mingw build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d2e6d851c890cb1826139b30ecd3413b60dcfc2

Comment 34

[Jacek Caban]

(In reply to Jacek Caban from comment #33)
> Here is try push with clang mingw build:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0d2e6d851c890cb1826139b30ecd3413b60dcfc2

That push missed mozconfig change, here is a new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05cb2b926ddd28a8ae265d0093e91f49ca628b76

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984531 [details]
Bug 1461421 Use OffsetOf to calculate the location of parameters_ rather than making assumptions about the parent class

Makes downstream maintenance easier for Tor. Approved for ESR 60.2.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/4b45706bab90
https://hg.mozilla.org/releases/mozilla-esr60/rev/c633b410482c