[apparmor] [patch 3/4] utils/aa-unconfined: allow specifying ss/netstat binary locations

[Christian Boltz]

Hello,

Am Donnerstag, 29. Dezember 2016, 23:24:57 CET schrieb Steve Beattie:
> This patch allows a user to specify a specific location for ss or
> netstat for use in aa-unconfined, allowing a user to work around a
> tool that's buggy, uninstalled, or installed in an unexpected
> location. Note this option in the manpage.

How likely is it that someone really needs this?

I mean, if I have a broken ss or netstat installed, I'd want to fix it
for everything - but I wouldn't think about overriding it just for one
application, and leave everything else broken ;-)

So even without the security concerns, I tend to dislike this patch 
simply because I think these options are superfluous IMHO.


However, parts of the patch make sense. I'm talking about handing over
the command name as parameter to the function - this will allow to write
tests using a fake_ss and fake_netstat command.
(We might need to move out those functions to a python module when we
write those tests to avoid running the global code in aa-unconfined, but 
that can be easily done once someone writes such a test.)


> Signed-off-by: Steve Beattie <steve at nxnw.org>
> ---
>  utils/aa-unconfined     |   24 +++++++++++++++---------
>  utils/aa-unconfined.pod |    8 +++++---
>  2 files changed, 20 insertions(+), 12 deletions(-)
> 
> Index: b/utils/aa-unconfined
> ===================================================================
> --- a/utils/aa-unconfined
> +++ b/utils/aa-unconfined
> @@ -34,8 +34,8 @@ _ = init_translation()
>  parser = argparse.ArgumentParser(description=_("Lists unconfined
> processes having tcp or udp ports"))
> parser.add_argument("--paranoid", action="store_true", help=_("scan
> all processes from /proc")) bin_group =
> parser.add_mutually_exclusive_group()
> -bin_group.add_argument("--with-ss", action='store_true', help=_("use
> ss(8) to find listening processes (default)"))
> -bin_group.add_argument("--with-netstat", action='store_true',
> help=_("use netstat(8) to find listening processes"))
> +bin_group.add_argument("--with-ss", nargs='?', const='ss',
> metavar='SS_PATH', help=_("use ss(8) to find listening processes
> (default)")) +bin_group.add_argument("--with-netstat", nargs='?',
> const='netstat', metavar='NETSTAT_PATH', help=_("use netstat(8) to
> find listening processes")) args = parser.parse_args()
> 
>  paranoid = args.paranoid

As indicated above, I'd not add these options.

> @@ -50,17 +50,20 @@ def get_all_pids():
>      return set(filter(lambda x: re.search(r"^\d+$", x),
> aa.get_subdirectories("/proc")))
> 
> 
> -def get_pids_ss():
> +def get_pids_ss(ss):
>      '''Get a set of pids listening on network sockets via ss(8)'''
>      regex_lines =
> re.compile(r"^(tcp|udp|raw|p_dgr)\s.+\s+users:(?P<users>\(\(.*\)\))$"
> ) regex_users_pids = re.compile(r'(\("[^"]+",(pid=)?(\d+),[^)]+\))')
> 
> +    if ss is None:
> +        ss = 'ss'
> +

Just use
    def get_pids_ss(ss='ss'):
and drop the "if ss is None:" check again

>      pids = set()
>      my_env = os.environ.copy()
>      my_env['LANG'] = 'C'
>      my_env['PATH'] = '/bin:/usr/bin:/sbin:/usr/sbin'
>      for family in ['inet', 'inet6', 'link']:
> -        cmd = ['ss', '-nlp', '--family', family]
> +        cmd = [ss, '-nlp', '--family', family]
>          if sys.version_info < (3, 0):
>              output = subprocess.check_output(cmd, shell=False,
> env=my_env).split("\n") else:
> @@ -76,11 +79,14 @@ def get_pids_ss():
>      return pids
> 
> 
> -def get_pids_netstat():
> +def get_pids_netstat(netstat):
>      '''Get a set of pids listening on network sockets via
> netstat(8)''' regex_tcp_udp =
> re.compile(r"^(tcp|udp|raw)6?\s+\d+\s+\d+\s+\S+\:(\d+)\s+\S+\:(\*|\d+
> )\s+(LISTEN|\d+|\s+)\s+(?P<pid>\d+)\/(\S+)")
> 
> -    cmd = ['netstat', '-nlp', '--protocol', 'inet,inet6']
> +    if netstat is None:
> +        netstat = 'netstat'

Same here:
    def get_pids_netstat(netstat='netstat'):
instead of the "if netstat is None:" would be better if we only plan to 
use this parameter for testing.

> +    cmd = [netstat, '-nlp', '--protocol', 'inet,inet6']
>      my_env = os.environ.copy()
>      my_env['LANG'] = 'C'
>      my_env['PATH'] = '/bin:/usr/bin:/sbin:/usr/sbin'


> @@ -101,10 +107,10 @@ def get_pids_netstat():
>  pids = set()
>  if paranoid:
>      pids = get_all_pids()
> -elif args.with_ss or (not args.with_netstat and
> (os.path.exists('/bin/ss') or os.path.exists('/usr/bin/ss'))): -   
> pids = get_pids_ss()
> +elif args.with_ss is not None or (args.with_netstat is None and
> (os.path.exists('/bin/ss') or os.path.exists('/usr/bin/ss'))): +   
> pids = get_pids_ss(args.with_ss)
>  else:
> -    pids = get_pids_netstat()
> +    pids = get_pids_netstat(args.with_netstat)
> 
>  for pid in sorted(map(int, pids)):
>      try:

This change is superfluous if we don't add the parameters, and change
the function parameters to be optional as described above.

> Index: b/utils/aa-unconfined.pod
> ===================================================================
> --- a/utils/aa-unconfined.pod
> +++ b/utils/aa-unconfined.pod

The manpage also doesn't need any changes if we don't add the parameters 
;-)


To sum it up:
    Acked-by: Christian Boltz <apparmor at cboltz.de>
for adding the function parameters that make testing possible with a
fake_ss or fake_netstat script

The Ack does _not_ cover allowing users to specify the ss or netstat to 
use via commandline parameters - if you really want that, use John's ack 
for this part of the patch ;-)


Regards,

Christian Boltz
-- 
We work *with* SUSE, but not *for* SUSE.  Using @suse.de
would imply that to the world that we are somehow employed
by SUSE, and I haven't seen a paycheck from them yet.  :-)
[Bryen M Yunashko in opensuse-project]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20161230/30d01e35/attachment.pgp>