[utils] DBus rules enforce stricter ordering of dbus attributes

I'm aware of this. The reason is that the tools use a regex instead of a "real" parser, which also means the parameter order gets enforced.

Changing this will be quite some work, so for now I recommend to use the default parameter order ;-)

Adding an apparmor-easyprof-ubuntu task because the Python utilities in AppArmor 2.11.0 can't handle some of the dbus rules that it generates. I'm hoping that we can work around this regression in the Python utils in the meantime. This bug renders the Python utils useless when certain packages containing AppArmor profiles generated via apparmor-easyprof-ubuntu, such as webbrowser-app, are installed on the system.

I'm bumping the importance of the upstream AppArmor task to high. I think this regression is going to affect a considerable number of users since there has never been a restriction on the ordering of dbus rule components.

Well, up to 2.10 dbus rule handling in the tools was simply matching for "dbus.*," and writing the line back to the profile without any changes. I'm not sure if I'd call full support for dbus rules (including handling of log events) a regression ;-) but I understand that it's annoying.

Writing a "real" parser in the python code would be quite some work, so - how difficult would it be to make apparmor_parser's code to parse dbus rules available via libapparmor? (Or a separate libapparmor_parser or libapparmor_private if you worry about including too much in libapparmor.)

On 01/24/2017 06:13 AM, Christian Boltz wrote:
> Well, up to 2.10 dbus rule handling in the tools was simply matching for
> "dbus.*," and writing the line back to the profile without any changes.
> I'm not sure if I'd call full support for dbus rules (including handling
> of log events) a regression ;-) but I understand that it's annoying.

If a user can't use tools such as aa-enforce/aa-complain/aa-disable
after updating to 2.11 because of this bug, it would certainly be a
regression in the eyes of that user. It is too bad that all of the
profiles have to be fully parsed just to use basic utilities that don't
necessarily care about the rules inside of a profile.

I do understand that it is very nice to have dbus rule support in the
utils. Very nice job on that!

> Writing a "real" parser in the python code would be quite some work, so
> - how difficult would it be to make apparmor_parser's code to parse dbus
> rules available via libapparmor? (Or a separate libapparmor_parser or
> libapparmor_private if you worry about including too much in
> libapparmor.)

I think writing a more complete parser in the python code would be a
mistake. Unfortunately, exposing the existing parser functionality in
libapparmor is a lot of work. It took me months to move the parser's
cache handling and policy loading functionality into libapparmor and I
think that is likely easier than moving the policy parsing code to
libapparmor. I think it is the right path forward for better integration
with the utils but I don't think it is likely to happen in the near term. :/

I've got a patch in progress that adjusts the dbus rule regex to accept
any order of dbus rule attributes at the expense of losing the ability
to detect multiple, repeated attributes. What this means is that the
following *valid* rule will parse:

  dbus path=/ bus=session,

But that the following *invalid* rule will also parse:

  dbus bus=session bus=system,

The utils will see that rule as 'dbus bus=system,' as the last match of
a given attribute will be used.

While not perfect, I think this is a better approach than refusing to
parse valid profiles that have existed for quite a few years. What do
you think?

> It is too bad that all of the
> profiles have to be fully parsed just to use basic utilities that don't
> necessarily care about the rules inside of a profile.

The main problem is that we allow "random" filenames for the profiles, so we need to check all files for the to-be-changed profile - but you probably already know that.

Yes, in theory we could just parse the headers and ignore the profile content, but that would mean that we need a (simplified, but still) copy of the profile parsing code.

> While not perfect, I think this is a better approach than refusing to
> parse valid profiles that have existed for quite a few years. What do
> you think?

I'm not the biggest fan of this workaround. Having the tools error out on invalid rules like your example would be much better - especially because such a rule will automagically be changed when saving the profile without any warning. Nevertheless, replacing "break the tools completely" with "unexpected bevaviour on invalid rules" still is a small improvement.

FYI: FileRule accepts the permissions in any order, so maybe you could look at how it's done there. (Needless to say that having a list of possible permissions is easier to handle, but maybe it helps nevertheless.)

Please don't forget to run "make check" for the utils ;-)

BTW: Does your patch also work for something like
    dbus bus=session bind bus=system,

On 01/27/2017 12:05 PM, Christian Boltz wrote:
> FYI: FileRule accepts the permissions in any order, so maybe you could
> look at how it's done there. (Needless to say that having a list of
> possible permissions is easier to handle, but maybe it helps
> nevertheless.)

I'll have a look at that.

>
> Please don't forget to run "make check" for the utils ;-)

That's why I haven't yet sent the patch out. Sorting out the test
changes that need to happen.

>
> BTW: Does your patch also work for something like
> dbus bus=session bind bus=system,

No. That's not a valid rule.

Feel free to send out what you have now (with a "just FYI, WIP" note) - maybe I can help in some details.

For "my" invalid rule: Well, I managed to pick an example that is "more invalid" than yours ;-)
What I wanted to know is - if there's another parameter between two bus=... parameters, will your patch still accept the rule? Let's say something like
    dbus send bus=session path=/com/example/path bus=system,
(Yes, that's an invalid rule because of the second "bus=...". I just want to know what your patch says about it ;-)

I submitted my patch to the list: https://lists.ubuntu.com/archives/apparmor/2017-February/010537.html

Committed as r3634