[apparmor] RFC: Policy versioning

[Christian Boltz]

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.

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

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.

[...]
>   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.

> 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.

[...]
> 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:

    /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.

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.

I'd even consider to 
- let the parser print a warning if an include_if_exists-included files 
  doesn't exist
- 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

>   * 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

b) people might have crazy ideas like

     if -e /etc/apache2/conf.d/whatever.conf {
       include <abstractions/apache-whatever>
     }

so we might end up with having to check for files that are not really 
related to AppArmor every time we load the policy.


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 ;-)

>   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

[...]
> 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.

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.

    * BTW - it seems deny blocks aren't supported by the parser.

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).

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.

> 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"?


[...]

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.


Regards,

Christian Boltz
-- 
> Genau, Office und M$-Programme haben meist alle den gleichen Stil.
Stimmt, die schaffen das Kammquoting meist besonders gut.    *g,d&r*
[> Andre Heine und Florian Gross in suse-linux]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20171210/d440fcb4/attachment.sig>