[apparmor] RFC: Policy versioning

[John Johansen]

On 12/10/2017 06:47 AM, Christian Boltz wrote:
> Hello,
> 
> wow, that was the longest human-written mail I read for a long time ;-)
> 
> I sometimes scrolled up and down during answering, so you might want to 
> read all of my comments and questions before starting to write a reply.
> 
> Am Sonntag, 10. Dezember 2017, 12:05:53 CET schrieb John Johansen:
> 
>> I. Proposed Solution
>>
>> The basic proposal to address the issues is fairly simple, some of the
>> details are harder. However the work can be split into several phases
>> so we can move forward immediately.
>>
>>
>> 1. We extend policy so that the feature file can be included directly
>> into profiles.
>>
>>   something like
>>
>>     features=/etc/apparmor/featuresX
>>
>>   OR
>>
>>     #pragma features=/etc/apparmor/featuresX
>>
>>   and the feature set will be made available to policy conditionals
>>   (see 6. Handling abstractions below). Hiding the features as a
>>   pragma comment would allow this policy change to be transparent to
>>   older parsers and tools, but I am unsure if it is worth doing.
>>   The feature definition is not the only thing in this proposal that
>>   would break on older parsers, so I am leaning towards features=
> 
> Agreed, and if we really hide everything in include files, the solution 
> for old parsers is simple - give them an empty <version/11> file instead 
> of one that contains   features=...
> 
>>   The specified features will be used instead of the kernel's exported
>> feature set abi. If a policy feature abi version is not supported by
>> the kernel it may fail to load (at least for the first pass at this).
>>  The compiler will of course still be free to take in the live kernel
>> information and use it in conjunction with the policy abi, to
>> generate rule downgrades or broader support. It just won't use new
>> features that the kernel supports and the policy doesn't.
>>
>>   Directly referencing the features file however is somewhat ugly and
>>   doesn't deal with abstractions, tunables, or making the abi
>>   information human friendly. Instead I propose we wrap the feature
>>   file with an include, giving the include a meaningful name.
>>
>>     include <version/4.14>
>>
>>     include <version/ubuntu-17.10>
>>
>>   or maybe
>>
>>     include <abi/4.14>
>>
>>   and of course instead of using the name for specific kernel or
>>   release we can use a simple revision number if we so choose.
>>
>>     include <version/11>
> 
> What's the plan for distro-patched kernels? 
> 
> For example, the openSUSE kernel supports network rules since years, but 
> (before 4.13/4.14) didn't support ptrace, mount, signal and pivot_root. 
> OTOH, these rules were supported in the Ubuntu kernel since years.
> 
> This means that relying on the kernel version is obviously a bad idea, 
> and even <version/11> might be problematic.
> 
I purposely left this some what vague. There will custom development
abis, there will potentially be distro abis, and there will be
upstream abis. The goal is that distros will only have to do a custom
abi if they are supporting a feature that isn't supported upstream.

> Please don't even propose <version/11-openSUSE> - that would mean to 
> patch *all* profiles to avoid loosing network confinement.
> 

only if suse is carrying custom features not supported by upstream
(and I am not referring to the upstream kernel either).

> Unfortunately patching <version/11> also isn't perfect - if someone uses 
> a profile from a "foreign source" that is made for 4.14, but not 
> prepared for network rules yet, it will cause denials. At least it will 
> error out on the secure side, but it will still error out.
> 

Ideally it will fail to compile because they adjusted the abi to work
with the foreign source feature, and the local policy doesn't have
that abi.

If they change abi the abi and that abi is also installed then, if the
kernel supports the feature it will work, if the kernel doesn't support
feature it will be ignored by the kernel (not enforced) or the compiler
will have to downgrade the rule, and the feature won't be enforced.

Now lets suppose they modified the abi for <version/11> instead of creating
a new one (which is wrong but will happen). There are two potential things
that will happen when that policy is pulled in some place else.

1. the parser knows about the rule type and either downgrades or completely
   drops the rule, because the abi does not support it.

   This is no different than what suse saw with all the ubuntu features
   working their way into policy, that were not supported on suse.

   In this case you do not get denials for these rules even if you boot
   a kernel that support them because the policy abi is being used, and
   it says it does not support the rule type.

   Alternatively to dropping/downgrading the rule we will be adding a
   flag to fail compiles if rules that aren't supported by the abi
   exist. I don't think we want to use that by default as it would
   discourage sharing policy snippets, and make it harder for policy
   devs to have a single version that will work everywhere.

2. the parser doesn't support the rule
  2.1 the parser does not have unknown rule support
     the compile fails
  2.2 the unknown rule support exists in the parser
    2.2.1 the parser doesn't have a template for the rule
      the compile fails
    2.2.2 the parser has a template for the unknown rule
      the compile skips the rule, policy is compiled loaded but the
      rule is not enforced

In none of these situations does the foreign rule result in denials.
There is one case I covered below in 5.

> [...]
>>   It will be easy to inspect what policy versions are supported on the
>> system by listing the version/abi directory
>>
>>     ls /etc/apparmor.d/version/
>>     7
>>     8
>>     9
> 
> Does it make sense to also have a "latest" that will behave like our 
> current behaviour? It wouldn't be recommended for average users, but 
> would be useful for profile development and adventurous users.
> 
Sort of. I am very gun shy of allowing policy to have a way to specify
the current behavior, because it will leak out into the wild.

Instead I was thinking there would be a way to specify it manually
so that the tools could do it. At the same time they could update the
abi include, to avoid the problem brought up above.

>> 3. Policy hashing for better cache conistency
>>
>>   We need to adopt policy hashing to provide better cache consistency.
>> This is not only so we can fix problems with using file time stamps
>> but also as away to detect inconsistencies with the compiled feature
>> set.
> 
> This might answer what I ask in the include_if_exists section below, but 
> I'll keep that question nevertheless ;-)
> 
>> 4. Limit distros ability to compile policy to the current kernels
>>    feature abi
>>
>>   Along with this Distros will no longer be able to set a default
>>   policy compile that will use the current kernel's abi. This will not
>> even be supported at the distro level as the project can not afford
>> to break the feature abi of current policy for kernel developers.
>>
>>   To address this a new tool will be added to extract the kernel
>>   features abi, and tooling will be updated to allow users update a
>>   profiles abi and thus begin development on newer versions. Basically
>> a per user opt in only approach.
> 
> Sounds like you already answered my question if version/latest is a good 
> idea.
> 
yeah I know its a pita for distros but I can not see another way

> [...]
>> 6.2 Conditional include
>>
>>   The current include fails if the file or dir it references doesn't
>>   exist. We need to extend the include mechanism that it can
>>   conditionally not fail if the referenced entry does not exist.
> 
> While I understand the need for that, let me also warn that it's a good 
> way to accidently not include a file:
> 
yes, its not something that can be used lightly. But its also needed to
make policy flexible so we can do per package policy. If we want to only
do a monolithic policy it is not needed.

>     /usr/bin/untrusted_binary {
>         include_if_exists <abstractions/private-file-strict>
>         network,
>         @{HOME}/** r,
>     }
> 
> If you now think the program running under this profile can't steal your 
> GPG and SSH keys, check again.
> 
yes, and I would argue it shouldn't be used like this. It works for
things like
  include_if_exist <abstractions/@{abi}/base>

and other drop in snippets that packaging may need to extend policy.

> This also means we should make such conditional rules as visible as 
> possible.Therefore I don't like the syntax options that just add a "-" 
> and vote for...
> 
>>   * a new keyword
>>
>>     include_if_exists <abstractions/@{abi}>
> 
> ... this one.
> 
okay, feel free to suggest a different keyword, or we could just make
it
  include if exists

it doesn't have to be a single word, the parser is capable of supporting
a phrase, so underlines are not needed.

> I'd even consider to 
> - let the parser print a warning if an include_if_exists-included files 
>   doesn't exist

yes, this needs to be an option

> - have a config option to whilelist (maybe regex-based?) paths that 
>   should not print such a warning, for example  abstractions/[0-9]+/base
> - and finally have a parser option to ignore the whitelist so that 
>   people could be double-sure about non-existing include files
> 

interesting proposal, I am not opposed to it. But I do think it falls
into a later stage roll out because it is not absolutely required
and we just aren't going to be able to get everything done for the
next release in a couple of months.

>>   * wrapping it in a conditional
>>
>>     this requires extending conditions to support an existence test
>>
>>     if -e <abstractions/@{abi}> {
>>       include <abstractions/@(abi}>
>>     }
> 
> That might also be an option, but it comes with two more problems:
> 
> a) supporting it in the parser and the tools is more painful
> 
yes

> b) people might have crazy ideas like
> 
>      if -e /etc/apache2/conf.d/whatever.conf {
>        include <abstractions/apache-whatever>
>      }
> 
yes

> so we might end up with having to check for files that are not really 
> related to AppArmor every time we load the policy.
> 
yes, but at the same time it brings in some interesting flexibility
regardless of whether we use it here, it may be something we want to
revisit at some point in the future.

> 
> Let me also point out that include_if_exists (independent on the syntax) 
> will make cache validation funny[tm] - files can easily appear and 
> disappear ;-)
> 

The cache validation is going to be done through hashing the policy,
conditional includes are one of the reasons we need to do this. The
hash won't include the conditional include file if it fails so it
will be fine.

I should have noted this better as part of the reason for the policy
hashing.

>>   I would like to note that the - is going to be used to indicate set
>>   subtraction in future expressions, to aid in righting righter
>>   expressions.
>>
>>   eg.
>>
>>     allow rw /** - {/foo,/bar*},
> 
> I'm looking forward to those exceptions, even if I already know that 
> I'll hate them in the tools ;-)
> 
:)

>> 7. Dealing with new policy features on older releases.
> [...]
>> 7.1 Wrapping rules in conditionals
> [...]
>>     if @{features/network/af_unix} {
>>        unix peer=foo,
>>     }
> [...]
>> 7.2 Supporting unknown rule templates
> [...]
>>   eg.
>>     if !supports(key) {
>>       template key='key\w.*,'		# yes its overly simple
>>     }
> 
> I'd prefer 7.2
> 

yes but that is going to take a while to properly support. Though the
earlier we can land it the better, as it is only really useful to
older userspaces.

> [...]
>> III. Impact of the compiler and tools
> [...]
>> - current tools will need to be updated to support the language
>>   extensions
> 
> This task is probably more interesting[tm] than you might think.
> by interesting you mean painful right? ;-)

> At the moment, the tools expect that   }   is the end of a profile or 
> child profile, and that's "good enough" because nobody uses 
> conditionals, audit or owner blocks * in practise.
> 
right, but that was bound to change at some point

>     * BTW - it seems deny blocks aren't supported by the parser.
> 
correct, there are some issues there atm. Its not just the tools this
will be interesting for.

> To support conditionals etc., the tools will need to switch from knowing 
> in which profile and child profile/hat they are currently reading  to a 
> full stack (profile, child profile/hat, and any nesting level of 
> conditionals, audit and owner blocks).
> 
yes

> The only good thing would be that we can add support for nested child 
> profiles for, well, I hesitate to write "for free" ;-) because these 
> changes will need quite some work.
> 

right, which is another reason to try splitting the roll out into
different phases. Its not so bad to support feature= and conditional
include as a first pass. That will get us started as a minimum
requirement, but we won't be able to stick with the minimum for long
as we start getting policy with different feature sets.

>> IV. Impact on packaging
> [...]
>> - It will require packaging to be able to cleanup old policy caches
>>   that are no longer needed
> 
> Given that the cache could be valid for multiple similar-enough kernels, 
> how can you decide about "no longer needed"?
> 
Indeed interesting problem. I assume we could drop a link to each
kernel that is using it. When a kernel is repeated its link could be
removed, or at least detected that it is gone, at which point its
link could be repeaped. I can think of a couple of ways to do it
but I was hoping to hear more from the people who deal with packaging
more than I do

> 
> [...]
> 
> These are my questions from a quick read (what a word for a 35k mail), 
> so I might come up with a few more questions after thinking about these 
> changes for a few days.
> 

heh thats expected.