Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove virtfs-proxy-helper dependency #1547

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove virtfs-proxy-helper dependency #1547

wants to merge 2 commits into from

Conversation

bensmrs
Copy link
Contributor

@bensmrs bensmrs commented Dec 24, 2024

Closes: #1543

Please note that backwards compatibility WAS NOT tested. Also, I used the mapped-xattr security model; maybe it should have its own dedicated option in device config?

@bensmrs bensmrs requested a review from stgraber as a code owner December 24, 2024 15:09
@bensmrs bensmrs changed the title incusd/device/disk: Remove virtfs-proxy-helper dependency Remove virtfs-proxy-helper dependency Dec 24, 2024
@stgraber
Copy link
Member

So I guess the main feature gap here is that using the local fsdriver doesn't let us do the idmap handling? So if someone has raw.idmap set on their VM, the filesystem access doesn't get correctly limited/mapped.

We need to either check if there's a way to handle or otherwise if raw.idmap is set, we'll need to disable the 9p drive entirely. Failing to do this could lead to a rather major security issue as unpriv Incus users (member of incus and not incus-admin) can't be allowed to pass in a directory and then have the VM trigger file creation on there as a uid/gid that they don't own (otherwise they could easily copy a shell to the disk and make it setuid-root, then use that to gain real root on the host).

@bensmrs
Copy link
Contributor Author

bensmrs commented Dec 24, 2024

So I guess the main feature gap here is that using the local fsdriver doesn't let us do the idmap handling? So if someone has raw.idmap set on their VM, the filesystem access doesn't get correctly limited/mapped.

Yeah, that’s the cost of abandoning the proxy driver.

We need to either check if there's a way to handle or otherwise if raw.idmap is set, we'll need to disable the 9p drive entirely.

I don’t think it can be handled properly without… erm… a proxy :P
Maybe we could do an idmapped bind mount on the host (with X-mount.idmap) and exposing this mount to the guest.

Failing to do this could lead to a rather major security issue as unpriv Incus users (member of incus and not incus-admin) can't be allowed to pass in a directory and then have the VM trigger file creation on there as a uid/gid that they don't own (otherwise they could easily copy a shell to the disk and make it setuid-root, then use that to gain real root on the host).

… and I think it calls for a security test suite :)
Btw, what is currently preventing what you’re describing if no idmap is given?

@stgraber
Copy link
Member

Btw, what is currently preventing what you’re describing if no idmap is given?

restricted.idmap.uid and restricted.idmap.gid get set at the project level, when those are set, all raw.idmap values are then checked against what the project allows.

If either raw.idmap is set on the instance or restricted.devices.disk.paths is set on the project, then the VM's disk processes will run in a user namespace.

@bensmrs
Copy link
Contributor Author

bensmrs commented Dec 25, 2024

Oh I see. Not sure exactly how everything is put together, especially in clustered environments; that’s a code path I haven’t looked at yet. Anyway, regarding the bind mount, I think the code can be adapted to make it work, but we’d need a place to put the bind-mounted directories. If we go in this direction, what would that place be?

@stgraber
Copy link
Member

stgraber commented Jan 3, 2025

It would be possible under /var/lib/incus/devices but this gets pretty tricky pretty quickly.
First we'd need VFS idmap support on whatever the source filesystem ends up being (not too bad on recent kernels), then we'd either need pretty fancy handling of recursive mounts or just limit that to a single mount and finally we'd need to set things up to prevent random users traversing to the idmap shifted bind-mount tree.

I suspect that's just too much work for the benefit we're getting out of it.

These days io.bus defaults to auto, which normally means virtiofs+9p.

We can tweak this a bit further so that the default remains virtiofs+9p unless and idmap has been provided, in which case, we skip 9p. If the user specifically requested 9p, then they should get an error.

We also need to use passthrough mode for the security model as the way things currently work, users changing ownership or permissions inside the VM do expect those to change on the host too.

@bensmrs
Copy link
Contributor Author

bensmrs commented Jan 6, 2025

Ok, I’ll see what I can do this week

@stgraber
Copy link
Member

stgraber commented Jan 7, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Can't start VM with qemu 9.2: Required binary "virtfs-proxy-helper" couldn't be found
2 participants