procfs: workarounds for AppArmor design flaw

[cyphar]

Okay, I've figured it out. This is really dumb. tl;dr: This is really an AppArmor bug (or even a design flaw if you prefer).

For context, the file we are trying to write to is /proc/sys/net/ipv4/ip_unprivileged_port_start. @stgraber figured out that the problematic AppArmor rules are the rules they have which block writing to most /sys files. How is it possible that one affects the other?

Well, the problem is that runc now uses a detached mount of procfs to operate on (this avoids mount race attacks). Because detached mounts have not been attached to the filesystem, d_name (the kernel's facility for generating names for dentries) just generates a name that looks like /foo if you try to open a file foo inside the detached procfs mount. AFAICS this is what AppArmor uses to determine what file you are trying to write to (because AppArmor is path-based, and d_name is the only way to get pathnames from dentries).

This means that when we try to write to /proc/sys/net/ipv4/ip_unprivileged_port_start, AppArmor sees this as us trying to write to /sys/net/ipv4/ip_unprivileged_port_start which is forbidden by the /sys denial rules. I have attached a program that can show this behaviour using a detached tmpfs mount, it's very trivial to trigger:

% ./aa-bug &
c1:~ # ./aa-bug &
fd: /proc/2061/fd/5
[1] 2061
c1:~ # mkdir /proc/2061/fd/5/sys
c1:~ # mkdir /proc/2061/fd/5/sys/foo
mkdir: cannot create directory ‘/proc/2061/fd/5/sys/foo’: Permission denied

aa-bug.go.txt

There is a trivial workaround for this particular sysctl:

-  deny /sys/[^fdck]*{,/**} wklx,
+  deny /sys/[^fdckn]*{,/**} wklx,

(In /etc/apparmor.d/abstractions/lxc/container-base.)

But this doesn't help in the general case for all sysctls. @stgraber has just submitted lxc/incus#2624 which just removes these rules entirely. I think AppArmor should not do this, because it's incredibly broken (literally any detached mount could match against a rule by accident), but this is unfortunately how AppArmor's design works.

From runc's side, we could in theory use this to our advantage -- if we created a tmpfs with a subpath like .go-away-apparmor and then attached our procfs mount to that path, we might be able to subvert AppArmor. However, this has a risk of causing lifetime issues that would require a rework of how we do lookups -- the tmpfs must not be closed after we attach to it because it will lazy-unmount the procfs...

Originally posted by @cyphar in #12484

[cyphar]

To further clarify my point at the end, I suspect that if we created our fsopen("proc") handle and then move_mounted it to a separate fsopen("tmpfs") handle (to a subdirectory) then you would be able to avoid this because AppArmor would then see /.go-away-apparmor/sys/net/... as the write target.

Personally I think AppArmor should just permit all detached mount operations if deny mount is not in effect because it allows you to bypass their rules completely like this, and only innocent usage of fsopen is at risk of breaking due to unrelated AppArmor rules.

[cyphar]

Unfortunately, for the C API, it's not clear what we should do -- the API is fd-based and we operate with the fd of the procfs root, but if we attach the procfs root to some other detached mount then when that file descriptor gets closed the kernel will trigger a recursive MNT_DETACH which could cause issues with the procfs root.

If the kernel used reference counting (based on open files / bind-mounts associated with the mount) instead of tying the lifecycle of the mount to the fsmount file descriptor then this would be much easier, but I don't know how viable that is.

[cyphar]

Based on some quick prototyping, the attached-to-a-tmpfs approach doesn't work -- d_name still shows as /sys/net/... even though it is attached to a root. Detached mounts completely screw up d_path and actually there is a special-case for it in __prepend_path:

static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
			  const struct path *root, struct prepend_buffer *p)
{
	while (dentry != root->dentry || &mnt->mnt != root->mnt) {
		const struct dentry *parent = READ_ONCE(dentry->d_parent);

		if (dentry == mnt->mnt.mnt_root) {
			struct mount *m = READ_ONCE(mnt->mnt_parent);
			struct mnt_namespace *mnt_ns;

			if (likely(mnt != m)) {
				dentry = READ_ONCE(mnt->mnt_mountpoint);
				mnt = m;
				continue;
			}
			/* Global root */
			mnt_ns = READ_ONCE(mnt->mnt_ns);
			/* open-coded is_mounted() to use local mnt_ns */
			if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
				return 1;	// absolute root
			else
				return 2;	// detached or not attached yet
		}

		if (unlikely(dentry == parent))
			/* Escaped? */
			return 3;

		prefetch(parent);
		if (!prepend_name(p, &dentry->d_name))
			break;
		dentry = parent;
	}
	return 0;
}

[cyphar]

^^ A little confused why the if (likely(mnt != m)) case is not being hit in the mount-moved-to-other detached mount...