Linux: bit hacky way to ensure Pepper plugins get loaded by zygote.

Linux: bit hacky way to ensure Pepper plugins get loaded by zygote.

BUG=49702
TEST=Integrated PDF plugin should work on Linux Chrome, and --register-pepper-plugins works on Linux even with sandbox enabled.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53977

[viettrungluu, 2010-07-21 17:58:20]

I said this on the bug, but code review is probably a better place: This patch
is not especially nice. Suggestions on how to make it better welcome.

[Evan Martin, 2010-07-21 18:24:33]

+agl, sandbox design master

http://codereview.chromium.org/3031011/diff/1/2
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/2#newcode108
chrome/browser/zygote_host_linux.cc:108: // the zygote before the sandbox is
enabled.
Is this because the sandboxed renderer can't open the .so file?  Or is it
something more complicated?

[darin (slow to review), 2010-07-21 18:27:06]

http://codereview.chromium.org/3031011/diff/1/2
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/2#newcode90
chrome/browser/zygote_host_linux.cc:90: if
(browser_command_line.HasSwitch(switches::kLoggingLevel)) {
the code for all 4 of these is identical.  how about writing a for loop?

http://codereview.chromium.org/3031011/diff/1/3
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/3#newcode551
chrome/browser/zygote_main_linux.cc:551: // Half-load Pepper plugins before the
sandbox is turned on.
This should probably be a function on PepperPluginRegistry.

http://codereview.chromium.org/3031011/diff/1/5
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/3031011/diff/1/5#newcode17
chrome/common/pepper_plugin_registry.h:17: bool internal;  // Defaults to false.
nit: is_internal

[agl, 2010-07-21 18:57:55]

http://codereview.chromium.org/3031011/diff/1/2
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/2#newcode108
chrome/browser/zygote_host_linux.cc:108: // the zygote before the sandbox is
enabled.
You may be interested to know that the following code works:


#include <dlfcn.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>

int main() {
  const int fd = open("/usr/lib64/libcrypto.so.1.0.0", O_RDONLY);
  if (fd < 0) {
    perror("open");
    return 1;
  }

  char buf[64];
  snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
  void* lib = dlopen(buf, RTLD_LAZY);
  if (!lib) {
    perror("dlopen");
    return 1;
  }

  void* p = dlsym(lib, "BN_rand");
  printf("p: %p\n", p);

  return 0;
}

[viettrungluu, 2010-07-22 06:24:54]

(I'll get to cleaning up this CL tomorrow.)

@agl: If I'm understanding you right, you're suggesting that I just open() in
the zygote and then dlopen("/proc/self/fd/N", ...) when needed? This sounds
appealing, but I have some questions/concerns:
- In the case of the chroot sandbox, wouldn't /proc/... inaccessible? (I admit,
I'm a bit confused that the subsequent, post-sandboxing dlopen()s still succeed,
though I suspect that ld.so just isn't smart enough to notice that it's been
chrooted. This is probably an argument to avoid the second call to dlopen().)
- Even if you could dlopen() via an fd, wouldn't it be problematic if a plugin
needed shared libraries other than the ones Chrome already brought in?

[agl, 2010-07-22 11:48:55]

On Thu, Jul 22, 2010 at 2:24 AM,  <viettrungluu@chromium.org> wrote:
> @agl: If I'm understanding you right, you're suggesting that I just open()
> in
> the zygote and then dlopen("/proc/self/fd/N", ...) when needed? This sounds
> appealing, but I have some questions/concerns:
> - In the case of the chroot sandbox, wouldn't /proc/... inaccessible? (I
> admit,
> I'm a bit confused that the subsequent, post-sandboxing dlopen()s still
> succeed,
> though I suspect that ld.so just isn't smart enough to notice that it's been
> chrooted. This is probably an argument to avoid the second call to
> dlopen().)
> - Even if you could dlopen() via an fd, wouldn't it be problematic if a
> plugin
> needed shared libraries other than the ones Chrome already brought in?

Sorry, I was just driving by in a rush. I think the current hack is
fine, my comment was just FYI because I didn't know if it would work.

The SUID sandbox would, indeed, block access to /proc/self/fd.
However, over time we want to switch to the seccomp sandbox, which is
flexible enough to allow that. Then, plugins could be passed over the
IPC channel from the browser to the renderer and linked into the
renderer process on demand.

However, that's probably a ways off.


AGL

[Evan Martin, 2010-07-22 15:02:32]

On Thu, Jul 22, 2010 at 4:48 AM, Adam Langley <agl@chromium.org> wrote:
> The SUID sandbox would, indeed, block access to /proc/self/fd.
> However, over time we want to switch to the seccomp sandbox, which is
> flexible enough to allow that. Then, plugins could be passed over the
> IPC channel from the browser to the renderer and linked into the
> renderer process on demand.
>
> However, that's probably a ways off.

At that point I think we could just allow blanket read access to
/opt/google/chrome/*.so too.

[sanjeevr, 2010-07-22 15:47:37]

I don't know enough about the Linux sandbox to be able to review this code for
anything but nits. LGTM if/when it is OK with the others.

[viettrungluu, 2010-07-22 18:15:01]

I cleaned it up a bit. It's still not my idea of optimal design, but that would
be more involved....

http://codereview.chromium.org/3031011/diff/1/2
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/2#newcode90
chrome/browser/zygote_host_linux.cc:90: if
(browser_command_line.HasSwitch(switches::kLoggingLevel)) {
On 2010/07/21 18:27:06, darin wrote:
> the code for all 4 of these is identical.  how about writing a for loop?

Done. Actually, it does no harm to AppendSwitchWithValue() even for those
without a value, so I included all the others as well.

http://codereview.chromium.org/3031011/diff/1/2#newcode108
chrome/browser/zygote_host_linux.cc:108: // the zygote before the sandbox is
enabled.
On 2010/07/21 18:24:34, Evan Martin wrote:
> Is this because the sandboxed renderer can't open the .so file?

Yes. (On other platforms, it's done in the renderer before enabling the
sandbox.)

> Or is it
> something more complicated?

No.

http://codereview.chromium.org/3031011/diff/1/3
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/3#newcode551
chrome/browser/zygote_main_linux.cc:551: // Half-load Pepper plugins before the
sandbox is turned on.
On 2010/07/21 18:27:06, darin wrote:
> This should probably be a function on PepperPluginRegistry.

Done. (I don't avoid calling LoadNativeLibrary() twice though, since doing so
would require rewiring the PepperPluginRegistry to not initialize plugins on
creation. Possibly, it should just create the list of plugins on creation, and
loading the modules and initializing them should be separate steps. Then the
PepperPluginRegistry could be used in other processes, instead of a bunch of
stateless static methods.)

http://codereview.chromium.org/3031011/diff/1/5
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/3031011/diff/1/5#newcode17
chrome/common/pepper_plugin_registry.h:17: bool internal;  // Defaults to false.
On 2010/07/21 18:27:06, darin wrote:
> nit: is_internal

Done.

[Evan Martin, 2010-07-22 18:23:09]

http://codereview.chromium.org/3031011/diff/1/2
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/2#newcode90
chrome/browser/zygote_host_linux.cc:90: if
(browser_command_line.HasSwitch(switches::kLoggingLevel)) {
On 2010/07/22 18:15:02, viettrungluu wrote:
> On 2010/07/21 18:27:06, darin wrote:
> > the code for all 4 of these is identical.  how about writing a for loop?
> 
> Done. Actually, it does no harm to AppendSwitchWithValue() even for those
> without a value, so I included all the others as well.

I am working on refactoring all of these loops in another CL, so I will clean
this up myself later.

[viettrungluu, 2010-07-22 19:03:49]

http://codereview.chromium.org/3031011/diff/1/2
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3031011/diff/1/2#newcode90
chrome/browser/zygote_host_linux.cc:90: if
(browser_command_line.HasSwitch(switches::kLoggingLevel)) {
On 2010/07/22 18:23:10, Evan Martin wrote:
> On 2010/07/22 18:15:02, viettrungluu wrote:
> > On 2010/07/21 18:27:06, darin wrote:
> > > the code for all 4 of these is identical.  how about writing a for loop?
> > 
> > Done. Actually, it does no harm to AppendSwitchWithValue() even for those
> > without a value, so I included all the others as well.
> 
> I am working on refactoring all of these loops in another CL, so I will clean
> this up myself later.

Is your preference for me to not put in the loop, or is what I have okay?

[Evan Martin, 2010-07-23 01:21:22]

On Thu, Jul 22, 2010 at 12:03 PM,  <viettrungluu@chromium.org> wrote:
>> I am working on refactoring all of these loops in another CL, so I
>
> will clean
>>
>> this up myself later.
>
> Is your preference for me to not put in the loop, or is what I have
> okay?

Whatevs, I will change it either way.

[viettrungluu, 2010-07-28 02:47:23]

@darin / anyone else: ping?

[agl, 2010-07-28 14:46:49]

On Tue, Jul 27, 2010 at 10:47 PM,  <viettrungluu@chromium.org> wrote
> @darin / anyone else: ping?

Nobody seems to be objecting, which I always take to be the quiet
version of LGTM ;)


AGL

[Evan Martin, 2010-07-28 15:21:05]

LGTM