Re: Apparmor move_mount mediation breaks mount tool in containers

[Sasha Levin <sashal@kernel.org>]

On Mon, Dec 04, 2023 at 05:14:11AM -0800, John Johansen wrote:
>On 12/3/23 17:34, Stéphane Graber wrote:
>>On Sun, Dec 3, 2023 at 8:21 PM John Johansen
>><john.johansen@canonical.com> wrote:
>>>
>>>On 12/2/23 17:20, Stéphane Graber wrote:
>>>>Hey John,
>>>>
>>>>Upstream commit 157a3537d6bc28ceb9a11fc8cb67f2152d860146 which just
>>>>landed in 6.6.3 stable as 96af45154a0be30485ad07f70f852b1456cb13d7 is
>>>>blocking new mounts for all LXC, LXD and Incus users (at least) on
>>>>distributions using the newer version of util-linux.
>>>>
>>>>That's because for a simple mount like "mount -t tmpfs tmpfs /tmp",
>>>>the new mount command now performs:
>>>>```
>>>>fsconfig(3, FSCONFIG_SET_STRING, "source", "tmpfs", 0) = 0
>>>>fsconfig(3, FSCONFIG_CMD_CREATE, NULL, NULL, 0) = 0
>>>>fsmount(3, FSMOUNT_CLOEXEC, 0)          = 4
>>>>statx(4, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_MNT_ID,
>>>>{stx_mask=STATX_BASIC_STATS|STATX_MNT_ID,
>>>>stx_attributes=STATX_ATTR_MOUNT_ROOT, stx_mode=S_IFDIR|S_ISVTX|0777,
>>>>stx_size=40, ...}) = 0
>>>>move_mount(4, "", AT_FDCWD, "/tmp", MOVE_MOUNT_F_EMPTY_PATH) = 0
>>>>```
>>>>
>>>>That last call to "move_mount" is incorrectly interpreted by AppArmor
>>>>as an attempt to move-mount "/" to "/mnt" rather than as a new mount
>>>>being created, this therefore results in:
>>>>```
>>>>Dec 03 01:05:03 kernel-test kernel: audit: type=1400
>>>>audit(1701565503.599:34): apparmor="DENIED" operation="mount"
>>>>class="mount" info="failed perms check" error=-13
>>>>profile="incus-a_</var/lib/incus>" name="/tmp/" pid=2190 comm="mount"
>>>>srcname="/" flags="rw, move"
>>>>```
>>>>
>>>>Note that the flags here show "move", the fstype isn't even set and
>>>>the source path at srcname incorrectly shows "/".
>>>>
>>>>This operation therefore trips any container manager which has an
>>>>apparmor security policy preventing arbitrary move-mount (as those
>>>>could be used to bypass other apparmor path based policies).
>>>>
>>>>
>>>>The way I see it, the current mediation support effectively breaks any
>>>>attempt at mediating mounts in a useful way in apparmor as it's now
>>>>impossible to mediate new mounts based on their fstype or even
>>>>distinguish them from a move-mount operation.
>>>>
>>>Indeed it is a far from good solution. It is a stop gap.
>>>>
>>>>I don't know if this warrants pulling the mediation patch out of
>>>>stable (and out of linus' tree), obviously doing that would
>>>>reintroduce that hole in mount coverage, but at the same time, the
>>>>current coverage is broken enough that our only alternative is to
>>>>effectively allow all mounts, making the current mediation useless.
>>>
>>>pulling it effectively means ALL applications by-pass mediation, the
>>>alternative is to block all applications from using the move_mount
>>>system call as part of mediation. Which might have been acceptable
>>>as a stop gap when the syscall first landed but not now.
>>
>>Pulling it from the stable branch may still make sense, you now have
>>folks who are updating to get actual bugfixes and end up with broken
>>containers, that doesn't exactly seem like a good outcome...
>>
>
>I will defer such a decision to the maintainers the stable trees. I
>can see arguments either way.

This decision isn't up to the stable tree: we want to stay "bug
compatible" with upstream, and thus if this issue also exists upstream
(and it sounds like it does), we'll keep this patch in the stable trees.

If a revert was to be submitted to Linus, we'll take the same revert to
the stable trees.

-- 
Thanks,
Sasha