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

[Anjali Kulkarni]

> On Jul 6, 2023, at 1:16 PM, Liam Howlett <liam.howlett@xxxxxxxxxx> wrote:
> 
> * Anjali Kulkarni <anjali.k.kulkarni@xxxxxxxxxx> [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@xxxxxxxxxxxxx/
>> 
>> 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@xxxxxxxxxx>
>> ---
>> 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