Re: Process isolation support for start-stop-daemon

[Guillem Jover <guillem@debian.org>]

Hi!

On Sat, 2014-03-08 at 14:13:41 +0100, Andrew Shadura wrote:
> Yesterday I've hacked process isolation support into start-stop-daemon.
> The feature is available for Linux only, but the option itself is
> accepted even if the kernel support isn't available.
> 
> More details see in my blog,
> http://blog.shadura.me/2014/03/08/process-isolation-support-in-start-stop-daemon/
> 
> The patch is available here:
> https://bitbucket.org/andrew_shadoura/start-stop-daemon-isolate/commits/8646be59e3fde0b22f84a842ef5729a5de08fd3b/raw/

It would have been nice to send all the above to the list. It's way
easier to review. :)

> I also thought about adding support for moving network interfaces too,
> but it proved more complicated than I thought without using liblxc.
> 
> Please consider merging this patch, I think it may be useful for
> someone else, not just for me :)

I've taken a quick look, and the idea seems interesting, although I see
several issues I'd like to go over before considering merging this.

First, would be defining in a system-independent way what such option
implies, and how and if it can be extended in the future, like what
restrictions, guarantees and expectactions should the process get,
etc. Depending on those, the implementations might change, for example
FreeBSD's Capsicum might be too restrictive depending on the started
process and its expectations, but jails or Solaris Zones could cover
most of it. The Hurd might be able to use an interposed proc server,
perhaps.

In principle I think this should be mostly something that improves the
security of the started process, but not something the process should
depend for it to be secure at all. So if it ends up being a no-op on
some systems or on older kernels, that should not make the process a
security threat for example.

Then there's several implementation details, here's a short list from
a quick skimming (ignoring style issues and similar):

* The command-line option should always be visible in the help output.
* Some system calls are missing proper error checks.
* The quiet warnings seem suspect, I'd say they should either be
  actual errors or normal warnings.
* Why remount the /dev filesystems? /proc is needed to get the new
  PID namespace, but the others do not seem needed? And they are
  problematic as they might change depending on the system, for
  example /dev/shm is now /run/shm in Debian.
* The stack might grow upwards or downwards depending on the system.
* The dummy SIGTERM handler seems suspect, if it's needed (why?), it
  should exit(0), otherwise there's a race in the loop if the signal
  gets caught not during the wait(), which would then become ignored.
* Pointers should get NULL not 0.
* The two wait()s in the parent could be collapsed into one, and
  handle error conditions from normal conditions (EINTR and ECHILD vs
  the rest for example).
* The fatal() belongs with the execve(), which makes the “return;”
  unnecessary.

Thanks,
Guillem