1402133 - Precompiler directives inconsistent on former file ipc/contentproc/plugin-container.cpp and current file toolkit/xre/nsEmbedFunctions.cpp

Status: ASSIGNED

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

Description

[Fernando B. Nascimento]

Attachment: firefox-52.3.0esr-fix_plugin-container_directives.patch

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

Steps to reproduce:

Build Firefox on Raspberry Pi 2 using emerge from Gentoo.
It seems that the maintainer disabled the Mozilla sandbox at 2017-09-10, so may be the issue will not show to an actual Gentoo user but the bug remains on the source code.


Actual results:

The build fails with error:
{source code base dir}/firefox-52.3.0esr/ipc/contentproc/plugin-container.cpp:168:5: error: 'SandboxEarlyInit' is not a member of 'mozilla'

This occurs because of a typo on the file /firefox-52.3.0esr/ipc/contentproc/plugin-container.cpp

The patch to the 52.3.0 version, that solves this problem is attached.
The patch to the 52.2.0 version would be almost identical, except the name of the first directory on the header lines.

A bit longer explanation:

The function SandboxEarlyInit  is defined on the file "mozilla/Sandbox.h" or "mozilla/SandboxInfo.h" (I don't remember which one) and it compaion c, this is ok.

On the plugin-container.cpp file these are included on the lines 30 and 31. Well, for these includes take place, the compiler directive "#if defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)" have to be true, but the function on line 168 is run when the compiler directive on line 164 is true, and that is "#if defined(XP_LINUX) && defined(MOZ_SANDBOX)".

They are not equal! Of course there will be a problem!

The code uses a function that is on an include that can be excluded by the directives.


Expected results:

The compilation should finish with no errors.

Comment 1

[Fernando B. Nascimento]

After applying the provided patch, the compilation finishes with no error, and firefox works.

Comment 2

[Fernando B. Nascimento]

There is a bug filled on the Gentoo bugzilla about this:
https://bugs.gentoo.org/show_bug.cgi?id=630452

Comment 3

[Jory A. Pratt [:anarchy]]

(In reply to Fernando B. Nascimento from comment #0)
> Created attachment 8910952 [details]
> firefox-52.3.0esr-fix_plugin-container_directives.patch
> 
> User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36
> (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36
> 
> Steps to reproduce:
> 
> Build Firefox on Raspberry Pi 2 using emerge from Gentoo.
> It seems that the maintainer disabled the Mozilla sandbox at 2017-09-10, so
> may be the issue will not show to an actual Gentoo user but the bug remains
> on the source code.
> 
> 
> Actual results:
> 
> The build fails with error:
> {source code base
> dir}/firefox-52.3.0esr/ipc/contentproc/plugin-container.cpp:168:5: error:
> 'SandboxEarlyInit' is not a member of 'mozilla'
> 
> This occurs because of a typo on the file
> /firefox-52.3.0esr/ipc/contentproc/plugin-container.cpp
> 
> The patch to the 52.3.0 version, that solves this problem is attached.
> The patch to the 52.2.0 version would be almost identical, except the name
> of the first directory on the header lines.
> 
> A bit longer explanation:
> 
> The function SandboxEarlyInit  is defined on the file "mozilla/Sandbox.h" or
> "mozilla/SandboxInfo.h" (I don't remember which one) and it compaion c, this
> is ok.
> 
> On the plugin-container.cpp file these are included on the lines 30 and 31.
> Well, for these includes take place, the compiler directive "#if
> defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)" have to be true, but the
> function on line 168 is run when the compiler directive on line 164 is true,
> and that is "#if defined(XP_LINUX) && defined(MOZ_SANDBOX)".
> 
> They are not equal! Of course there will be a problem!
> 
> The code uses a function that is on an include that can be excluded by the
> directives.
> 
> 
> Expected results:
> 
> The compilation should finish with no errors.

You need to test with mozilla-central, the bug must be fixed there before any backport would occur.

Comment 4

[Fernando B. Nascimento]

Mozilla-central does not have these lines on this file anymore, them have been moved on this commit:
--------------------
b9993235ff99
created 2016-11-08 14:40 -0700 
pushed 2016-11-23 03:27 +0000
-----
Jed Davis
-----
Jed Davis - Bug 1313808 - Part 2: Move SandboxEarlyInit call into libxul. r=glandium,tedd
--------------------
The diff of this move can be viewed on https://hg.mozilla.org/mozilla-central/rev/b9993235ff99

But the code, on the destination, have the exact same problem:

lines 93-96:
--------------------
#if defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)
#include "mozilla/Sandbox.h"
#include "mozilla/SandboxInfo.h"
#endif
--------------------

lines 357-360:
--------------------
#if defined(XP_LINUX) && defined(MOZ_SANDBOX)
    // This has to happen while we're still single-threaded.
    mozilla::SandboxEarlyInit(XRE_GetProcessType());
#endif
--------------------

Look at the test on line 357, "defined(MOZ_SANDBOX)". The "GMP" is missing. Almost for sure it will fail.

The patch will be different, but with the same content.

I can't find the directory "mozilla" on the mozila-central, where can I find the files Sandbox.h and SandboxInfo.h?

If the mozilla-central is configured to be installed on the /opt will it interfere in any way with the system installed one, preferences, favorites or anything else?

I intend to try to build it tomorrow, but I don't know how to force the compiler to use this code, it may take me a few days. It is a litle tricker than just use "emerge" with the USE isn't it? Any advices?

If all goes well I will provide a patch to the new file that hold these lines now as well.

Comment 5

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

That is a mistake, but it's the conditional on the `#include <mozilla/Sandbox.h>` that's wrong; defined(MOZ_SANDBOX) is the correct condition for the call to SandboxEarlyInit().

The other problem here is Gentoo: they seem to be using --enable-content-sandbox for Firefox 55 on all platforms[1], which is probably broken on ARM even if it compiles, and shouldn't be necessary on x86.  Sandboxing is disabled by default on non-x86 (and isn't being tested, which is why I'm assuming it has bugs), so forcing --enable-content-sandbox gives a build where MOZ_CONTENT_SANDBOX and MOZ_SANDBOX are defined but MOZ_GMP_SANDBOX isn't.

This would also apply to Gentoo's Firefox 52 packages before the change mentioned in comment #0 — they used --enable-content-sandbox and --enable-sandbox, but --enable-sandbox doesn't actually do anything because MOZ_SANDBOX is already initialized to 1.  Yes, this is confusing; I'm starting to agree with bug 1375863 that we should just collapse everything into one build flag for all sandboxing.

Anyway, this should be reproducible on x86 by building with --disable-gmp-sandbox: not really a recommended configuration, especially if EME would be used, but not explicitly unsupported.

[1] https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/mozconfig-v6.55.eclass#n309

Comment 6

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

…except that --disable-gmp-sandbox doesn't exist and never has, and as the person who added MOZ_GMP_SANDBOX to configure.in I really ought to know that, but apparently I've been using it as shorthand for “MOZ_GMP_SANDBOX is not set” for long enough (e.g., bug 1141825) that I forgot that.

Even so, this is reproducible on x86 Linux by commenting out one line of old-configure.in.

I'm going to take this bug because I already have the patch.

Comment 7

[Fernando B. Nascimento]

I have two questions, but first let me see if I understood everything (please, correct me if I am wrong):

- The function of the sandbox is to isolate some tasks with lower privilleges;

- MOZ_GMP_SANDBOX is wrong even though it was more common on file ipc/contentproc/plugin-container.cpp. The right is MOZ_SANDBOX, so, at first, all ocurrences should be changed to MOZ_SANDBOX;

- Given the previous item, my patch to the 52.3.0 is doing the opposite of what should be done;

- The sandbox is not tested on anything non-x86 so it may be broken, despite this, bug reports from other architetures are accepted, even that they can take lower priority over x86 issues and so, normally, the time to deal with that issues are longer.

- --enable-content-sandbox is makes no difference on x86 but on everything else it is opposite of the default;

- --enable-sandbox will not change anything because MOZ_SANDBOX is initialized as 1 on all platforms, systems, etc.

- Gentoo is using, by default, configurations that does not change anything on x86, but non-x86 had content disabled and this is changed changes to enabled. I think Gentoo should keep the config this way and make the necessary changes to allow this to be controlled by the user (USE flags on Gentoo), and, if possible, disable the config by default on non-x86. I may be wrong, but I understand that when using USE flags they have to either enable or disable things, not having the option to keep the default (i.e. if it is possible to do +content-sandbox or -content-sandbox, the ebuild will pass either --enable-content-sandbox or  --disable-content-sandbox, but not leave it alone, so they can do the default + or - but it will ever be explicit to the configure script).

Questions:

- Should the Gentoo folks be told that they should do a patch to change MOZ_GMP_SANDBOX to MOZ_SANDBOX on everything?

- Considering arm (or any other non-x86), MOZ_SANDBOX is initialized as 1, but MOZ_CONTENT_SANDBOX is not defined and MOZ_GMP_SANDBOX doesn't exists, but reading bug 1375863, it seems that these was the only uses to the sandbox, is there other uses for the sandbox or being both disabled sandbox will be disabled also? I got really confused here.

- Jed said that have a patch, would it be applied to the current release only or also to previous versions? (as the files of the issue have changed)

- Must I just sit down and keep calm? ;-)

Comment 8

[Fernando B. Nascimento]

Well it was four questions not two! -.-

I see now that I misunderstold most of the things, MOZ_GMP_SANDBOX is right, what you said is that on the cited files the occurences should be MOZ_SANDBOX. From this I undertand now that the files on the title of the bug are not changing the use, or not use, of Gecko Media Plugin. It must check if sandbox is enabled not if GMP is enabled.

I would like just to call attention to these comments on two other bugs (cited above separated):
- bug 1375863 comment 0 where there is the citation "<bobowen> I agree, now that we sandboxing content on all platforms we can merge them";
- bug 1375863 comment 2 that what I understand is it has a suggestion to merge MOZ_GMP_SANDBOX and MOZ_CONTENT_SANDBOX into (already present) MOZ_SANDBOX; and
- bug 1141825 comment 0 that says "This indicates to me that --enable-gmp-sandbox / MOZ_GMP_SANDBOX should be changed to control whether GMP support is part of Gecko — and probably renamed to drop the “sandbox”."

But what happens if the platform does not support GMP and supports sandbox? Isn't it what old-configure.in is doing?

I am just putting them together to not forget one of them if a change/fix is made.

I will stop here as I don't know enough to go farer.