Hello, On Sun, 16 Mar 2014 05:35:41 +0100 Guillem Jover <guillem@debian.org> wrote: > 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. That's what I thought as well, and that's the reason --isolate compiles to a no-op on systems which aren't enough capable: it *may* add some security, but it's not something to rely on. > 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. Probably if it's a no-op, there should be a notice about that then? > * 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. Well, those warnings don't necessarily mean something went wrong, which is why I set them to default-quiet mode. > * 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. That code it ported from lxc-unshare. I haven't checked if /dev/* things are really needed, so left it as is for a while. > * 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. Probably. I haven't thought about possible races. > * 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 for the comments, I think I may try to address them this week, but not sure when exactly :) -- Cheers, Andrew
Attachment:
signature.asc
Description: PGP signature