Remove virtfs-proxy-helper dependency

[bensmrs]

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?

[stgraber]

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]

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]

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]

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]

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]

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

[stgraber]

Thanks!

[bensmrs]

I’ll soon have something, I’ve had a busy last week, sorry for the delay…
I wanted to point out that raw.idmap has the condition “unprivileged container” in the documentation, although it is also supposed to work in VMs. Shall I fix that in this PR?

[bensmrs]

It was poorly written, we actually don’t need to check whether the VM is down