containers / common

Location for shared common files in github.com/containers repos.
Apache License 2.0
169 stars 180 forks source link

AppArmor profile: recently added signal peers allow too much #2023

Open cboltz opened 1 month ago

cboltz commented 1 month ago

In https://github.com/containers/common/commit/1aedc12e356cfd29a5bb54d94e9b2e09da3649ca you added the following signal rules to the AppArmor profile:

  # Allow certain signals from OCI runtimes (podman, runc and crun)
  signal (receive) peer={/usr/bin/,/usr/sbin/,}runc,
  signal (receive) peer={/usr/bin/,/usr/sbin/,}crun*,
  signal (receive) set=(int, quit, kill, term) peer={/usr/bin/,/usr/sbin/,}podman,

This is not completely wrong, but it allows more than really needed.

a) The profiles added in https://gitlab.com/apparmor/apparmor/-/commit/2594d936 are all "named" profiles:

profile runc /usr/sbin/runc flags=(unconfined) {
profile crun /usr/bin/crun flags=(unconfined) {
profile podman /usr/bin/podman flags=(unconfined) {

This means you can reference them by their name (runc, crun and podman). Including the path in peer= is superfluous, peer=runc is enough.

b) Wildcard for crun*

I don't know why you allow crun* instead of just crun, but that means that profiles matching that name (for example "cruncher") will be allowed to send signals. If this isn't intentional, I'd recommend to remove the *.

.

To sum it up: I propose to change the lines added in https://github.com/containers/common/commit/1aedc12e356cfd29a5bb54d94e9b2e09da3649ca to

  # Allow certain signals from OCI runtimes (podman, runc and crun)
  signal (receive) peer=runc,
  signal (receive) peer=crun,
  signal (receive) set=(int, quit, kill, term) peer=podman,
rhatdan commented 1 month ago

You could change crun to be crun and crun- to make it more specific.

crun-wasm crun-qm

And potentially others in the future should be handled. These are sybolic links to crun, so not sure if that changes the equation.

cboltz commented 1 month ago

These are sybolic links to crun, so not sure if that changes the equation.

Yes - in this case, it makes things easier.

AppArmor checks paths after symlink resolution, so if you have a symlink /usr/bin/crun-wasm -> /usr/bin/crun, AppArmor will always see /usr/bin/crun, and use the crun profile.

rhatdan commented 1 month ago

Actually the symlink is only true for crun-wasm. crun-vm is a standalong executable.

So for precision it would be better to do crun and crun-*