netdev - Re: [patch 2/4] mISDN: add support for group membership check

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]

Date:	Sun, 15 Sep 2013 00:28:56 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	akpm@...ux-foundation.org, jeffm@...e.com
Cc:	davem@...emloft.net, netdev@...r.kernel.org, isdn4linux@...pen.de,
	isdn@...ux-pingi.de, jslaby@...e.cz,
	sergei.shtylyov@...entembedded.com
Subject: Re: [patch 2/4] mISDN: add support for group membership check

On Fri, 2013-09-13 at 14:52 -0700, akpm@...ux-foundation.org wrote:
> From: Jeff Mahoney <jeffm@...e.com>
> Subject: mISDN: add support for group membership check
> 
> This patch adds a module parameter to allow a group access to the mISDN
> devices.

This doesn't just 'add support' or 'allow a group access'.  It also
changes the default behaviour.

[...]
> --- a/drivers/isdn/mISDN/core.c~misdn-add-support-for-group-membership-check
> +++ a/drivers/isdn/mISDN/core.c
> @@ -21,10 +21,14 @@
>  #include "core.h"
>  
>  static u_int debug;
> +static u_int gid;
> +kgid_t misdn_permitted_gid;
>  
>  MODULE_AUTHOR("Karsten Keil");
>  MODULE_LICENSE("GPL");
>  module_param(debug, uint, S_IRUGO | S_IWUSR);
> +module_param(gid, uint, 0);

So you can't change the privileged gid after loading, or even see what
it is.

[...]
> --- a/drivers/isdn/mISDN/socket.c~misdn-add-support-for-group-membership-check
> +++ a/drivers/isdn/mISDN/socket.c
> @@ -612,6 +612,11 @@ data_sock_create(struct net *net, struct
>  {
>  	struct sock *sk;
>  
> +	if (!capable(CAP_SYS_ADMIN) &&
> +			!gid_eq(misdn_permitted_gid, current_gid()) &&
> +			!in_group_p(misdn_permitted_gid))
> +		return -EPERM;
> +
>  	if (sock->type != SOCK_DGRAM)
>  		return -ESOCKTNOSUPPORT;
>  

I'm pretty sure you could restrict this with LSMs, in a much more
flexible way.

> @@ -694,6 +699,10 @@ base_sock_ioctl(struct socket *sock, uns
>  	case IMSETDEVNAME:
>  	{
>  		struct mISDN_devrename dn;
> +		if (!capable(CAP_SYS_ADMIN) &&
> +				!gid_eq(misdn_permitted_gid, current_gid()) &&
> +				!in_group_p(misdn_permitted_gid))
> +			return -EPERM;
>  		if (copy_from_user(&dn, (void __user *)arg,
>  				   sizeof(dn))) {
>  			err = -EFAULT;

This seems to be the important bit: renaming of devices (if allowed at
all) ought to be limited to CAP_SYS_ADMIN or possibly CAP_NET_ADMIN.
But why should the group that is allowed to use mISDN data sockets also
be allowed to do this?

Ben.

-- 
Ben Hutchings
In a hierarchy, every employee tends to rise to his level of incompetence.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)