[apparmor] [PATCH] aa-sandbox

[James Troup]

Jamie Strandboge <jamie at canonical.com> writes:

> diff -Naur -x .bzr -x .bzrignore apparmor-trunk/utils/aa-sandbox.pod apparmor-sandbox/utils/aa-sandbox.pod
> --- apparmor-trunk/utils/aa-sandbox.pod	1969-12-31 18:00:00.000000000 -0600
> +++ apparmor-sandbox/utils/aa-sandbox.pod	2012-08-24 12:15:51.000000000 -0500

[...]

> +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?

> +with B<aa-genprof> and B<aa-logprof>.

> diff -Naur -x .bzr -x .bzrignore apparmor-trunk/utils/apparmor/common.py apparmor-sandbox/utils/apparmor/common.py
> --- apparmor-trunk/utils/apparmor/common.py	1969-12-31 18:00:00.000000000 -0600
> +++ apparmor-sandbox/utils/apparmor/common.py	2012-08-23 20:49:01.000000000 -0500

[...]

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

'output' is unused.

> +def cmd_pipe(command1, command2):
> +    '''Try to pipe command1 into command2.'''

FWIW, this function doesn't appear to (yet?) be used.

> diff -Naur -x .bzr -x .bzrignore apparmor-trunk/utils/apparmor/sandbox.py apparmor-sandbox/utils/apparmor/sandbox.py
> --- apparmor-trunk/utils/apparmor/sandbox.py	1969-12-31 18:00:00.000000000 -0600
> +++ apparmor-sandbox/utils/apparmor/sandbox.py	2012-08-24 11:54:06.000000000 -0500

[...]

> +    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.

> +    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 '


[...]

> +def aa_exec(command, opt):

[...]

> +        policy_name = apparmor.sandbox.gen_policy_name(binary)

Any reason this couldn't be just 'policy_name = gen_policy_name(binary)'?

> +        easyp = apparmor.easyprof.AppArmorEasyProfile(binary, opt)
> +        params = apparmor.easyprof.gen_policy_params(policy_name, opt)
> +        policy = easyp.gen_policy(**params)
> +        debug("\n%s" % policy)


> +        if opt.withx == True:

As previously, don't do comparisons with True/False.

> +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?

> +            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.

> +    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?

> +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?

> +        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?

> +            if common.DEBUGGING == True:

As previously, don't do comparisons with True/False.

-- 
James