[et-mgmt-tools] [PATCH 5/5] "Launch virt-viewer" (new) browser plugin.

[Daniel P. Berrange]

On Fri, Aug 08, 2008 at 02:43:14PM +0100, Richard W.M. Jones wrote:
> +NPError
> +launcherRun (PluginInstance *self,
> +	     int16 argc, char* argn[], char* argv[])
> +{
> +  int i;
> +  const char *uri = NULL;
> +  const char *name = NULL;
> +  int direct = 0;
> +  int waitvnc = 0;
> +  int pid;
> +
> +  /* Protect against being called multiple times. */
> +  if (!self->launched) {
> +    self->launched = TRUE;
> +
> +    /* Parse the parameters given in <embed ...>. */
> +    i = argc;
> +    while (i > 0) {
> +      i --;
> +      if (argv[i] != NULL) {
> +        if (!PL_strcasecmp (argn[i], "URI"))
> +	  uri = argv[i];
> +        else if (!PL_strcasecmp (argn[i], "NAME"))
> +	  name = argv[i];
> +	else if (!PL_strcasecmp (argn[i], "DIRECT"))
> +	  direct = strcmp (argv[i], "1") == 0;
> +	else if (!PL_strcasecmp (argn[i], "WAITVNC"))
> +	  waitvnc = strcmp (argv[i], "1") == 0;
> +      }
> +    }
> +
> +    /* Create child process. */
> +    pid = fork ();
> +    if (pid < 0) {
> +      perror ("fork");
> +      return NPERR_GENERIC_ERROR;
> +    }
> +    if (pid == 0)		/* Child. */
> +      startVirtViewerAndExit (self, uri, name, direct, waitvnc);
> +    else			/* Parent. */
> +      waitpid (pid, NULL, 0);
> +  }
> +
> +  return NPERR_NO_ERROR;
> +}

Am I understanding this correctly, that it'll launch the virt-viewer
program immediately upon loading the HTML page containing the plugin
<embed> snippet ?  If so that's a huge security problem - you are
spawning a program which is allowed to connect to any host on the
internet. It is also a denial-of-service - malicous  javascript
could write a page containing thousands of <embed> snippets which
would spawn thousands of processes.

I'd rather expect the plugin to have a small embedded area in the
HTML page showing the details of what host will be connected to,
what port, and then a button which has to be explicitly pressed
to launch the external viewer.

For safety I think you also need to block all signals before doing
the fork() and reset signal handlers before unblocking and exec'ing,
and unblocking the parent. You don't want signal handlers from the
browser running in the child between the fork/exec window.

> +
> +static void
> +startVirtViewerAndExit (PluginInstance *self ATTRIBUTE_UNUSED,
> +			const char *uri, const char *name,
> +			int direct, int waitvnc)
> +{
> +  int pid;
> +  int i, maxfd, len;
> +  const char **argv;
> +
> +  /* Fork again to detach ourselves completely from the browser
> +   * process (probably not necessary on Windoze).
> +   */
> +  pid = fork ();

Well Windows doesn't have fork()/exec() at all does it ?

> +  if (pid < 0) {
> +    perror ("fork");
> +    return;
> +  }
> +  if (pid > 0)			/* Parent. */
> +    _exit (0);
> +
> +  /* Child ... Close any open file descriptors, leaving 0, 1 & 2
> +   * connected so that we'll still see any error messages.
> +   */
> +  maxfd = sysconf (_SC_OPEN_MAX) - 1;
> +  for (i = 3; i <= maxfd; ++i)
> +    close (i);
> +
> +  /* Build the argument array. */
> +  len = 1 +			/* "virt-viewer" (argv 0) */
> +    (uri ? 2 : 0) +		/* "-c" uri */
> +    (direct ? 1 : 0) +		/* "-d" */
> +    (waitvnc ? 1 : 0) +		/* "-w" */
> +    1 +				/* name */
> +    1;				/* NULL */
> +  argv = NPN_MemAlloc (sizeof (char *) * len);
> +  if (!argv) {
> +    perror ("malloc");
> +    goto cleanup;
> +  }
> +
> +  i = 0;
> +  argv[i++] = "virt-viewer";
> +  if (uri) {
> +    argv[i++] = "-c";
> +    argv[i++] = uri;
> +  }
> +  if (direct)
> +    argv[i++] = "-d";
> +  if (waitvnc)
> +    argv[i++] = "-w";
> +  argv[i++] = name;
> +
> +  argv[i++] = NULL;
> +
> +  if (i != len) {
> +    fprintf (stderr,
> +	     "virt-viewer launcher_plugin: internal error: i=%d != len=%d\n",
> +	     i, len);
> +    goto cleanup;
> +  }
> +
> +  execvp ("virt-viewer", argv);
> +  perror ("execvp");
> +
> + cleanup:
> +  _exit (1);
> +}
> +
> +void
> +launcherDestroy (PluginInstance *self)
> +{
> +  /* Nothing to free.  Just stop launcherRun from doing anything if
> +   * the browser tries to (incorrectly) call it a second time.  Not
> +   * that well-written browsers would do anything like that.  Oh no.
> +   */
> +  self->launched = TRUE;
> +}


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|