netdev - [PATCH] net: revert "align SO_RCVMARK required privileges with SO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230605081218.113588-1-maze@google.com>
Date: Mon,  5 Jun 2023 01:12:18 -0700
From: "Maciej Żenczykowski" <maze@...gle.com>
To: "Maciej Żenczykowski" <zenczykowski@...il.com>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>, 
	"Maciej Żenczykowski" <maze@...gle.com>, Eyal Birger <eyal.birger@...il.com>, 
	Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Patrick Rohr <prohr@...gle.com>
Subject: [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK"

This reverts:
    commit 1f86123b97491cc2b5071d7f9933f0e91890c976
    net: align SO_RCVMARK required privileges with SO_MARK

    The commit referenced in the "Fixes" tag added the SO_RCVMARK socket
    option for receiving the skb mark in the ancillary data.

    Since this is a new capability, and exposes admin configured details
    regarding the underlying network setup to sockets, let's align the
    needed capabilities with those of SO_MARK.

This reasoning is not really correct:
  SO_RCVMARK is used for 'reading' incoming skb mark (via cmsg), as such
  it is more equivalent to 'getsockopt(SO_MARK)' which has no priv check
  and retrieves the socket mark, rather than 'setsockopt(SO_MARK) which
  sets the socket mark and does require privs.

  Additionally incoming skb->mark may already be visible if
  sysctl_fwmark_reflect and/or sysctl_tcp_fwmark_accept are enabled.

  Furthermore, it is easier to block the getsockopt via bpf
  (either cgroup setsockopt hook, or via syscall filters)
  then to unblock it if it requires CAP_NET_RAW/ADMIN.

On Android the socket mark is (among other things) used to store
the network identifier a socket is bound to.  Setting it is privileged,
but retrieving it is not.  We'd like unprivileged userspace to be able
to read the network id of incoming packets (where mark is set via iptables
[to be moved to bpf])...

An alternative would be to add another sysctl to control whether
setting SO_RCVMARK is privilged or not.
(or even a MASK of which bits in the mark can be exposed)
But this seems like over-engineering...

Note: This is a non-trivial revert, due to later merged:
  commit e42c7beee71d0d84a6193357e3525d0cf2a3e168
  bpf: net: Consider has_current_bpf_ctx() when testing capable() in sk_setsockopt()
which changed both 'ns_capable' into 'sockopt_ns_capable' calls.

Fixes: 1f86123b9749 ("align SO_RCVMARK required privileges with SO_MARK")
Cc: Eyal Birger <eyal.birger@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Patrick Rohr <prohr@...gle.com>
Signed-off-by: Maciej Żenczykowski <maze@...gle.com>
---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 24f2761bdb1d..6e5662ca00fe 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1362,12 +1362,6 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
 		__sock_set_mark(sk, val);
 		break;
 	case SO_RCVMARK:
-		if (!sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) &&
-		    !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) {
-			ret = -EPERM;
-			break;
-		}
-
 		sock_valbool_flag(sk, SOCK_RCVMARK, valbool);
 		break;

-- 
2.41.0.rc0.172.g3f132b7071-goog