[apparmor] [PATCH] aa-sandbox

[Jamie Strandboge]

On Sat, 2012-08-25 at 10:23 +0100, James Troup wrote:
> Jamie Strandboge <jamie at canonical.com> writes:

> > +B<aa-sandbox> provides a mechanism for sandboxing an application using an
> > +existing profile or via dynamic profile generation. Please note that while this
> > +tool can help with quickly confining an application, its utility is dependent on
> > +the quality of the templates, policy groups and abstractions used. Also, this
> > +tool may create policy which is less restricted than creating policy by hand or
> 
> 'restrictive' rather than 'restricted' perhaps?
> 
Fixed

> > +def msg(out, output=sys.stdout):
> > +    '''Print message'''
> > +    try:
> > +        print("%s" % (out), file=sys.stdout)
> > +    except IOError:
> > +        pass
> 
> 'output' is unused.
> 
Fixed

> > +def cmd_pipe(command1, command2):
> > +    '''Try to pipe command1 into command2.'''
> 
> FWIW, this function doesn't appear to (yet?) be used.

Not yet, no, but I expect it to be.

> > +    if my_opt.debug == True:
> > +        common.DEBUGGING = True
> 
> Don't do comparisons with raw True/False (c.f. PEP8), i.e. just 'if
> my_opt.debug:' instead.

Fixed

> > +    if my_opt.withx and my_opt.xserver.lower() != 'xpra' and \
> > +                        my_opt.xserver.lower() != 'xpra3d' and \
> > +                        my_opt.xserver.lower() != 'xephyr':
> 
> 
> "if my_opt.withx and my_opt.xserver.lower() not in [ 'xpra', 'xpra3d', 'xephyr' ]:"
> 
> perhaps ?
> 
> > +            error("Invalid server '%s'. Use 'xpra', ''xpra3d', or 'xephyr'" % \
>                                                        ^^
> Should be single '
> 
Fixed


> > +        policy_name = apparmor.sandbox.gen_policy_name(binary)
> 
> Any reason this couldn't be just 'policy_name = gen_policy_name(binary)'?
> 
Nope. Fixed


> > +        if opt.withx == True:
> 
> As previously, don't do comparisons with True/False.
> 
Fixed

> > +class SandboxXserver():
> 
> [...]
> 
> > +    def find_free_x_display(self):
> > +        '''Find a free X display'''
> > +        display = ""
> > +        current = os.environ["DISPLAY"]
> > +        for i in range(1,257): # TODO: this puts an artificial limit of 256
> > +                               #       sandboxed applications
> 
> How come?

I like to put a limit on things that could be problematic. I wasn't sure
what that limit should be on this, if any, so it is a point of
discussion.

> 
> > +            tmp = ":%d" % i
> > +            os.environ["DISPLAY"] = tmp
> > +            rc, report = cmd(['xset', '-q'])
> > +            if rc != 0:
> > +                display = tmp
> > +                break
> 
> It probably doesn't matter, but needless to say, this approach is racy.

Right, this isn't there for security, but for usability. The code errors
out gracefully if it hits a display that is already used.

> > +    def cleanup(self):
> > +        '''Cleanup our forked pids, etc'''
> > +        # kill server now. It should've terminated, but be sure
> > +        for pid in self.pids:
> > +            os.kill(pid, 15)
> > +            os.waitpid(pid, 0)
> 
> s/15/signal.SIGTERM/ perhaps?  This also assumes that these processes
> are well behaved and will respect SIGTERM; perhaps that's safe, or
> perhaps it'd be more robust to SIGTERM, wait, then SIGKILL?
> 
Fixed

> > +class SandboxXpra(SandboxXserver):
> 
> [...]
> 
> > +    def start(self):
> > +        for e in ['xpra']:
> > +            debug("Searching for '%s'" % e)
> > +            rc, report = cmd(['which', e])
> > +            if rc != 0:
> > +                raise AppArmorException("Could not find '%s'" % e)
> 
> Loop of one?

What, you don't like that? I figured I might add more there. Fixed for
now.

> > +        xvfb_args = self._get_xvfb_args()
> > +        listener_x = os.fork()
> > +        if listener_x == 0:
> > +            # This will clean out any dead sessions
> > +            cmd(['xpra', 'list'])
> 
> It will?

Interestingly, yes. 'DEAD session's shouldn't happen normally, but it
seemed a painless enough way to keep things clean.


> > +            if common.DEBUGGING == True:
> 
> As previously, don't do comparisons with True/False.

Fixed.

Thanks for the review! :)