[apparmor] [patch] Remove pname to bin_name mapping in autodep()

[Kshitij Gupta]

Hello,

On Sun, Jan 3, 2016 at 11:21 PM, Christian Boltz <apparmor at cboltz.de> wrote:

> Hello,
>
> if autodep() is called with a pname starting with / (which can happen
> for (N)amed exec depending on the user input), this pname is mapped to
> bin_name.
>
> This might look like a good idea, however if the given pname doesn't
> exist as file on-disk, autodep() returns None instead of a (mostly
> empty) profile. (Reproducer: choose (N)amed, enter "/foo/bar")
>
> Further down the road, this results in two things:
> a) the None result gets written as empty profile file (with only a "Last
>    modified" line)
> b) a crash if someone chooses to add an abstraction to the None, because
>    None doesn't support the delete_duplicates() method for obvious
>    reasons ;-)
>
>
> Unfortunately this patch also introduces a regression - aa-logprof now
> fails to follow the exec and doesn't ask about the log events for the
> exec target anymore. However this doesn't really matter because of a) -
> asking and saving to /dev/null vs. not asking isn't a real difference
> ;-)
>
Where is the b)? The universe demands it!

>
> Actually the patch slightly improves things - it creates a profile for
> the exec target, but only with the depmod() defaults (abstractions/base)
> and always in complain mode.
>
>
> I'd prefer a patch that also creates a complete profile for the exec
> target, but that isn't as easy as fixing the issues mentioned above and
> therefore is something for the TODO list or a bugreport.
>
>
> BTW: the code that is removed with this commit was introduced in the old
> perl modules in r1097 with the not too helpful commit message
>     Add new exec modes and many bug fixes
> therefore I can't tell what the idea behind it was - any ideas?
>
>
> I propose this patch for trunk, 2.10 and 2.9.
> (2.9 "only" writes an empty file and doesn't crash - but writing an empty
> profile is still an improvement)
>
> I also won't complain if someone comes up with a better patch that
> follows the exec, asks for all events and creates the full profile.
>
> Damn! Some explanation for removing 2 lines of code ;-)

>
> [ 61-autodep-remove-pname-bin_name-mapping.diff ]
>
> === modified file ./utils/apparmor/aa.py
> --- utils/apparmor/aa.py        2016-01-03 16:31:01.336411176 +0100
> +++ utils/apparmor/aa.py        2016-01-03 16:47:43.205831925 +0100
> @@ -665,8 +665,6 @@
>  def autodep(bin_name, pname=''):
>      bin_full = None
>      global repo_cfg
> -    if not bin_name and pname.startswith('/'):
> -        bin_name = pname
>      if not repo_cfg and not cfg['repository'].get('url', False):
>          repo_conf = apparmor.config.Config('shell', CONFDIR)
>          repo_cfg = repo_conf.read_config('repository.conf')
>
> Thanks for the patch.

Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>
for trunk, 2.10 and 2.9(if you feel necessary).


> Regards,
>
> Christian Boltz
> --
> Und weshalb nicht vorerst weiterhin sysvinit benutzen? systemd
> ist so frisch und appetitlich wie ein dampfender Kuhfladen. ;)
> [Lars Müller in opensuse-de]
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
>


-- 
Regards,

Kshitij Gupta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160212/e29af8c8/attachment-0001.html>