[apparmor] Mount flag handling

John Johansen john.johansen at canonical.com
Mon Sep 21 19:50:42 UTC 2015


On 09/21/2015 08:56 AM, Ash Wilson wrote:
> 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.
> 
yep, thanks and that has even made it into the upstream dev tree as commit 3239

> 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.
> 
I need to look into this one more, I'll get back to you

> 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:
> 
Mount mediation is actually broken out internal into separate command paths much as the kernel does. Certain flags aren't allowed under certain commands, and the rules are built up to match that. Whether this is stricter following of what the kernel is doing than is necessary ...

Basically its being paranoid tight because its easier to loosen than it is to come back a tighten later.

>  * 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).
> 
that is possible and changes in what flags are allowed within a sub command (bind, remount, ...) have happened

> 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.
> 
I do think it would be better to fail or emit a warning

Doing the check in the parser is currently not sufficient because variable expansion happens post parse.




More information about the AppArmor mailing list