lxc ships apparmor config that confuses aa-logprof

[Thomas-Thomas]

== Summary ==
liblxc-common_5.0.3-2ubuntu7_amd64.deb apparently ships with some files that are broken or incomplete or use some future config semantics (I do not know which)
aa-logprof chokes on those files on loading, and therefore is not usable at all.

Possible solutions:
1. aa-logprof should ignore those files, instead of exiting with an error.
2. aa-logprof should support the new config sematics (if that is actually valid)
3. liblxc should not distribute unsupported files.

== Long description:==

# aa-logprof

ERROR: Can't parse mount rule mount options=(rw, make-slave) -> **,

Apparently there are files:

# grep -R make-slave /etc/apparmor.d/*
/etc/apparmor.d/abstractions/lxc/start-container: mount options=(rw, make-slave) -> **,
/etc/apparmor.d/abstractions/lxc/container-base:# mount options=(rw,make-slave) -> **,

Note that the 2nd file "container-base" has that commented out with some comment
" # allow paths to be made slave, shared, private or unbindable
  # FIXME: This currently doesn't work due to the apparmor parser treating those as allowing all mounts.
"

The 1st file "start-container" uses this command, but prefixed with:
  # currently blocked by apparmor bug

step-by step commenting this resulted in other commands that fail.

also, at one point, they are including duplicate files:
# aa-logprof
AppArmor-Profile in /etc/apparmor.d werden aktualisiert.

ERROR: Conflicting profiles for /usr/bin/lxc-start defined in two files:
- /etc/apparmor.d/usr.bin.lxc-copy
- /etc/apparmor.d/usr.bin.lxc-start

Note that these files are identical. specifically, the "usr.bin.lxc-copy" contains "/usr/bin/lxc-start"
At this point I stopped and purged lxc, and waydroid (which was using it) for now

[John Johansen (jjohansen)]

The answer is 2 (support) then 1 (ignore and warn/treat like a comment if it doesn't understand).

What is the version of apparmor, or distro release you are seeing this on? aa-logprof has had work recently to support mount rules (eg. https://gitlab.com/apparmor/apparmor/-/merge_requests/1195, and a few others) and before this should have been accepting and ignoring them).

[Thomas-Thomas (thomas-thomas)]

This was Ubuntu 24.04 noble, with:

$ apt-cache policy apparmor
apparmor:
  Installed: 4.0.0-beta3-0ubuntu3
  Candidate: 4.0.0-beta3-0ubuntu3
  Version table:
 *** 4.0.0-beta3-0ubuntu3 500
        500 http://de.archive.ubuntu.com/ubuntu noble/main amd64 Packages
        100 /var/lib/dpkg/status

[Maxime Bélair (mbelair)]

This bug is due to lxc using invalid mount rules according to the specification.

`man 5 apparmor.d` ==> `[mountpoint] must start with ’/’ (after variable expansion)`.

We can find such problematic rules in `src/lxc/lsm/apparmor.c` and config/apparmor/abstractions/{start-container.in,container-base,container-base.in}

    $ cd lxc
    $ grep -r '\-> \*\*,'
    config/apparmor/abstractions/start-container.in: mount options=(rw, make-slave) -> **,
    config/apparmor/abstractions/start-container.in: mount options=(rw, make-rslave) -> **,
    [...]
    src/lxc/lsm/apparmor.c:" mount options=(rw,make-unbindable) -> **,\n"
    src/lxc/lsm/apparmor.c:" mount options=(rw,make-runbindable) -> **,\n"
    [...]
    $ grep -r '\-> \*\*,'|wc -l
    36

Although this restriction is only enforced in AppArmor since version 4.0, similar restrictions have always been in place for files: `/** ix,` is a valid rule but `** ix` is not. Therefore, IMHO, the best way to handle this issue would be to create a merge request for lxc to update their rules (hence not making any change to AppArmor, since the rule is indeed invalid).

@jjohansen Would you agree with this approach ?

Side note: In man apparmor.d, we also find
    `mount,
    the 'mount' rule without any conditionals is the most generic and allows any mount. Equivalent to 'mount fstype=** options=** ** -> /**'.`

IMO we should remove the sentence `Equivalent to 'mount fstype=** options=** ** -> /**'` since this rule is very invalid, even for apparmor_parser and could confuse AppArmor users.

[John Johansen (jjohansen)]

@mbelair:

  the rule is accepted by the parser, and has been since mount rules have been supported. So this is a case of it may not be to spec, but it is not new and existed as such for a long time. So just like

  mount (rbind) /foo,

we need to support this. That is to say we make every effort not to break existing userspace unless necessary. IF its bug that we catch when we first ship the feature, yes fix and follow spec, a bug like this however. We don't break existing userspace.

As a side note, logprof/genprof exiting because they can't parse a rule is bad, and a behavior that needs to be fixed as well.

Actually the sentence isn't invalid, the original mount rules did not restrict source to be a path, and there are ways to use mount where the source isn't a path. Any addition to the man page claim source must be a path is wrong. Admittedly it is almost always a path, but there are in fact cases where it isn't. But I agree it should be removed because as options are introduced they will get left off and it will be confusing.

[Maxime Bélair (mbelair)]

Ok, I understand.

So I guess, we can leave it as-is then. The bug is still present in 4.0.0~beta3, currently shipped in noble, but is fixed by e96fdc0f5b5aea78c7860e37e0ca149e9dac3480 / 1c7127d30d3388119a4f8293ae3ac093016356ea which is included in 4.0.0~beta4+

[John Johansen (jjohansen)]

I opened a Ubuntu Noble specific task. We can close it after verifying the current apparmor in noble fixes the issue.

[Aleksandr Mikhalitsyn (mihalicyn)]

From LXC side, we probably should fix this too, just to follow the AppArmor spec. I'll prepare a PR for that.

John, what's the best way to validate AppArmor profiles syntax and conformance with the spec?

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Aleksandr Mikhalitsyn (mihalicyn)]

JFYI: https://github.com/lxc/lxc/pull/4452

[Aleksandr Mikhalitsyn (mihalicyn)]

If I understand correctly, a proper replacement for

 mount options=(rw,make-unbindable) -> **,

is

 mount options=(rw,make-unbindable) -> /{,**},

It turned out that replacing it with:

 mount options=(rw,make-unbindable) -> /**,

does not work properly and restricts anything on /

(see also https://github.com/lxc/lxc/pull/4456 )