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 <st...@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 <appar...@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]
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
