md: check uid when non-root users detach/resize the mds

[val_packett.cool]

Summary

e.g. on desktop systems, you might want to allow regular users to mount files as disks, which can be accomplished by changing /dev/mdctl permissions.

However currently in that case any user would be able to delete any other user's ramdisks.
This patch changes that.

bikeshedding: PRIV_VFS_ADMIN vs new PRIV_ or something else?

Sponsored by: https://www.patreon.com/valpackett

Test Plan

/usr/src % doas chmod 0777 /dev/mdctl
/usr/src % doas mdconfig -f README
md0
/usr/src % mdconfig -f README.md
md1
/usr/src % mdconfig -d -u 1
/usr/src % mdconfig -d -u 0
mdconfig: ioctl(/dev/mdctl): Operation not permitted

[markj]

bikeshedding: PRIV_VFS_ADMIN vs new PRIV_ or something else?

PRIV_VFS_ADMIN seems appropriate to me.

sys/dev/md/md.c

1782

Would cr_cansee(td->td_ucred->cr_uid, sc->cred->cr_uid) be a better check? That would catch the case where users in different jails with the same UIDs have access to mdctl. There are other isolation problems with respect to md disks and jails, but this would be a step in the right direction.

[val_packett.cool]

Use cr_cansee as suggested

[markj]

Seems reasonable to me. I do wonder though why malloc-backed devices don't carry a credential reference. As far as I can see, if you change the mdctl mode there's nothing preventing unprivileged users from creating them.

It would also be nice to have some regression tests for this. Ideally we could use a jail to modify mdctl permissions without affecting the host, but it's a pain to create a custom devfs ruleset from a test, I think.

[val_packett.cool]

rebased patch, no changes

[markj]

I'm sorry for having follow-up comments after I clicked accept long ago.

sys/dev/md/md.c

1781

I think this isn't quite right:

• PRIV_VFS_ADMIN is granted to a jailed root, so even with this change, a jailed root can destroy an MD device on the host. The cr_cansee() check would prevent that, so I think it should be done unconditionally, i.e., no priv_check(). But:
• It's possible for an MD device to not have a credential. Specifically, malloc-backed MDs do not have one. I guess this is because there's special support for creating a malloc-backed MD from within the kernel, in md_preloaded()? We should just use thread0.td_ucred in that case IMO.

So now it looks like the right way to do this is to ensure that MD devices always have a credential, and to unconditionally perform the cr_cansee() check.

I would suggest adding a new cred parameter to mdnew(). In kern_mdattach_locked(), use td->td_ucred, and in md_preloaded(), use thread0.td_ucred. Then drop the priv_check() and handling for sc->cred == NULL. Does that seem sensible?

[val_packett.cool]

sys/dev/md/md.c

1781

Yeah, seems to make sense. Now.. does mdsetcred become redundant/silly? apart from the NFS part…

[val_packett.cool]

Oops… cr_cansee actually would only deny this with security.bsd.see_other_uids=0 btw. Is there a "fs write" style check anywhere in the common code?

[markj]

In D28704#881190, @val_packett.cool wrote:

Oops… cr_cansee actually would only deny this with security.bsd.see_other_uids=0 btw. Is there a "fs write" style check anywhere in the common code?

I see, yes. In that case, we should compare the prison pointers of the two ucreds as well as the UIDs.

sys/dev/md/md.c

1781

Yeah, I'd just push the crhold() into kern_mdattach_locked().

I have no idea whether that hack for NFS makes sense anymore. I hope not. But that can be moved into mdcreate_vnode() I think.