Closed Bug 1313808 Opened 3 years ago Closed 3 years ago

Break plugin-container's dependency on libmozsandbox

Categories

(Core :: Security: Process Sandboxing, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: sb+)

Attachments

(3 files)

For bug 1277968 we're going to allow the firefox executable to run child processes, which means that any library dependencies in plugin-container's code for acting as a child process will be introduced into firefox, and we'd like to avoid that.  The libmozsandbox dependencies are:

* LinuxSandboxStarter, which can move back into libxul now that we won't need it in the executable after all

* The call to SandboxEarlyInit, which only needs to be this early on B2G, which is no more (and didn't need library depdendencies pruned from the main executable like on desktop anyway)

* The symbol-interposition wrappers for sigprocmask and pthread_sigmask, which need to read the signal number that libmozsandbox dynamically allocates to force all threads to start seccomp-bpf (if seccomp thread sync mode isn't available).  Bug 1313218 will take care of that by moving them out of the executable.
Comment on attachment 8809656 [details]
Bug 1313808 - Part 1: Move LinuxSandboxStarter back into libxul.

https://reviewboard.mozilla.org/r/92192/#review92630

::: ipc/app/moz.build:32
(Diff revision 1)
>      '/xpcom/base',
>  ]
>  
>  # We link GMPLoader into plugin-container on desktop so that its code is
>  # covered by the desktop DRM vendor's voucher.
> -if CONFIG['OS_TARGET'] != 'Android':
> +if CONFIG['OS_ARCH'] != 'Linux':

I'm assuming OS_ARCH=='Linux' on Android?

::: ipc/contentproc/plugin-container.cpp:168
(Diff revision 1)
>          mozilla::SanitizeEnvironmentVariables();
>          SetDllDirectoryW(L"");
>      }
>  #endif
> -#if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK) && defined(MOZ_PLUGIN_CONTAINER)
> +#if !defined(XP_LINUX) && defined(MOZ_PLUGIN_CONTAINER)
>      // On desktop, the GMPLoader lives in plugin-container, so that its

s/On desktop/On Windows and MacOS/

::: toolkit/xre/nsEmbedFunctions.cpp:320
(Diff revision 1)
>  #endif /* MOZ_CRASHREPORTER */
>  
> +#if defined (XP_LINUX) && defined(MOZ_GMP_SANDBOX)
> +namespace {
> +class LinuxSandboxStarter : public mozilla::gmp::SandboxStarter {
> +    LinuxSandboxStarter() { }

Looks like the LinuxSandboxStarter has tab width 4, but the rest of this file has tab width 2.
Attachment #8809656 - Flags: review?(cpearce) → review+
Comment on attachment 8809657 [details]
Bug 1313808 - Part 2: Move SandboxEarlyInit call into libxul.

https://reviewboard.mozilla.org/r/92194/#review92800

::: ipc/contentproc/plugin-container.cpp:32
(Diff revision 1)
> -#include "mozilla/SandboxInfo.h"
> -#endif
> -
>  #ifdef MOZ_WIDGET_GONK
>  # include <sys/time.h>
>  # include <sys/resource.h> 

while you edit the file, you could remove the trailing whitespace here
Attachment #8809657 - Flags: review?(julian.r.hector) → review+
Comment on attachment 8809656 [details]
Bug 1313808 - Part 1: Move LinuxSandboxStarter back into libxul.

https://reviewboard.mozilla.org/r/92192/#review92804

ok, when you fixed the 'issues' mentioned by :cpearce, it looks good to me
Attachment #8809656 - Flags: review?(julian.r.hector) → review+
Comment on attachment 8809658 [details]
Bug 1313808 - Part 3: GC the B2G-specific code in plugin-container.cpp.

https://reviewboard.mozilla.org/r/92196/#review92828
Attachment #8809658 - Flags: review?(wmccloskey) → review+
Comment on attachment 8809656 [details]
Bug 1313808 - Part 1: Move LinuxSandboxStarter back into libxul.

https://reviewboard.mozilla.org/r/92192/#review92978

::: toolkit/xre/moz.build:157
(Diff revision 1)
> +if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_ARCH'] == 'Linux':
> +    USE_LIBS += [
> +        'mozsandbox',
> +    ]

Seems it would be simpler to just add a FINAL_LIBRARY = 'xul' to the moz.build file for mozsandbox.
Attachment #8809656 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8809657 [details]
Bug 1313808 - Part 2: Move SandboxEarlyInit call into libxul.

https://reviewboard.mozilla.org/r/92194/#review92982

::: ipc/app/moz.build
(Diff revision 1)
> -    # gcc lto likes to put the top level asm in syscall.cc in a different partition
> -    # from the function using it which breaks the build.  Work around that by
> -    # forcing there to be only one partition.
> -    if '-flto' in CONFIG['OS_CXXFLAGS'] and not CONFIG['CLANG_CXX']:
> -        LDFLAGS += ['--param lto-partitions=1']
> -

The comment suggests there's no reason this shouldn't still be needed. Although that might be required in a different moz.build now.

It also seems part 1 and 2 ought to be folded together (does building with part 1 without part 2 produce a working build?)
Attachment #8809657 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #8)
> Seems it would be simpler to just add a FINAL_LIBRARY = 'xul' to the
> moz.build file for mozsandbox.

I thought that was only for static libraries; does it also work for shared libraries?  (libmozsandbox needs to be linked separately from libxul to avoid symbol conflicts between ipc/chromium/src and security/sandbox/chromium, so it can't be part of libxul.so; we also have this issue on Windows.)
Flags: needinfo?(mh+mozilla)
Oh, sorry, I thought it was a static library.
Flags: needinfo?(mh+mozilla)
OTOH, why is it not a static library if only libxul uses it?
Comment on attachment 8809657 [details]
Bug 1313808 - Part 2: Move SandboxEarlyInit call into libxul.

https://reviewboard.mozilla.org/r/92194/#review93284

::: ipc/app/moz.build
(Diff revision 1)
> -    # gcc lto likes to put the top level asm in syscall.cc in a different partition
> -    # from the function using it which breaks the build.  Work around that by
> -    # forcing there to be only one partition.
> -    if '-flto' in CONFIG['OS_CXXFLAGS'] and not CONFIG['CLANG_CXX']:
> -        LDFLAGS += ['--param lto-partitions=1']
> -

It's needed only for the linkage unit that contains the Chromium sandboxing code, so this actually hasn't been needed here since `mozsandbox` stopped being statically linked into `plugin-container` (bug 1268733).  There's already an instance of this in `security/sandbox/linux/moz.build` for `libmozsandbox.so`, which is the only build product that needs it now.

And I noticed while I was writing that that there's another copy of this in `ipc/app/pie`, which can also go away (especially because `MOZ_SANDBOX` has never been true for Android Fennec).

::: ipc/contentproc/plugin-container.cpp:32
(Diff revision 1)
> -#include "mozilla/SandboxInfo.h"
> -#endif
> -
>  #ifdef MOZ_WIDGET_GONK
>  # include <sys/time.h>
>  # include <sys/resource.h> 

This entire ifdef block gets deleted in the last patch.
(In reply to Mike Hommey [:glandium] from comment #9)
> It also seems part 1 and 2 ought to be folded together (does building with
> part 1 without part 2 produce a working build?)

It does; I tested it locally.
Comment on attachment 8809657 [details]
Bug 1313808 - Part 2: Move SandboxEarlyInit call into libxul.

https://reviewboard.mozilla.org/r/92194/#review94850
Attachment #8809657 - Flags: review?(mh+mozilla) → review+
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40c8ee5e5f1d
Part 1: Move LinuxSandboxStarter back into libxul. r=cpearce,glandium,tedd
https://hg.mozilla.org/integration/autoland/rev/b9993235ff99
Part 2: Move SandboxEarlyInit call into libxul. r=glandium,tedd
https://hg.mozilla.org/integration/autoland/rev/f591a4672390
Part 3: GC the B2G-specific code in plugin-container.cpp.  r=billm
Blocks: 1333511
No longer blocks: 1333511
Let it ride the train with 53
You need to log in before you can comment on or make changes to this bug.