Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cboltz opened this issue May 28, 2024 · 5 comments
Open

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

cboltz opened this issue May 28, 2024 · 5 comments

Comments

@cboltz
Copy link

cboltz commented May 28, 2024

In 1aedc12 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 1aedc12 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
Copy link
Member

rhatdan commented May 29, 2024

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
Copy link
Author

cboltz commented May 29, 2024

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
Copy link
Member

rhatdan commented May 29, 2024

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

@panlinux
Copy link

Where are these other crun-* binaries being shipped or used, like crun-vm? I don't see them in any of the packages in the ubuntu archive, for example:

$ apt-file search /usr/bin/crun
crun: /usr/bin/crun                       
crunch: /usr/bin/crunch
libcam-pdf-perl: /usr/bin/crunchjpgs

But as can be seen, the concern raised by this ticket is valid: there are binaries matching /usr/bin/crun* that have nothing to do with the container world, and that this apparmor rule would allow.

@rhatdan
Copy link
Member

rhatdan commented Sep 24, 2024

Change the rule to crun and crun-* if you like. We do not track what is and is not packaged for certain distributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants