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

[Liam R. Howlett]

* 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 | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 35bec1fd7ee0..046a8c1d8577 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -408,12 +408,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;
> @@ -460,7 +454,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>  		break;
>  	}
>  
> -out:
>  	cn_proc_ack(err, msg->seq, msg->ack);
>  }
>  
> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
> index d1179df2b0ba..193d3056de64 100644
> --- a/drivers/connector/connector.c
> +++ b/drivers/connector/connector.c
> @@ -166,6 +166,18 @@ static int cn_call_callback(struct sk_buff *skb)
>  	return err;
>  }
>  

Should there be a comment here about non-root access?

> +static int cn_bind(struct net *net, int group)
> +{
> +	unsigned long groups = 0;
> +	groups = (unsigned long) group;

I don't understand why you have this cast on a second line or why you
zero groups before the assignment?  It would all fit on one line.

> +
> +	if (ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return 0;

New line here please

> +	if (test_bit(CN_IDX_PROC - 1, &groups))
> +		return 0;

New line here please

> +	return -EPERM;
> +}
> +
>  static void cn_release(struct sock *sk, unsigned long *groups)
>  {
>  	if (groups && test_bit(CN_IDX_PROC - 1, groups)) {
> @@ -261,6 +273,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.40.0
>