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

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

[akpm]

From: Jeff Mahoney <jeffm@suse.com>
Subject: mISDN: add support for group membership check

This patch adds a module parameter to allow a group access to the mISDN
devices.  Otherwise, unpriviledged users on systems with ISDN hardware
have the ability to dial out, potentially causing expensive bills.

Based on a different implementation by Patrick Koppen.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Acked-by: Jeff Mahoney <jeffm@suse.com>
Cc: Patrick Koppen <isdn4linux@koppen.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/isdn/mISDN/core.c   |    6 ++++++
 drivers/isdn/mISDN/core.h   |    1 +
 drivers/isdn/mISDN/socket.c |    9 +++++++++
 3 files changed, 16 insertions(+)

diff -puN drivers/isdn/mISDN/core.c~misdn-add-support-for-group-membership-check drivers/isdn/mISDN/core.c
--- 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);
+MODULE_PARM_DESC(gid, "Unix group for accessing misdn socket (default 0)");
 
 static u64		device_ids;
 #define MAX_DEVICE_ID	63
@@ -372,6 +376,8 @@ mISDNInit(void)
 {
 	int	err;
 
+	misdn_permitted_gid = make_kgid(current_user_ns(), gid);
+
 	printk(KERN_INFO "Modular ISDN core version %d.%d.%d\n",
 	       MISDN_MAJOR_VERSION, MISDN_MINOR_VERSION, MISDN_RELEASE);
 	mISDN_init_clock(&debug);
diff -puN drivers/isdn/mISDN/core.h~misdn-add-support-for-group-membership-check drivers/isdn/mISDN/core.h
--- a/drivers/isdn/mISDN/core.h~misdn-add-support-for-group-membership-check
+++ a/drivers/isdn/mISDN/core.h
@@ -17,6 +17,7 @@
 
 extern struct mISDNdevice	*get_mdevice(u_int);
 extern int			get_mdevice_count(void);
+extern kgid_t misdn_permitted_gid;
 
 /* stack status flag */
 #define mISDN_STACK_ACTION_MASK		0x0000ffff
diff -puN drivers/isdn/mISDN/socket.c~misdn-add-support-for-group-membership-check drivers/isdn/mISDN/socket.c
--- 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;
 
@@ -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;
_

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

[David Miller]

From: akpm@linux-foundation.org
Date: Fri, 13 Sep 2013 14:52:02 -0700

> @@ -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;

This and the other conditional need to be styled correctly:

	if (a &&
	    b &&
	    c)

the object is to get the second and subsequent lines to start at the
first column after the openning parenthesis of the condition, using
the appropriate number of tab and space characters.

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]

On Fri, 2013-09-13 at 14:52 -0700, akpm@linux-foundation.org wrote:
> From: Jeff Mahoney <jeffm@suse.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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[Jiri Slaby]

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?

thanks,
-- 
js
suse labs

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

[David Miller]

From: Jiri Slaby <jslaby@suse.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.

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

[Jiri Slaby]

On 09/20/2013 05:56 PM, David Miller wrote:
> From: Jiri Slaby <jslaby@suse.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.

What I'm suggesting is just to put a !capable(CAP_NET_ADMIN) test into
the rename path and nothing more.

-- 
js
suse labs

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

[David Miller]

From: Jiri Slaby <jslaby@suse.cz>
Date: Fri, 20 Sep 2013 18:14:28 +0200

> On 09/20/2013 05:56 PM, David Miller wrote:
>> From: Jiri Slaby <jslaby@suse.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.
> 
> What I'm suggesting is just to put a !capable(CAP_NET_ADMIN) test into
> the rename path and nothing more.

And I'm saying that regardless of such a change, the patch itself
is fundamentally implemented incorrectly and not acceptable for
upstream inclusion until the interface for configuration is changed.

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

[Jiri Slaby]

On 09/20/2013 06:15 PM, David Miller wrote:
> From: Jiri Slaby <jslaby@suse.cz>
> Date: Fri, 20 Sep 2013 18:14:28 +0200
> 
>> On 09/20/2013 05:56 PM, David Miller wrote:
>>> From: Jiri Slaby <jslaby@suse.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.
>>
>> What I'm suggesting is just to put a !capable(CAP_NET_ADMIN) test into
>> the rename path and nothing more.
> 
> And I'm saying that regardless of such a change, the patch itself
> is fundamentally implemented incorrectly and not acceptable for
> upstream inclusion until the interface for configuration is changed.

Ok, let's leave the hole in there then.

-- 
js
suse labs

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

[David Miller]

From: Jiri Slaby <jslaby@suse.cz>
Date: Fri, 20 Sep 2013 18:18:02 +0200

> Ok, let's leave the hole in there then.

Jiri, don't turn this on me.

The issue is not whether something needs to be fixed or not, I never
said "don't fix this" and therefore I'd like to ask politely that you
not make it seem like I have.

Rather, I'm saying that the implementation is terrible, sets a bad
precendence for fixing similar problems, and therefore not is in
a state where we can apply it.

So, I'm not saying "don't fix this", I'm saying "fix it properly."

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

[Jiri Slaby]

On 09/20/2013 06:21 PM, David Miller wrote:
> From: Jiri Slaby <jslaby@suse.cz>
> Date: Fri, 20 Sep 2013 18:18:02 +0200
> 
>> Ok, let's leave the hole in there then.
> 
> Jiri, don't turn this on me.
> 
> The issue is not whether something needs to be fixed or not, I never
> said "don't fix this" and therefore I'd like to ask politely that you
> not make it seem like I have.
> 
> Rather, I'm saying that the implementation is terrible, sets a bad
> precendence for fixing similar problems, and therefore not is in
> a state where we can apply it.
> 
> So, I'm not saying "don't fix this", I'm saying "fix it properly."

Yeah, I understand your point and agree, that the parameter is ugly as
hell. But investing any more time in ISDN is not worth it, I think.
Look, I sent the patch a couple times to the ISDN maintainers and they
didn't even bother to reply. And I am not familiar with the code and do
not want to spend more time than putting there a single check, sorry.
Hence, if there is anybody from the ISDN fellows to take care of that,
nice. If not, we will just live with this patch locally some more years
and drop it when the ISDN subsystem is dropped from the kernel, if
dropped at all.

thanks,
-- 
js
suse labs

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

[Ben Hutchings]

On Fri, Sep 20, 2013 at 12:21:42PM -0400, David Miller wrote:
> From: Jiri Slaby <jslaby@suse.cz>
> Date: Fri, 20 Sep 2013 18:18:02 +0200
> 
> > Ok, let's leave the hole in there then.
> 
> Jiri, don't turn this on me.
> 
> The issue is not whether something needs to be fixed or not, I never
> said "don't fix this" and therefore I'd like to ask politely that you
> not make it seem like I have.
> 
> Rather, I'm saying that the implementation is terrible, sets a bad
> precendence for fixing similar problems, and therefore not is in
> a state where we can apply it.
> 
> So, I'm not saying "don't fix this", I'm saying "fix it properly."

I think Jiri's last suggestion was to add the capability check to
device renaming (which I think everyone agrees is needed) and not
to add any group identity checks for any operation.

Isn't that fixing it properly?

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

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

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Fri, 20 Sep 2013 17:30:44 +0100

> On Fri, Sep 20, 2013 at 12:21:42PM -0400, David Miller wrote:
>> From: Jiri Slaby <jslaby@suse.cz>
>> Date: Fri, 20 Sep 2013 18:18:02 +0200
>> 
>> > Ok, let's leave the hole in there then.
>> 
>> Jiri, don't turn this on me.
>> 
>> The issue is not whether something needs to be fixed or not, I never
>> said "don't fix this" and therefore I'd like to ask politely that you
>> not make it seem like I have.
>> 
>> Rather, I'm saying that the implementation is terrible, sets a bad
>> precendence for fixing similar problems, and therefore not is in
>> a state where we can apply it.
>> 
>> So, I'm not saying "don't fix this", I'm saying "fix it properly."
> 
> I think Jiri's last suggestion was to add the capability check to
> device renaming (which I think everyone agrees is needed) and not
> to add any group identity checks for any operation.
> 
> Isn't that fixing it properly?

I'm saying that using a module parameter for this is a deeper and
even more important problem with this change.

He seems to agree with me.

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

[Karsten Keil]

Hi Jiri,

thanks for bringing up this this issue again and sorry for not jumping
in earlier.

Am 20.09.2013 18:30, schrieb Jiri Slaby:
> On 09/20/2013 06:21 PM, David Miller wrote:
>> From: Jiri Slaby <jslaby@suse.cz>
>> Date: Fri, 20 Sep 2013 18:18:02 +0200
>>
>>> Ok, let's leave the hole in there then.
>>
>> Jiri, don't turn this on me.
>>
>> The issue is not whether something needs to be fixed or not, I never
>> said "don't fix this" and therefore I'd like to ask politely that you
>> not make it seem like I have.
>>
>> Rather, I'm saying that the implementation is terrible, sets a bad
>> precendence for fixing similar problems, and therefore not is in
>> a state where we can apply it.

OK, the history of this this patch is, that the somebody came up with
the issue, that with the old device node implementation it was simple to
restrict access to a group via standard unix file access rules to avoid
normal users could use ISDN to sniff the bus or even worst for the owner
make high cost calls.
Then somebody came up with this idea, which did allow a similar level of
protection. I did not like it very much nor I had a better idea, SuSE
security people did want it, so it was implemented. I never tried to
push it.

>>
>> So, I'm not saying "don't fix this", I'm saying "fix it properly."
> 

David, since you mention that here are maybe other drivers with same
issues, do you have an idea about a better interface or maybe a
completely other way which could be used to restrict the access to some
group of users.

> Yeah, I understand your point and agree, that the parameter is ugly as
> hell. But investing any more time in ISDN is not worth it, I think.
> Look, I sent the patch a couple times to the ISDN maintainers and they
> didn't even bother to reply. And I am not familiar with the code and do
> not want to spend more time than putting there a single check, sorry.
> Hence, if there is anybody from the ISDN fellows to take care of that,
> nice. If not, we will just live with this patch locally some more years
> and drop it when the ISDN subsystem is dropped from the kernel, if
> dropped at all.