Paths

Table of Contentst

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

Authored by val_packett.cool on Feb 16 2021, 3:05 PM.

Details

Reviewers

markj
mjg

Group Reviewers

Contributor Reviews (src)

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 49536
Build 46426: arc lint + arc unit

Event Timeline

val_packett.cool requested review of this revision.Feb 16 2021, 3:05 PM

val_packett.cool created this revision.

Harbormaster completed remote builds in B37052: Diff 84015.Feb 16 2021, 3:05 PM

val_packett.cool edited the summary of this revision. (Show Details)Feb 16 2021, 3:10 PM

val_packett.cool edited the test plan for this revision. (Show Details)

val_packett.cool added a reviewer: Contributor Reviews (src).

val_packett.cool set the repository for this revision to rG FreeBSD src repository.

val_packett.cool edited the summary of this revision. (Show Details)Jul 26 2021, 6:18 PM

val_packett.cool added reviewers: markj, mjg.

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.

Use cr_cansee as suggested

Herald added a subscriber: imp. · View Herald TranscriptJul 27 2021, 2:16 PM

Harbormaster completed remote builds in B40693: Diff 92796.Jul 27 2021, 2:16 PM

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.

This revision is now accepted and ready to land.Jul 27 2021, 2:36 PM

rebased patch, no changes

This revision now requires review to proceed.Feb 5 2023, 8:48 PM

Harbormaster completed remote builds in B49536: Diff 116536.Feb 5 2023, 8:48 PM

val_packett.cool edited the summary of this revision. (Show Details)Feb 5 2023, 8:48 PM

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 added inline comments.Feb 22 2023, 6:01 AM

sys/dev/md/md.c
1781	Yeah, seems to make sense. Now.. does `mdsetcred` become redundant/silly? apart from the NFS part…

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?

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.

Revision Contents
Changeset List

Path

Size

sys/

dev/

md/

md.c

15 lines

Diff 116536

View Options

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