netdev - Re: [PATCH v7 5/6] connector/cn_proc: Allow non-root users access

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2B585919-6017-4496-B001-F6CEE8F35470@oracle.com>
Date: Thu, 6 Jul 2023 21:00:27 +0000
From: Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
To: Liam Howlett <liam.howlett@...cle.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        "david@...es.net"
	<david@...es.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        Evgeniy Polyakov
	<zbr@...emap.net>,
        Christian Brauner <brauner@...nel.org>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>,
        "leon@...nel.org"
	<leon@...nel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "socketcan@...tkopp.net" <socketcan@...tkopp.net>,
        "petrm@...dia.com"
	<petrm@...dia.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: [PATCH v7 5/6] connector/cn_proc: Allow non-root users access



> On Jul 6, 2023, at 1:16 PM, Liam Howlett <liam.howlett@...cle.com> wrote:
> 
> * Anjali Kulkarni <anjali.k.kulkarni@...cle.com> [691231 23:00]:
>> There were a couple of reasons for not allowing non-root users access
>> initially - one is there was some point no proper receive buffer
>> management in place for netlink multicast. But that should be long
>> fixed. See link below for more context.
>> 
>> Second is that some of the messages may contain data that is root only. But
>> this should be handled with a finer granularity, which is being done at the
>> protocol layer. The only problematic protocols are nf_queue and the
>> firewall netlink. Hence, this restriction for non-root access was relaxed
>> for NETLINK_ROUTE initially:
>> https://lore.kernel.org/all/20020612013101.A22399@wotan.suse.de/
>> 
>> This restriction has also been removed for following protocols:
>> NETLINK_KOBJECT_UEVENT, NETLINK_AUDIT, NETLINK_SOCK_DIAG,
>> NETLINK_GENERIC, NETLINK_SELINUX.
>> 
>> Since process connector messages are not sensitive (process fork, exit
>> notifications etc.), and anyone can read /proc data, we can allow non-root
>> access here. However, since process event notification is not the only
>> consumer of NETLINK_CONNECTOR, we can make this change even more
>> fine grained than the protocol level, by checking for multicast group
>> within the protocol.
>> 
>> Allow non-root access for NETLINK_CONNECTOR via NL_CFG_F_NONROOT_RECV
>> but add new bind function cn_bind(), which allows non-root access only
>> for CN_IDX_PROC multicast group.
>> 
>> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
>> ---
>> drivers/connector/cn_proc.c | 7 -------
>> drivers/connector/connector.c | 19 +++++++++++++++++++
>> 2 files changed, 19 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
>> index dfc84d44f804..bb1fa5d66cf9 100644
>> --- a/drivers/connector/cn_proc.c
>> +++ b/drivers/connector/cn_proc.c
>> @@ -410,12 +410,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>> 	 !task_is_in_init_pid_ns(current))
>> 		return;
>> 
>> -	/* Can only change if privileged. */
>> -	if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) {
>> -		err = EPERM;
>> -		goto out;
>> -	}
>> -
>> 	if (msg->len == sizeof(*pinput)) {
>> 		pinput = (struct proc_input *)msg->data;
>> 		mc_op = pinput->mcast_op;
>> @@ -467,7 +461,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>> 		break;
>> 	}
>> 
>> -out:
> 
> This label is still in use from your changes in patch 3.

Yes, will send updated patch with this fixed.

> 
>> 	cn_proc_ack(err, msg->seq, msg->ack);
>> }
>> 
>> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
>> index d1179df2b0ba..7f7b94f616a6 100644
>> --- a/drivers/connector/connector.c
>> +++ b/drivers/connector/connector.c
>> @@ -166,6 +166,23 @@ static int cn_call_callback(struct sk_buff *skb)
>> 	return err;
>> }
>> 
>> +/*
>> + * Allow non-root access for NETLINK_CONNECTOR family having CN_IDX_PROC
>> + * multicast group.
>> + */
>> +static int cn_bind(struct net *net, int group)
>> +{
>> +	unsigned long groups = (unsigned long) group;
>> +
>> +	if (ns_capable(net->user_ns, CAP_NET_ADMIN))
>> +		return 0;
>> +
>> +	if (test_bit(CN_IDX_PROC - 1, &groups))
>> +		return 0;
>> +
>> +	return -EPERM;
>> +}
>> +
>> static void cn_release(struct sock *sk, unsigned long *groups)
>> {
>> 	if (groups && test_bit(CN_IDX_PROC - 1, groups)) {
>> @@ -261,6 +278,8 @@ static int cn_init(void)
>> 	struct netlink_kernel_cfg cfg = {
>> 		.groups	= CN_NETLINK_USERS + 0xf,
>> 		.input	= cn_rx_skb,
>> +		.flags = NL_CFG_F_NONROOT_RECV,
>> +		.bind = cn_bind,
>> 		.release = cn_release,
>> 	};
>> 
>> -- 
>> 2.41.0