Potential improvements to mount-flatpak

Potential improvements to mount-flatpak

[Demi Marie Obenour]

[-- Attachment #1.1: Type: text/plain, Size: 1252 bytes --]

I looked at the mount-flatpak codebase and noticed some potential
improvements for the future:

- The code uses std::fs::write to write to files in the target
  directory.  This is safe because the VM isn't running, but it might
  be better to use libpathrs.

- The code doesn't validate the user-provided app ID, resulting in
  worse error messages than otherwise possible.  App IDs are always
  D-Bus well-known names, so there is no risk of the rules changing
  in future versions of Flatpak.

- Passing something that isn't a flatpak repository as a parameter
  will result in a confusing "no such file or directory" error.

- Various checks for corrupt repositories could be added, such as:
  - Wrong number of slashes in "current" symlink.
  - Wrong number of slashes in runtime path.
  - Bad "active" symlink.

- The code that resolves the "active" symlink is duplicated.

- The target mount point is not made "noexec".

- mount_setattr() is called on a directory FD.  I suspect this
  changes parameters of the source mount.  It might be better to use
  open_tree_attr() with OPEN_TREE_CLONE instead.

Which, if any, of these ideas would make sense to include?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Potential improvements to mount-flatpak

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> I looked at the mount-flatpak codebase and noticed some potential
> improvements for the future:
>
> - The code uses std::fs::write to write to files in the target
>   directory.  This is safe because the VM isn't running, but it might
>   be better to use libpathrs.

I think it's fine.

> - The code doesn't validate the user-provided app ID, resulting in
>   worse error messages than otherwise possible.  App IDs are always
>   D-Bus well-known names, so there is no risk of the rules changing
>   in future versions of Flatpak.

Even though Flatpak 2 won't use D-Bus?

> - Passing something that isn't a flatpak repository as a parameter
>   will result in a confusing "no such file or directory" error.

Could check that if it's cheap, since this is user-facing.  I would not
spend much time on it though, since ultimately this should not be the
interface people use.

> - Various checks for corrupt repositories could be added, such as:
>   - Wrong number of slashes in "current" symlink.
>   - Wrong number of slashes in runtime path.
>   - Bad "active" symlink.

I'm not sure I see the value?  Does normal Flatpak do these checks?
I don't think we need to check everything that could possibly go wrong
if it wouldn't be a security concern.

> - The code that resolves the "active" symlink is duplicated.

I think there's already a patch sitting at the bottom of my inbox for
you from this.

> - The target mount point is not made "noexec".

Sounds like a good improvement.

> - mount_setattr() is called on a directory FD.  I suspect this
>   changes parameters of the source mount.  It might be better to use
>   open_tree_attr() with OPEN_TREE_CLONE instead.

Well, does it change the source mount or doesn't it?  If there's a bug,
fix it; if there isn't, don't.  (I do think you are probably right though.)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: Potential improvements to mount-flatpak

[Demi Marie Obenour]

[-- Attachment #1.1: Type: text/plain, Size: 3046 bytes --]

On 7/22/26 04:17, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> I looked at the mount-flatpak codebase and noticed some potential
>> improvements for the future:
>>
>> - The code uses std::fs::write to write to files in the target
>>   directory.  This is safe because the VM isn't running, but it might
>>   be better to use libpathrs.
> 
> I think it's fine.

Makes sense.

>> - The code doesn't validate the user-provided app ID, resulting in
>>   worse error messages than otherwise possible.  App IDs are always
>>   D-Bus well-known names, so there is no risk of the rules changing
>>   in future versions of Flatpak.
> 
> Even though Flatpak 2 won't use D-Bus?

I think I forgot that.  Is there a document with the list of Flatpak
2 changes anywhere?

>> - Passing something that isn't a flatpak repository as a parameter
>>   will result in a confusing "no such file or directory" error.
> 
> Could check that if it's cheap, since this is user-facing.  I would not
> spend much time on it though, since ultimately this should not be the
> interface people use.

Makes sense.

>> - Various checks for corrupt repositories could be added, such as:
>>   - Wrong number of slashes in "current" symlink.
>>   - Wrong number of slashes in runtime path.
>>   - Bad "active" symlink.
> 
> I'm not sure I see the value?  Does normal Flatpak do these checks?
> I don't think we need to check everything that could possibly go wrong
> if it wouldn't be a security concern.

I don't see any reason either.  There is a trick one can do to
improve worst-case runtime, and that is to not use RESOLVE_BENEATH
or RESOLVE_IN_ROOT.  Instead, one uses RESOLVE_NO_SYMLINKS |
RESOLVE_NO_MAGICLINKS and checks that the path has no ".." components
and is not absolute.  My understanding is that RESOLVE_BENEATH
and RESOLVE_IN_ROOT require heavyweight kernel-side checks to guard
against race conditions, and these can produce false positives.
The other approach doesn't require these checks and so is more reliable.
libpathrs resolves this with an ugly retry loop.

It's up to you whether this change is worth making.

>> - The code that resolves the "active" symlink is duplicated.
> 
> I think there's already a patch sitting at the bottom of my inbox for
> you from this.
> 
>> - The target mount point is not made "noexec".
> 
> Sounds like a good improvement.

Yup, although all of this is in a separate mount namespace and I think
only virtiofsd runs in that namespace.

>> - mount_setattr() is called on a directory FD.  I suspect this
>>   changes parameters of the source mount.  It might be better to use
>>   open_tree_attr() with OPEN_TREE_CLONE instead.
> 
> Well, does it change the source mount or doesn't it?  If there's a bug,
> fix it; if there isn't, don't.  (I do think you are probably right though.)

There's no actual bug.  It was opened with open_tree() with OPEN_TREE_CLONE.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Potential improvements to mount-flatpak

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> On 7/22/26 04:17, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>> 
>>> - The code doesn't validate the user-provided app ID, resulting in
>>>   worse error messages than otherwise possible.  App IDs are always
>>>   D-Bus well-known names, so there is no risk of the rules changing
>>>   in future versions of Flatpak.
>> 
>> Even though Flatpak 2 won't use D-Bus?
>
> I think I forgot that.  Is there a document with the list of Flatpak
> 2 changes anywhere?

I think the talk from Linux App Summit this year is probably the most
comprehensive thing about it so far.

>> I'm not sure I see the value?  Does normal Flatpak do these checks?
>> I don't think we need to check everything that could possibly go wrong
>> if it wouldn't be a security concern.
>
> I don't see any reason either.  There is a trick one can do to
> improve worst-case runtime, and that is to not use RESOLVE_BENEATH
> or RESOLVE_IN_ROOT.  Instead, one uses RESOLVE_NO_SYMLINKS |
> RESOLVE_NO_MAGICLINKS and checks that the path has no ".." components
> and is not absolute.  My understanding is that RESOLVE_BENEATH
> and RESOLVE_IN_ROOT require heavyweight kernel-side checks to guard
> against race conditions, and these can produce false positives.
> The other approach doesn't require these checks and so is more reliable.
> libpathrs resolves this with an ugly retry loop.
>
> It's up to you whether this change is worth making.

Sounds like premature optimization to me.  All else equal I'd rather
have code in the kernel than in Spectrum.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]