Bug 1221840

Smallest reproducer I have:

$ buildah run $(buildah from registry.opensuse.org/opensuse/leap:15.5) /bin/bash

Created attachment 873715 [details]
Working rules

Thanks to darix, the included rules work. I don't understand why /dev/net/tun rule is necessary.

important note: abstraction/pasta already has a rule for the tun device. but abstractions/passt does not. but the two abstractions look very similar.

It could be the cause for the failures in the podman upstream tests:

https://openqa.opensuse.org/tests/latest?arch=x86_64&distri=opensuse&flavor=DVD&machine=64bit&test=containers_host_podman_testsuite&version=Tumbleweed

More details in:
 podman_integration-bats-user-local.tap
 podman_integration-bats-user-remote.tap

Tracking upstream: https://github.com/containers/buildah/issues/5440

(In reply to Marcus Rückert from comment #3)
> important note: abstraction/pasta already has a rule for the tun device. but
> abstractions/passt does not. but the two abstractions look very similar.

That's intended: passt(1) doesn't need to access the tun device at all. See also passt commit 63a8302961a4 ("apparmor: Add pasta's own profile") and Debian commit 5bb812e79143 ("debian/rules: Override pasta symbolic links with hard links", https://salsa.debian.org/sbrivio/passt/-/commit/5bb812e79143670a57440cd8aa7f2979583c5a0a).

I guess OpenSUSE would also benefit from having a hard link, so that we can apply two different AppArmor profiles?

Same for the '/proc/@{pid}/ns/ r,' part: it's already included in pasta's abstraction (but not the one for passt).

What I guess we should add upstream (upstream here means passt, not Podman or Buildah) is something on the lines of '@{run}/user/@{uid}/**            r," to cover operation with Buildah or Podman's custom network, correct? Can somebody test this on OpenSUSE?

I tried overriding the symlinks with hardlinks on openSUSE TW[1] but I'm running into the same error. Updated usr.bin.pasta AppArmor profile with the rules shared here additionally, invoking `pasta` gives:

> mount /: Permission denied
> Failed to sandbox process, exiting

`passt` also fails with the same error but not before displaying (template) interface details. In the package changes, I've added a profile for pasta, a replica of usr.bin.passt jfyi. 

[1] - https://build.opensuse.org/projects/home:danishprakash:branches:Virtualization:containers/packages/passt/files/passt.spec?expand=1

(In reply to Danish Prakash from comment #7)
> I tried overriding the symlinks with hardlinks on openSUSE TW[1] but I'm
> running into the same error. Updated usr.bin.pasta AppArmor profile with the
> rules shared here additionally, invoking `pasta` gives:
> 
> > mount /: Permission denied
> > Failed to sandbox process, exiting

I just reproduced this on Debian -- by mistake, the package there correctly creates a hard link, but doesn't install a separate usr.bin.pasta profile, and if I just copy the one we ship upstream, I hit the same problem.

Give me a bit to fix that upstream and I'll get back to you.

> `passt` also fails with the same error but not before displaying (template)
> interface details. In the package changes, I've added a profile for pasta, a
> replica of usr.bin.passt jfyi. 

By the way, it shouldn't be a replica, that's the whole point of the hard link, so that different profiles get attached to /usr/bin/pasta and /usr/bin/passt.

(In reply to Danish Prakash from comment #7)
> I tried overriding the symlinks with hardlinks on openSUSE TW[1]
>
> [...]
>
> [1] -
> https://build.opensuse.org/projects/home:danishprakash:branches:
> Virtualization:containers/packages/passt/files/passt.spec?expand=1

This change looks good to me, by the way.

Created attachment 873969 [details]
Proposed upstream patch, tested on Debian only

With these changes:

- remounting / from an empty mountpoint should now be allowed by AppArmor across AppArmor commit d4b0fef10a4a ("parser: fix rule flag generation change_mount type rules")

- access to the filesystem-bound network namespace typically used by Podman's custom networks or Buildah (not 'podman run') is now enabled for pasta, as well

...Danish, it would be great if you could apply the changes from comment #10 and give it a try on openSUSE Tumbleweed. If that works, I'll send that upstream and you should get it back in the source archive in a couple of days. Thanks.

Comment on attachment 873969 [details]
Proposed upstream patch, tested on Debian only

>diff --git a/contrib/apparmor/abstractions/passt b/contrib/apparmor/abstractions/passt
>index 6bb25e0..61ec32c 100644
>--- a/contrib/apparmor/abstractions/passt
>+++ b/contrib/apparmor/abstractions/passt
>@@ -27,6 +27,7 @@
> 
>   /					r,	# isolate_prefork(), isolation.c
>   mount options=(rw, runbindable) /,
>+  mount		""	-> "/",
>   mount		""	-> "/tmp/",
>   pivot_root	"/tmp/" -> "/tmp/",
>   umount	"/",
>diff --git a/contrib/apparmor/abstractions/pasta b/contrib/apparmor/abstractions/pasta
>index a890391..e10d2a7 100644
>--- a/contrib/apparmor/abstractions/pasta
>+++ b/contrib/apparmor/abstractions/pasta
>@@ -27,7 +27,7 @@
>   @{PROC}/@{pid}/net/udp		r,
>   @{PROC}/@{pid}/net/udp6		r,
> 
>-  @{run}/user/@{uid}/netns/*		r,	# pasta_open_ns(), pasta.c
>+  @{run}/user/@{uid}/**			r,	# pasta_open_ns(), pasta.c
> 
>   @{PROC}/[0-9]*/ns/net			r,	# pasta_wait_for_ns(),
>   @{PROC}/[0-9]*/ns/user		r,	# conf_pasta_ns()

Created attachment 873970 [details]
Fixed patch for comment #10

Thanks for the patch, even though I can invoke `pasta` without any errors, I'm still getting the same permission denied error on the netns:

> Couldn't open network namespace /proc/9080/ns/net: Permission denied

Adding the following rule to usr.bin.passt doesn't help either:

>   /proc/@{pid}/ns/          r,

On the contrary, setting the two usr.bin.pas* profiles to complain mode, things are back to normal so perhaps the rules are still not right?

(In reply to Danish Prakash from comment #14)
> Thanks for the patch, even though I can invoke `pasta` without any errors,
> I'm still getting the same permission denied error on the netns:
> 
> > Couldn't open network namespace /proc/9080/ns/net: Permission denied
> 
> Adding the following rule to usr.bin.passt doesn't help either:
> 
> >   /proc/@{pid}/ns/          r,

That should be '/proc/@{pid}/ns/**', but anyway that's already covered by abstractions/pasta:

  @{PROC}/[0-9]*/ns/net                 r,      # pasta_wait_for_ns(),
  @{PROC}/[0-9]*/ns/user                r,      # conf_pasta_ns()

...you should make sure that those rules are taken into account, though.

> On the contrary, setting the two usr.bin.pas* profiles to complain mode,
> things are back to normal so perhaps the rules are still not right?

Definitely, it's an issue with AppArmor rules. Can you tail -f /var/log/audit/audit.log while you run 'pasta' and check what AppArmor is denying as you do that?

Created attachment 873985 [details]
apparmor profile fix for pasta

The following patch should do the job. For starters, the permission denied error was coming from `ptrace` invocation being denied by apparmor. Secondly, there are denials in accessing `/proc/<pid>/ns/` which, if added to abstractions/pasta, results in pasta(buildah bud in extension) running as expected with no issues.

(In reply to Danish Prakash from comment #16)
> Created attachment 873985 [details]
> apparmor profile fix for pasta

Thanks for the patch! A couple of comments:

> The following patch should do the job. For starters, the permission denied
> error was coming from `ptrace` invocation being denied by apparmor.

I'm still confused as to why AppArmor doesn't seem to deny this on Debian, but I might simply have tested a slightly older combination of kernel and AppArmor components than what you're using on openSUSE Tumbleweed.

Anyway, we need the CAP_SYS_PTRACE capability (within our detached user namespace) to open /proc/<pid>/ns/net (see also passt commit 594dce66d3bb), so I guess this is reasonable. The comment should refer to pasta_open_ns() for this -- that's the function which needs it.

> Secondly, there are denials in accessing `/proc/<pid>/ns/` which, if added
> to abstractions/pasta, results in pasta(buildah bud in extension) running as
> expected with no issues.

Same for this one, I'm not sure why I can't reproduce this with Debian. But anyway, this seems to fit with the usage we need in pasta_netns_quit_init() (the comment should also be updated accordingly).

From the patch:
> +++ b/contrib/apparmor/usr.bin.pasta
> +  ptrace,

Just wondering - does pasta really need to trace everything, and be traced by everything - or could you make the rule more specific?

If you are unsure, please show the audit.log events for ptrace.

Created attachment 874005 [details]
updated comments

Updated the patch with the right comments, do you plan to include this upstream?

(In reply to Danish Prakash from comment #19)
> Created attachment 874005 [details]
> updated comments
> 
> Updated the patch with the right comments, do you plan to include this
> upstream?

Thanks, it looks good to me, but I haven't looked yet into how to possibly restricting the 'ptrace' rule as suggested by Christian in comment #18.

I can probably manage to look into it later today if you don't. Once that's solved, yes, I would apply this upstream. Just to confirm: you're testing with this patch on top of mine from comment #12, correct?

Created attachment 874006 [details]
audit log passt ptrace

(In reply to Christian Boltz from comment #18)
> From the patch:
> > +++ b/contrib/apparmor/usr.bin.pasta
> > +  ptrace,
> 
> Just wondering - does pasta really need to trace everything, and be traced
> by everything - or could you make the rule more specific?
> 
> If you are unsure, please show the audit.log events for ptrace.

I've attached a snippet from `ausearch` which led me to add ptrace to the profile, I haven't explored this further I must admit.


(In reply to Stefano Brivio from comment #20)
> Thanks, it looks good to me, but I haven't looked yet into how to possibly
> restricting the 'ptrace' rule as suggested by Christian in comment #18.
> 
> I can probably manage to look into it later today if you don't. Once that's
> solved, yes, I would apply this upstream. Just to confirm: you're testing
> with this patch on top of mine from comment #12, correct?

Yes, it's on top of your patch from comment #13 and the changes I made to the package i.e. overriding symlinks with hardlinks.

(In reply to Danish Prakash from comment #21)
> Created attachment 874006 [details]
> audit log passt ptrace
> 
> (In reply to Christian Boltz from comment #18)
> > From the patch:
> > > +++ b/contrib/apparmor/usr.bin.pasta
> > > +  ptrace,
> > 
> > Just wondering - does pasta really need to trace everything, and be traced
> > by everything - or could you make the rule more specific?
> > 
> > If you are unsure, please show the audit.log events for ptrace.
> 
> I've attached a snippet from `ausearch` which led me to add ptrace to the
> profile, I haven't explored this further I must admit.

Thanks, I just set up a current Tumbleweed virtual machine, let me try a couple of variations on your 'ptrace' rule later on Wednesday, I'll let you know if I can figure out something stricter than that, otherwise I would proceed with your patch as it is.

By the way, pasta(1) doesn't ptrace() anything and isn't ptrace()d by anybody, it just needs to open namespace entries in procfs.

(In reply to Stefano Brivio from comment #22)
> By the way, pasta(1) doesn't ptrace() anything and isn't ptrace()d by
> anybody, it just needs to open namespace entries in procfs.

Namespaces can be interesting[tm], and IIRC yo don't need to do explicit ptrace() calls to trigger ptrace events. (I'll need to ask someone who does the kernel-side work if you are interested in the details.)


That said - the only ptrace event in your audit.log is:

type=AVC msg=audit(04/02/2024 12:49:39.412:101237) : apparmor=DENIED operation=ptrace profile=passt pid=8042 comm=passt.avx2 requested_mask=read denied_mask=read peer="unconfined"

which translates to

    ptrace read peer=unconfined,

If passt also needs to open namespace entries of confined processes, remove the "peer=unconfined" part.

(In reply to Christian Boltz from comment #23)
> (In reply to Stefano Brivio from comment #22)
> > By the way, pasta(1) doesn't ptrace() anything and isn't ptrace()d by
> > anybody, it just needs to open namespace entries in procfs.
> 
> Namespaces can be interesting[tm], and IIRC yo don't need to do explicit
> ptrace() calls to trigger ptrace events. (I'll need to ask someone who does
> the kernel-side work if you are interested in the details.)

Ah, don't worry, it actually makes sense, and it turns out I didn't hit this on Debian simply because I didn't check that AppArmor profile together with Buildah.

> That said - the only ptrace event in your audit.log is:
> 
> type=AVC msg=audit(04/02/2024 12:49:39.412:101237) : apparmor=DENIED
> operation=ptrace profile=passt pid=8042 comm=passt.avx2 requested_mask=read
> denied_mask=read peer="unconfined"
> 
> which translates to
> 
>     ptrace read peer=unconfined,
> 
> If passt also needs to open namespace entries of confined processes, remove
> the "peer=unconfined" part.

Thanks, added read ('r' for consistency with the rest of the file), but I didn't specify anything about the peer, because we don't actually know.

Posting patchset upstream in a bit.

Series at:

    https://archives.passt.top/passt-dev/20240403190425.2848764-1-sbrivio@redhat.com/

pasta's upstream version 2024_04_05.954589b (https://archives.passt.top/passt-user/20240405233127.4dc213d3@elisabeth/) ships the amended AppArmor policies.

With the proposed changes an unprivileged podman still fails to create the network namespace if apparmor is enabled because of the passt profile.

> user@localhost:~> podman run --rm quay.io/podman/hello
> Error: pasta failed with exit code 1:
> Couldn't open network namespace /run/user/1000/netns/netns-c8477eb7-d107-28fd-c660-8c4c46fb3d19: Permission denied
> ---
> Profile: passt
> Operation: open
> Name: /run/user/1000/netns/netns-a30f2240-2b6f-d3b7-189d-02cc4510eef4
> Denied: r
> Logfile: /var/log/audit/audit.log
> ---
> type=AVC msg=audit(1712566709.070:152): apparmor="DENIED" operation="open" class="file" profile="passt" name="/run/user/1000/netns/netns-c8477eb7-d107-28fd-c660-8c4c46fb3d19" pid=1425 comm="passt.avx2" requested_mask="r" denied_mask="r" fsuid=1000 ouid=0

Adding "include <abstractions/pasta>" to "/etc/apparmor.d/abstractions/passt" fixes this.
Would that be an appropriate solution or is there a more strict option available?

Jay,

(In reply to Jay - from comment #27)
> With the proposed changes an unprivileged podman still fails to create the
> network namespace if apparmor is enabled because of the passt profile.

Did you also replace soft links with hard links (see comment #6)? If you don't, the AppArmor profile for pasta won't be loaded. On openSUSE Tumbleweed for x86_64, as root (or with sudo), you can do:

  rm /usr/bin/pasta{,.avx2}
  ln /usr/bin/passt /usr/bin/pasta
  ln /usr/bin/passt.avx2 /usr/bin/pasta.avx2

and try again -- you don't need to reload AppArmor profiles after this.

(In reply to Stefano Brivio from comment #28)
> Did you also replace soft links with hard links (see comment #6)? 
Stefano,

No I didn't. After that it works like a charm.
Thank you!

(In reply to Stefano Brivio from comment #28)
> Did you also replace soft links with hard links (see comment #6)? If you
> don't, the AppArmor profile for pasta won't be loaded. On openSUSE
> Tumbleweed for x86_64, as root (or with sudo), you can do:

Shouldn't we change the .spec file? Because installing the new version from develop [1] still gives the same issue.

[1]: https://build.opensuse.org/request/show/1166120/changes

(In reply to Eyad Issa from comment #30)
> (In reply to Stefano Brivio from comment #28)
> > Did you also replace soft links with hard links (see comment #6)? If you
> > don't, the AppArmor profile for pasta won't be loaded. On openSUSE
> > Tumbleweed for x86_64, as root (or with sudo), you can do:
> 
> Shouldn't we change the .spec file? Because installing the new version from
> develop [1] still gives the same issue.

Uh, yes, I thought that was the plan, see comment #21. needinfo'ing here and there, maybe Dan or Dario missed this?

(In reply to Stefano Brivio from comment #31)
> (In reply to Eyad Issa from comment #30)
> > (In reply to Stefano Brivio from comment #28)
> > > Did you also replace soft links with hard links (see comment #6)? If you
> > > don't, the AppArmor profile for pasta won't be loaded. On openSUSE
> > > Tumbleweed for x86_64, as root (or with sudo), you can do:
> > 
> > Shouldn't we change the .spec file? Because installing the new version from
> > develop [1] still gives the same issue.
> 
> Uh, yes, I thought that was the plan, see comment #21. needinfo'ing here and
> there, maybe Dan or Dario missed this?

Yeah, we missed it. I'm sending an SR as we speak, thanks for the heads up!

(In reply to Danish Prakash from comment #32)
> (In reply to Stefano Brivio from comment #31)
> > (In reply to Eyad Issa from comment #30)
> > > (In reply to Stefano Brivio from comment #28)
> > > > Did you also replace soft links with hard links (see comment #6)? If you
> > > > don't, the AppArmor profile for pasta won't be loaded. On openSUSE
> > > > Tumbleweed for x86_64, as root (or with sudo), you can do:
> > > 
> > > Shouldn't we change the .spec file? Because installing the new version from
> > > develop [1] still gives the same issue.
> > 
> > Uh, yes, I thought that was the plan, see comment #21. needinfo'ing here and
> > there, maybe Dan or Dario missed this?
> 
> Yeah, we missed it. I'm sending an SR as we speak, thanks for the heads up!

Thanks. I just had a look at https://build.opensuse.org/request/show/1166199 -- I think you also need something like:

  install -m 0644 usr.bin.pasta %{buildroot}%{_sysconfdir}/apparmor.d/

around line 108?

(In reply to Stefano Brivio from comment #33)
> (In reply to Danish Prakash from comment #32)
> > (In reply to Stefano Brivio from comment #31)
> > > (In reply to Eyad Issa from comment #30)
> > > > (In reply to Stefano Brivio from comment #28)
> > > > > Did you also replace soft links with hard links (see comment #6)? If you
> > > > > don't, the AppArmor profile for pasta won't be loaded. On openSUSE
> > > > > Tumbleweed for x86_64, as root (or with sudo), you can do:
> > > > 
> > > > Shouldn't we change the .spec file? Because installing the new version from
> > > > develop [1] still gives the same issue.
> > > 
> > > Uh, yes, I thought that was the plan, see comment #21. needinfo'ing here and
> > > there, maybe Dan or Dario missed this?
> > 
> > Yeah, we missed it. I'm sending an SR as we speak, thanks for the heads up!
> 
> Thanks. I just had a look at https://build.opensuse.org/request/show/1166199
> -- I think you also need something like:
> 
>   install -m 0644 usr.bin.pasta %{buildroot}%{_sysconfdir}/apparmor.d/
> 
> around line 108?

You're right, I lost my old project and somehow missed this change. I tested the local rpm build and buildah worked fine without the pasta profile though, are the abstractions enough for pasta? Either way, the SR with the missing profile[1]. Thanks for catching this!

[1] - https://build.opensuse.org/request/show/1166212

(In reply to Danish Prakash from comment #34)
> I tested the local rpm build and buildah worked fine without the pasta profile

Well, yes, because without a profile, pasta(1) will simply run unconfined. :)

> Either way, the SR with the missing profile[1].
> 
> [1] - https://build.opensuse.org/request/show/1166212

Thanks, for what it's worth, it looks good to me now!

Thanks for all the help! I'll wait for this update to be sent to Factory and then close this.

Closing as resolved.