Bug 200185 - if_tap: Deprecate net.link.tap.user_open sysctl
Summary: if_tap: Deprecate net.link.tap.user_open sysctl
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Kyle Evans
URL:
Keywords: needs-qa, patch
Depends on:
Blocks:
 
Reported: 2015-05-14 02:58 UTC by Yuri Victorovich
Modified: 2019-10-21 14:39 UTC (History)
4 users (show)

See Also:
koobs: mfc-stable11?
koobs: mfc-stable12?


Attachments
patch (3.17 KB, patch)
2015-05-14 02:58 UTC, Yuri Victorovich
no flags Details | Diff
git(1) diff (2.77 KB, patch)
2019-09-15 02:18 UTC, Kyle Evans
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Victorovich freebsd_committer 2015-05-14 02:58:13 UTC
Created attachment 156767 [details]
patch

net.link.tap.user_open does two thing:
* prevents non-root users to open /dev/tapN
* prevents non-root users to clone /dev/tapN

The first function is performed by the node permissions, as set by the admin. There is no need for this additional way to limit users to open devices.

The second function refers to the legacy cloning process. When some process attempts to open the non-existent device, devfs tries to call 'clone' functions of all available modules to see if they can auto-create such device. 'tapclone' is the relevant function in 'tap' module.

There is another sysctl variable net.link.tap.devfs_cloning, that currently allows/disallows cloning for everybody, and then net.link.tap.user_open checks for PRIV_NET_IFCREATE when set.

The new behavior is that net.link.tap.devfs_cloning also checks credentials based on PRIV_NET_IFCREATE, like this is currently for tunN.

Practically speaking, net.link.tap.user_open is always in the way of every user process which needs to use /dev/tapN, while such limitation is not necessary at all. Ex. user being in the 'network' group should be entirely sufficient. Admin should set these permissions.

Also, I doubt that PRIV_NET_IFCREATE even works properly, because I wasn't able to clone /dev/tapN even when my user is in wheel and network groups, and /dev is owned by root:wheel and has 0777 mask. I still got 'Permission denied'.

I suggest to apply this patch to 11 (trunk) only, because it incurs an interface change.

Also the following note should be added to the Change Log for 11.0:
* net.link.tap.user_open sysctl variable is deprecated. Opening of /dev/tapN is now based on the node permissions and user credentials only. The meaning of net.link.tap.devfs_cloning has changed: when set to non-zero it allows /dev/tapN cloning to users with PRIV_NET_IFCREATE privilege.
Comment 1 Kyle Evans freebsd_committer 2019-04-25 18:38:31 UTC
I'm attempting to merge tun and tap, then I'll look at deprecating it.
Comment 2 Kyle Evans freebsd_committer 2019-05-08 17:23:22 UTC
CC'ing rwatson@- PRIV_NET_TAP is allocated and also used for allowing opening of tap devices, because this is historically a superuser-only privilege. It was added after the user_open sysctl and we currently honor PRIV_NET_TAP xor user_open; it seems like PRIV_NET_TAP should've pushed user_open towards deprecation in favor of MAC policy to more cleanly do the same thing.

I'm not sure now what the correct behavior is- your point about groups is good, but do we want to (also, can we?) do away with PRIV_NET_TAP in favor of relying on group membership?

> Also, I doubt that PRIV_NET_IFCREATE even works properly, because I wasn't able to clone /dev/tapN even when my user is in wheel and network groups, and /dev is owned by root:wheel and has 0777 mask. I still got 'Permission denied'.

Basically all PRIV_* are only granted to root by default without a policy to grant them otherwise, so this is correct behavior.
Comment 3 Philip Paeps freebsd_committer 2019-05-08 18:12:56 UTC
While a MAC policy is a good idea, simple group policy is very easy to implement and I expect I'm not the only one using that in the wild (together with the sysctl).

I would definitely like the sysctl to go away and group policy to "just work".

I have no strong opinions on a MAC policy.
Comment 4 Kristof Provost freebsd_committer 2019-05-08 18:19:19 UTC
(In reply to Kyle Evans from comment #2)
Yuri makes excellent points w.r.t. the sysctl, and I agree that we should just get rid of it (in 13 now).

I'm less sure about getting rid of PRIV_NET_TAP, because creating the interface is distinct from opening (and using) it. We may still want to express letting users use an existing interface without immediately allowing them to create new ones.
Comment 5 Kyle Evans freebsd_committer 2019-05-08 18:20:51 UTC
(In reply to Kristof Provost from comment #4)

PRIV_NET_TAP currently only applies to the opening of a tapN interface; PRIV_NET_IFCREATE for cloning. I'm also in favor of letting group policy dictate it, but PRIV_NET_TAP is the barrier when we remove the sysctl.
Comment 6 Kyle Evans freebsd_committer 2019-09-15 02:18:22 UTC
Created attachment 207504 [details]
git(1) diff

Attaching the route I'm planning to take after sorting out a method to mark this sysctl as deprecated beforehand in https://reviews.freebsd.org/D21662 -- rip out the sysctl and add an option, TAP_OPEN_PRIVILEGED (OFF by default), for letting interested parties re-enable super user policy on their kernel. This is a lightweight, low-maintenance option.
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-15 02:29:12 UTC
Can/should this be merged or is is considered breaking (and relnotes) ?

If not set mfc-stable* to -
Comment 8 Kyle Evans freebsd_committer 2019-09-15 02:31:49 UTC
(In reply to Kubilay Kocak from comment #7)

Half of it gets MFC'd- the half where the sysctl gets marked as deprecated to announce to anyone that views/tries to use it that it's deprecated.
Comment 9 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-15 02:58:45 UTC
(In reply to Kyle Evans from comment #8)

Roger that, thank you for clarifying
Comment 10 commit-hook freebsd_committer 2019-10-21 14:39:01 UTC
A commit references this bug:

Author: kevans
Date: Mon Oct 21 14:38:12 UTC 2019
New revision: 353798
URL: https://svnweb.freebsd.org/changeset/base/353798

Log:
  tuntap(4): restrict scope of net.link.tap.user_open slightly

  net.link.tap.user_open has historically allowed non-root users to do devfs
  cloning and open /dev/tap* nodes based on permissions. Loosen this up to
  make it only allow users to do devfs cloning -- we no longer check it in
  tunopen.

  This allows tap devices to be created that can actually be opened by a user,
  rather than swiftly restricting them to root because the magic sysctl has
  not been set.

  The sysctl has not yet been completely deprecated, because more thought is
  needed for how to handle the devfs cloning case. There is not an easy
  suitable replacement for the sysctl there, and more care needs to be placed
  in determining whether that's OK or not.

  PR:		200185

Changes:
  head/UPDATING
  head/sys/net/if_tuntap.c