Re: Apparmor move_mount mediation breaks mount tool in containers

[John Johansen <john.johansen@canonical.com>]

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.

But I will note that even if we had been able perfectly match the
new interface mediation to the old breakage is still possible because
containers could have been using new interface to do things not
covered by the old interface rules.

Not that it matters to the upstream kernel maintainers but a CVE
is coming for this and that having this unmediated in the tree isn't
good either.

>> The situation can be broken down like this
>> unconfined applications have no restrictions, this doesn't affect
>> hem.
>>
>> confined applications using the old mount interface continue to
>> work as expected.
>>
>> confined applications using the new mount interface now are blocked
>> unless there is, admittedly, a very broad mount rule granted. This
>> is however better than the old situation in which confined
>> applications where not mediated at all.
> 
> It depends from your point of view. If you are using apparmor around a
> single application and have tailored rules for exactly what it is
> doing, then yes, I agree.
> But for all cases where apparmor is used as a safety net to block
> potentially dangerous/problematic actions, this is causing the kind of
> breakage which will cause people to just plain turn off apparmor.
> 

Only in the sense that closing a security regression causes breakage.

Lets cover the cases.
Policy is not updated, and application uses the old mount interface:
this continues to work.

*Policy is not updated, and application uses the new mount interface:
the access is likely blocked (the security hole has been closed)
unless the policy already allowed a generic
   allow mount,

+Policy is updated with generic
   allow mount,
and application uses the old mount interface:
this continues to work, security restrictions are reduced on the
old interface, but policy reflects what can actually be enforced
around mount, because if the application is compromised and uses
the new mount interface this is what can be mediated.

Policy is updated with generic
   allow mount,
and application uses the new mount interface:
The application continues to work.

* The breakage case is where the application was using the new
interface and not being mediated. While being able to mediate
the new interface to the same level of the old would help, it
does not guarantee this case won't break, as the application
could be using to do things that are not restricted by current
policy.

+ The regressed mediation case, is only sort of reduced mediation.
The policy can maintain all existing mount rules so that info
is not lost going forward.
The mediation on the old mount interface is reduced by the new
mount rule, but at the same time it now possible to turn of
the new mount interface, if mount is not being used by the
application.

>> The regression is in the policy, where an application like LXD/Incus
>> are specify mount rules and loading policy based on it. Those
>> restrictions continue to work on the old mount interface, they
>> however do not work as one would expect when the new mount interface
>> is used. To allow applications to use the new mount interface
>> a broad mount rule is needed, which basically makes the other
>> mount restrictions useless.
>>
>> This situation is however still better than without the patch
>> because, applications trying to use the new mount interface
>> were not restricted at all. The only regression is in mediation
>> of applications that are using the old mount interface. But
>> again those application, without the patch, can just use the
>> new interface and by-pass the restrictions from those rules. In
>> this case the broad rule is not good, but better than the by-pass.
>>
>> I am working on a patch that will improve the situation, over the
>> current patch using the existing move_mount hook. I wanted to
>> be able to get more testing in place, before we did more, and
>> just didn't have the time.
> 
> So just to understand, this change went upstream while it was known
> that effectively no sane mount policy would work with it?
> 

having a toggle to turn off mount is sane. It actually is a little
better than that in that you can do more with some cases. It still
allows full mediation under the old interface, it just can't match
how fine the mediation is with the old interface.

> That seems very odd, at that point, why not instead upstream a patch
> that disables the new mount API when apparmor is in use with mount
> rules?

It breaks applications, some will fallback, some don't. Here you
get a policy controllable toggle. Admittedly it returns -EACCES
instead of a different error code but that could be adjusted.

> That feels like it would have had the same theoretical benefits
> without the breakage that this change caused.
> 
I am willing to experiment with this more but it can break
applications.

>> But the reality is, apparmor won't be able to achieve equivalent
>> mediation without new hooks or at least more mount context passed
>> into the existing hook. That to is being looked at but won't be
>> is even further out.
> 
> That doesn't sound encouraging as far as getting a suitable fix to our users...
> 
If by suitable fix you mean equivalence of mediation between the
old and new interface, not in the short term.

> 
> Is there even a way in apparmor policies to cleanly prevent the use of
> the new mount API (return ENOSYS back to userspace)?

Its possible, it will take another patch or two kernel side for this
case. We could even add a boolean and make it so you can apply mount
rules specifically only to the old or new interface.

> We can obviously do this through a separate seccomp filter, but not
> everyone is using seccomp...
> 
> Stéphane