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:	Fri, 20 Sep 2013 11:56:34 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	jslaby@...e.cz
Cc:	ben@...adent.org.uk, akpm@...ux-foundation.org, jeffm@...e.com,
	netdev@...r.kernel.org, isdn4linux@...pen.de, isdn@...ux-pingi.de,
	sergei.shtylyov@...entembedded.com
Subject: Re: [patch 2/4] mISDN: add support for group membership check

From: Jiri Slaby <jslaby@...e.cz>
Date: Fri, 20 Sep 2013 15:44:33 +0200

> On 09/15/2013 01:28 AM, Ben Hutchings wrote:
>>> @@ -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?
> 
> This is based on an old patch we are dragging in SUSE since 2009:
> http://www.isdn4linux.de/pipermail/isdn4linux/2009-December/004493.html
> https://bugzilla.novell.com/show_bug.cgi?id=564423
> 
> The whole point of the gid-based access was to still allow some user
> group to manipulate the device in an arbitrary way.
> 
> So if everybody agrees I will just disallow rename to
> non-CAP_NET_ADMIN users and we are done?

No we are not done, sorry.

Having a device specific module parameter for this is wrong on several
fundamental levels.

Module parameters are wrong because you're going to eventually want to
do this for other ISDN or other devices, and nobody is going to make
sure the name of the parameter nor the semantics all match up.

This is the worst possible user experience.

You need to make it so that there is a standard, regular, interface
for performing this task or making this setting.  Module parameters
are not it.

The reason this patch has rotted since 2009 is that it really is far
from suitable for upstream inclusion, and I really don't see that much
thought or care has been put into how it is implemented, so I wonder
how much anyone really cares about the change.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html