[apparmor] Mount flag handling

[Ash Wilson]

Greetings,

Recently, I submitted a patch to permit rules to accept mount calls that
specify both MS_REMOUNT and MS_BIND (or MS_REC | MS_BIND). However, there's
a bug remaining. There are a few different ways that it could be addressed
and I wanted to ask for feedback from someone more familiar with the
codebase than myself before I submitted a fix.

The problem occurs in the mnt_rule::gen_policy_re method in
parser/mount.cc. When handling rules that specify options=(remount, bind),
*two* rules are added to the policy: one to permit calls with MS_REMOUNT |
MS_BIND and another that permits all calls to MS_BIND, with all other
options masked out! The second rule is almost certainly unintentional. The
quickest fixes would be to add a clause to make the if-statements mutually
exclusive again, or to convert all of the ifs to a chain of if-elses, if
the intention is, in fact, for only one of them to ever apply.

However, I'm curious about the motivation for doing the flag masking in the
first place. I assume that it's done to prevent the creation of rules that
would never match because mount() doesn't permit certain combinations of
flags. But that doesn't seem desirable to me, because:

 * It results in the final rules enforced by the policy being silently
different from the rules specified by the user, sometimes in subtle ways;
 * Different kernel versions might accept different combinations of
parameters (the man page for mount hints at this already).

It seems to me that it would be better to catch invalid flag combinations
during parsing and either fail or emit a warning, instead. I'd be happy to
work on a patch to implement this if anyone agrees that it's the way to go.

- Ash
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150921/6ec4adfd/attachment.html>