Bug 277228

Comment 1 Poul-Henning Kamp 2024-02-23 11:49:18 UTC

Oh boy...

The 'operator' group predates FreeBSD, and I speculate that the intent was to enable non-root users to backup filesystems with dump(8), which reads the raw disk, rather than go through the filesystem.

See for instance 386BSD's MAKEDEV script, which grants 'operator' read access to disk devices:

    fd*|wd*)
            umask 2 ; unit=`expr $i : '..\(.*\)'`
            case $i in
            fd*) name=fd; blk=2; chr=9;;
            wd*) name=wd; blk=0; chr=3;;
            esac
            case $unit in
            0|1)
                    mknod ${name}${unit}a   b $blk `expr $unit '*' 8 + 0`
                    […]
                    chgrp operator ${name}${unit}[a-h] r${name}${unit}[a-h]
                    chmod 640 ${name}${unit}[a-h] r${name}${unit}[a-h]

When I implemented DEVFS (3f54a085a60) I retained those permissions, which is why ``sys/conf.h`` knows the numeric gid of the operator group, and for similar reasons it ended up knowing about a dozen non-zero uids and gids, in order to stay compatible with userland and user expectations.

I did consider having DEVFS expose everything as ``600 root:wheel`` but there were no sane way to have a trusted program in userland fix the permissions to what tradition and users expected.  (Also: RAM was /expensive/, so adding another background process was unthinkable.)

GEOM had a much tougher row to hoe and it took a number of gyrations to finally get access to "on-disk metadata" such as disklabels, MBR's and other stuff sorted out, while trying to remain compatible with what people had on their disks at the time.

Along that route I tried to get rid of the magic ``GID_OPERATOR`` in the kernel, but as (0a2ece0481909f7) attest, people relied on it still.

I do not recall anything depending on ``/dev/geom.ctl`` having ``640 root:operator`` permission, and I suspect it may have gotten those as a side effect of reusing common geom functions.

Now that we have devd(8), we could zap ``UID_*`` and ``GID_*`` from ``sys/conf.h`` and move the adjustment of permissions to userland where it belongs, and maybe we should, or maybe it is not worth the effort.

However, for multifunctional "control devices", typically named ``*ctl``, the file system access control is insufficient, because only the parameters to the actual syscalls can tell if something innocent like ``gpart show`` or privileged like ``gpart destroy -F`` is being attempted and jails are another complication.

The ioctl(2) implementation magic know some things about "read/write/both" for the ioctl(2) argument, but unfortunately that does distinctions do not readily map to "harmless" vs "consequential", the way it may have back in the 1980'ies.

And we are not just talking ``geom.ctl`` here, other magic devices like ``mlx5ctl``, ``cam/ctl``, ``sa%d.ctl``, ``apmctl`` probably also grant ``GID_OPERATOR`` near-root behaviours.

My suggestion would be to modify ``sys/conf.h`` to define ``GID_OPERATOR`` as zero, and if nothing very important breaks, leave it like that.

Comment 2 Warner Losh 2024-02-25 18:18:52 UTC

On phk's message: i think devfs likely is better... otherwise there are races. But there is no position data for devfs.. 

I'd just change the modes on geom.ctl to 600. Ownership is long gone by the time we get the message in geom. The only way to grant fine grained control i think is with a daemon to proxie things. Operator can get what it needs without read access to this file... and we should change the open to requiring write perms.... but that's an incompatible change.. 

Operator is a super powerful club. Granting it means bad things can happen...

Comment 3 Kyle Evans 2024-02-26 00:03:44 UTC

The problem is one of the examples given: they can strip it easily enough today, but they want to, e.g., be able to allow a user to partition their USB flash drive but not internal media.  Ultimately it'd be kind of nice if gpart checked permissions on devfs nodes for the disks or something to that effect to allow finer access control, but that would seem to get hairy quickly.

Comment 4 Warner Losh 2024-02-26 00:59:55 UTC

(In reply to Kyle Evans from comment #3)

There's no API for cdevs to get their owners, nor is there any process associated with the request by the time we get into the geom nodes that are fielding the verbs. So it's quite difficult to apply security checks that deeply down in the stack since there's no process context associated with the request. One could do ad-hoc things at that level, but no other drivers do that sort of thing. Once you are past the 'open' check, there's very little else. One could add checks at the ioctl, but you don't know what node(s) the command affects there (without putting all the knowledge of the lower layers into the ioctl).

What's unclear to me, though I think it would work, would just be to remove any permission for operator for /dev/geom.ctl (that is make it 0600 permission). I don't think there's anything that operator needs to do its mandate (such as it still is) that it can't get from other sources.

Operator already can read all the data in your system, though, so there's already a fair amount of trust in operator.

Are there any architectural limitations that would prevent you from making
gpart run under setuid or setgid using the same group ID as geom.ctl
(something other than operator, so that drives can still belong to operator
group for backups. etc), then let gpart check the permissions on the
individual devices before allowing you to modify the partition table?

It seems that that you could do this with any tool that needs *.ctl
permissions.