1042426 - The chromium sandbox's use of __try/__except breaks mingw

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[(no longer active)]

I haven't tested this on minggw myself, but in clang-cl, I've seen build failures that make us fall back to the MSVC compiler since the sandbox code has been turned on by default.

Not sure how we're planning to address this, but I'm filing this so that we're aware of the issue.

Comment 1

[Brian R. Bondy [:bbondy]]

Is the eventual plan to have clang as the main compiler on Windows?  Otherwise can we just default sandbox to off for some compilers?  CC'ed TimAbraldes who's the sandbox owner on Windows.

Comment 2

[Jacek Caban]

mingw has even more problems with sandbox code. I have fixes for most of them in my tree, but a strange problem with STL code remains. I have disabled __try/__except blocks in my tree. I haven't looked at them closely yet. Are they critical to get sandbox running (as in Firefox will always crash without them) or are they additional security feature? If they are not critical, then I think fixing the compilation is preferable. Otherwise, I'm affraid that this will have to be disabled for mingw.

I'm happy to help working on this bug, but I will be mostly away until middle August.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I'm on board with disabling sandbox code on compilers that fail to build it.

The __try/__except is in Chromium code which a) we don't want to change because we want to be able to pull upstream changes easily and b) the upstream authors probably already would have fixed if this was an issue they cared about.

I am a bit surprised that __try/__except is causing an issue since there's also a usage of it in "nsprpub/pr/src/md/windows/ntthread.c" but maybe that doesn't build during mingw builds.

Comment 4

[(no longer active)]

(In reply to comment #3)
> I'm on board with disabling sandbox code on compilers that fail to build it.
> 
> The __try/__except is in Chromium code which a) we don't want to change because
> we want to be able to pull upstream changes easily and b) the upstream authors
> probably already would have fixed if this was an issue they cared about.
> 
> I am a bit surprised that __try/__except is causing an issue since there's also
> a usage of it in "nsprpub/pr/src/md/windows/ntthread.c" but maybe that doesn't
> build during mingw builds.

Those are hidden behind #ifdef _MSC_VER.

Comment 5

[(no longer active)]

(And we use MOZ_SEH_TRY/MOZ_SEH_CATCH in a couple of other places)

Comment 6

[Jacek Caban]

Yeah, that's not a problem. We could introduce similar abstraction in Chromium and I think that they don't have it now because they don't support clang/mingw at all.

My question was how critical working __try/__except are in runtime. For example, if they are meant to handle a critical errors in a more secure way, we can probably live with it for non-tier1 platforms (disabling is obviously also an option in this case). If they are part of the design and properly working code expects those exceptions to happen, then we need to disable the whole sandbox.

Comment 7

[(no longer active)]

From a quick look, it seems like the __try/__except blocks are used for setting the thread's name and also protecting against accessing invalid memory (wow!!!), so just removing them doesn't seem like an option.

Comment 8

[Jacek Caban]

Attachment: WIP

Here is a WIP patch that works for me with mingw build. I added a new configure that disables the whole sandbox support instead. It also adds new MOZ_SANDBOX define that mostly replaces adding defined(MOZ_CONTENT_SANDBOX) || defined(MOZ_GMP_SANDBOX) (and potentially more in the future) all over the place.

Here is try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88211e9a2f6d

Comment 9

[Jacek Caban]

The previous version had a typo:
https://tbpl.mozilla.org/?tree=Try&rev=78e86ebc6b45

Comment 10

[(no longer active)]

Comment on attachment 8478426 [details] [diff] [review]
WIP

Review of attachment 8478426 [details] [diff] [review]:
-----------------------------------------------------------------

bbondy can give you feedback on the idea, and glandium on the build system changes...

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8478426 [details] [diff] [review]
WIP

Review of attachment 8478426 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for checking.  Yep, I agree with the idea and the implementation.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8478426 [details] [diff] [review]
WIP

Review of attachment 8478426 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +6359,4 @@
>  dnl = Content process sandboxing
>  dnl ========================================================
>  if test -n "$gonkdir"; then
> +    MOZ_CONTENT_SANDBOX=

You're effectively changing the value of MOZ_CONTENT_SANDBOX on B2G.

@@ +6399,5 @@
>  fi
>  
>  AC_SUBST(MOZ_GMP_SANDBOX)
>  
> +if test -z "$MOZ_CONTENT_SANDBOX" -a -z "$MOZ_CMP_SANDBOX"; then

typo

Comment 13

[Jacek Caban]

Attachment: patch

Thanks for feedbacks. Try with the typo fixed is green:
https://tbpl.mozilla.org/?tree=Try&rev=78e86ebc6b45
The attached version also fixes the case where $gonkdir is specified.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8479015 [details] [diff] [review]
patch

Review of attachment 8479015 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the build system changes.

Comment 15

[Jacek Caban]

(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #14)
> r+ for the build system changes.

Thanks. I assume that's enough, given earlier feedback+:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a12ddb46882

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a12ddb46882

Comment 17

[Bob Owen (:bobowen)]

Does this new option work with the installer?

I've got a feeling that the installer expects sandboxbroker.dll to exist and I'm not sure how it handles things if it doesn't.

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Bob Owen (:bobowen) from comment #17)
> Does this new option work with the installer?
> 
> I've got a feeling that the installer expects sandboxbroker.dll to exist and
> I'm not sure how it handles things if it doesn't.

If I change the XP_LINUX case to unconditional like the Windows one, and then --disable-sandbox:

Error: /home/jld/obj/gecko-dev/obj-x86_64-unknown-linux-gnu/browser/installer/package-manifest:549: Missing file(s): bin/libmozsandbox.so

So I'm guessing that's how it would handle things.

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1042426-installer-stuff-hg0.diff

Here's a patch for the manifests.  I can refile it as a separate bug if that would make more sense.

Comment 20

[Jacek Caban]

I missed that because I used a build with installer disabled. Thank you for taking care of that.

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1284474ca9ac

And I'll need to go through the procedures for 34 branch uplift.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1284474ca9ac

Comment 23

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8480852 [details] [diff] [review]
bug1042426-installer-stuff-hg0.diff

Approval Request Comment
[Feature/regressing bug #]: Bug 1042426 — it's cleanup for the main part of this bug, which didn't miss the branch point like this patch did.
[User impact if declined]: --disable-sandbox builds on win32, including mingw, break the installer (and break `make install`, if it's anything like Unix).
[Describe test coverage new/current, TBPL]: Has gone through a few TBPL runs on mozilla-central by now.
[Risks and why]: Minimal — shouldn't change anything for other types of builds.
[String/UUID change made/needed]: None.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8480852 [details] [diff] [review]
bug1042426-installer-stuff-hg0.diff

This bug is difficult to follow as part of the fix landed on 34 and part landed on 35. To your question about filing another bug, in the future, I would prefer that a follow-up bug be filed for work that has to land after a merge has already been completed. I'm marking Aurora+ on this bug as the second patch has already landed on m-c 35.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91d7c9c5b349