[PATCH net 0/2] net: diag: fix a potential security issue

[PATCH net 0/2] net: diag: fix a potential security issue

[Xin Long]

This patch is to void the potential security issue that the family
or protocol modules are autoloaded when requesting _diag module by
not requesting _diag module if the family or protocol is not added
or registered in sock_diag and inet_diag.

As the repost of the patch '[PATCH net] sock_diag: request _diag
module only when the family or proto has been registered', this
patchset fixes the compiling errors when INET is not set, and
also split into two patches to make it clear to review.

Xin Long (2):
  sock_diag: request _diag module only when the family has been
    registered
  inet_diag: request _diag module only when the proto has been
    registered

 include/linux/net.h    |  1 +
 include/net/protocol.h |  1 +
 net/core/sock_diag.c   | 21 +++++++++++++--------
 net/ipv4/inet_diag.c   |  3 ++-
 net/ipv4/protocol.c    |  6 ++++++
 net/socket.c           |  5 +++++
 6 files changed, 28 insertions(+), 9 deletions(-)

-- 
2.1.0

[PATCH net 1/2] sock_diag: request _diag module only when the family has been registered

[Xin Long]

Now when using 'ss' in iproute, kernel would try to load all _diag
modules. It causes the corresponding family or proto modules to be
loaded as well.

Like after 'ss -a', sctp, dccp, af_packet(if it works as a moudle)
will be loaded.

As these family or proto modules are loaded unexpectly, this might
have some security implications.

This patch is to introduce sock_diag_request_module() in sock_diag
where we only request _diag module when it's corresponding family
has been registered. The fix for inet_diag will be done in later
patch.

Note that we can't just load _diag module without the family or
proto loaded, as some symbols in _diag module are from the family
or proto moudle.

Fixes: 8ef874bfc729 ("sock_diag: Move the sock_ code to net/core/")
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/net.h  |  1 +
 net/core/sock_diag.c | 21 +++++++++++++--------
 net/socket.c         |  5 +++++
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/linux/net.h b/include/linux/net.h
index d97d80d..6c7cf09 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -225,6 +225,7 @@ enum {
 int sock_wake_async(struct socket_wq *sk_wq, int how, int band);
 int sock_register(const struct net_proto_family *fam);
 void sock_unregister(int family);
+bool sock_is_registered(int family);
 int __sock_create(struct net *net, int family, int type, int proto,
 		  struct socket **res, int kern);
 int sock_create(int family, int type, int proto, struct socket **res);
diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 217f4e3..643a446 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -207,6 +207,15 @@ void sock_diag_unregister(const struct sock_diag_handler *hnld)
 }
 EXPORT_SYMBOL_GPL(sock_diag_unregister);
 
+static int sock_diag_request_module(int family)
+{
+	if (!sock_is_registered(family))
+		return -ENOENT;
+
+	return request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
+			      NETLINK_SOCK_DIAG, family);
+}
+
 static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	int err;
@@ -220,8 +229,7 @@ static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
 		return -EINVAL;
 
 	if (sock_diag_handlers[req->sdiag_family] == NULL)
-		request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				NETLINK_SOCK_DIAG, req->sdiag_family);
+		sock_diag_request_module(req->sdiag_family);
 
 	mutex_lock(&sock_diag_table_mutex);
 	hndl = sock_diag_handlers[req->sdiag_family];
@@ -247,8 +255,7 @@ static int sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	case TCPDIAG_GETSOCK:
 	case DCCPDIAG_GETSOCK:
 		if (inet_rcv_compat == NULL)
-			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-					NETLINK_SOCK_DIAG, AF_INET);
+			sock_diag_request_module(AF_INET);
 
 		mutex_lock(&sock_diag_table_mutex);
 		if (inet_rcv_compat != NULL)
@@ -281,14 +288,12 @@ static int sock_diag_bind(struct net *net, int group)
 	case SKNLGRP_INET_TCP_DESTROY:
 	case SKNLGRP_INET_UDP_DESTROY:
 		if (!sock_diag_handlers[AF_INET])
-			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				       NETLINK_SOCK_DIAG, AF_INET);
+			sock_diag_request_module(AF_INET);
 		break;
 	case SKNLGRP_INET6_TCP_DESTROY:
 	case SKNLGRP_INET6_UDP_DESTROY:
 		if (!sock_diag_handlers[AF_INET6])
-			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				       NETLINK_SOCK_DIAG, AF_INET);
+			sock_diag_request_module(AF_INET);
 		break;
 	}
 	return 0;
diff --git a/net/socket.c b/net/socket.c
index c729625..733d657 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2590,6 +2590,11 @@ void sock_unregister(int family)
 }
 EXPORT_SYMBOL(sock_unregister);
 
+bool sock_is_registered(int family)
+{
+	return family < NPROTO && rcu_access_pointer(net_families[family]);
+}
+
 static int __init sock_init(void)
 {
 	int err;
-- 
2.1.0

[PATCH net 2/2] inet_diag: request _diag module only when the proto has been registered

[Xin Long]

The patch 'sock_diag: request _diag module only when the family has
been registered' fixed a security issue for sock_diag, the same fix
is needed for inet_diag module when requesting _diag module.

Fixes: 305e1e969114 ("[INET]: Let inet_diag and friends autoload")
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/protocol.h | 1 +
 net/ipv4/inet_diag.c   | 3 ++-
 net/ipv4/protocol.c    | 6 ++++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net/protocol.h b/include/net/protocol.h
index 4fc75f7..bf0dcc2 100644
--- a/include/net/protocol.h
+++ b/include/net/protocol.h
@@ -103,6 +103,7 @@ extern struct inet6_protocol __rcu *inet6_protos[MAX_INET_PROTOS];
 
 int inet_add_protocol(const struct net_protocol *prot, unsigned char num);
 int inet_del_protocol(const struct net_protocol *prot, unsigned char num);
+bool inet_proto_is_added(unsigned char num);
 int inet_add_offload(const struct net_offload *prot, unsigned char num);
 int inet_del_offload(const struct net_offload *prot, unsigned char num);
 void inet_register_protosw(struct inet_protosw *p);
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index c9c35b6..1460031 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -27,6 +27,7 @@
 #include <net/inet_hashtables.h>
 #include <net/inet_timewait_sock.h>
 #include <net/inet6_hashtables.h>
+#include <net/protocol.h>
 #include <net/netlink.h>
 
 #include <linux/inet.h>
@@ -52,7 +53,7 @@ static DEFINE_MUTEX(inet_diag_table_mutex);
 
 static const struct inet_diag_handler *inet_diag_lock_handler(int proto)
 {
-	if (!inet_diag_table[proto])
+	if (!inet_diag_table[proto] && inet_proto_is_added(proto))
 		request_module("net-pf-%d-proto-%d-type-%d-%d", PF_NETLINK,
 			       NETLINK_SOCK_DIAG, AF_INET, proto);
 
diff --git a/net/ipv4/protocol.c b/net/ipv4/protocol.c
index 32a691b..183386a 100644
--- a/net/ipv4/protocol.c
+++ b/net/ipv4/protocol.c
@@ -77,3 +77,9 @@ int inet_del_offload(const struct net_offload *prot, unsigned char protocol)
 	return ret;
 }
 EXPORT_SYMBOL(inet_del_offload);
+
+bool inet_proto_is_added(unsigned char protocol)
+{
+	return !!rcu_access_pointer(inet_protos[protocol]);
+}
+EXPORT_SYMBOL(inet_proto_is_added);
-- 
2.1.0

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Thu, 19 Oct 2017 15:32:23 +0800

> This patch is to void the potential security issue that the family
> or protocol modules are autoloaded when requesting _diag module by
> not requesting _diag module if the family or protocol is not added
> or registered in sock_diag and inet_diag.
> 
> As the repost of the patch '[PATCH net] sock_diag: request _diag
> module only when the family or proto has been registered', this
> patchset fixes the compiling errors when INET is not set, and
> also split into two patches to make it clear to review.

This makes no sense to me.

Any user can just open a socket() in the appropriate protocol
family to cause the module to be loaded.

If someone wants modules to not be loaded, block them using
traditional module loading infrastructure mechanisms.  Or
don't load the module at all.

Sorry I am not applying this.

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[Eric Dumazet]

On Fri, Oct 20, 2017 at 11:06 PM, Xin Long <lucien.xin@gmail.com> wrote:
>
>
> On Sat, Oct 21, 2017 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
>>
>> From: Xin Long <lucien.xin@gmail.com>
>> Date: Thu, 19 Oct 2017 15:32:23 +0800
>>
>> > This patch is to void the potential security issue that the family
>> > or protocol modules are autoloaded when requesting _diag module by
>> > not requesting _diag module if the family or protocol is not added
>> > or registered in sock_diag and inet_diag.
>> >
>> > As the repost of the patch '[PATCH net] sock_diag: request _diag
>> > module only when the family or proto has been registered', this
>> > patchset fixes the compiling errors when INET is not set, and
>> > also split into two patches to make it clear to review.
>>
>> This makes no sense to me.
>>
>> Any user can just open a socket() in the appropriate protocol
>> family to cause the module to be loaded.
>>
>> If someone wants modules to not be loaded, block them using
>> traditional module loading infrastructure mechanisms.  Or
>> don't load the module at all.
>>
>> Sorry I am not applying this.
>
> Hi David,
>
> I'm still thinking it's not good after 'ss', sctp, dccp,
> af_packet ... are just loaded, in which case, no one actually
> open any socket with these family or proto.
>
> I talked with Marcelo before, one scenario as he said:
>
> Imagine a customer generates a sosreport on their system, and
> with that, it loads sctp module. From then on, if their firewall
> doesn't block incoming packets for sctp, they may be prone to some
> remotely triggerable issue on sctp code, without even actually using
> sctp.

For that reason, we have disabled autoloading of SCTP.
( removing the
 MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132");
 MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132");
)
root must modprobe the module before it is accessible.

However inet_diag is a way to have the module loaded anyway.

This is why I like your patch Xin.

David is only saying that your patch alone is not enough to prevent a
user to use socket() to autoload SCTP.

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[Xin Long]

On Sat, Oct 21, 2017 at 2:18 PM, Eric Dumazet <edumazet@google.com> wrote:
> On Fri, Oct 20, 2017 at 11:06 PM, Xin Long <lucien.xin@gmail.com> wrote:
>>
>>
>> On Sat, Oct 21, 2017 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
>>>
>>> From: Xin Long <lucien.xin@gmail.com>
>>> Date: Thu, 19 Oct 2017 15:32:23 +0800
>>>
>>> > This patch is to void the potential security issue that the family
>>> > or protocol modules are autoloaded when requesting _diag module by
>>> > not requesting _diag module if the family or protocol is not added
>>> > or registered in sock_diag and inet_diag.
>>> >
>>> > As the repost of the patch '[PATCH net] sock_diag: request _diag
>>> > module only when the family or proto has been registered', this
>>> > patchset fixes the compiling errors when INET is not set, and
>>> > also split into two patches to make it clear to review.
>>>
>>> This makes no sense to me.
>>>
>>> Any user can just open a socket() in the appropriate protocol
>>> family to cause the module to be loaded.
>>>
>>> If someone wants modules to not be loaded, block them using
>>> traditional module loading infrastructure mechanisms.  Or
>>> don't load the module at all.
>>>
>>> Sorry I am not applying this.
>>
>> Hi David,
>>
>> I'm still thinking it's not good after 'ss', sctp, dccp,
>> af_packet ... are just loaded, in which case, no one actually
>> open any socket with these family or proto.
>>
>> I talked with Marcelo before, one scenario as he said:
>>
>> Imagine a customer generates a sosreport on their system, and
>> with that, it loads sctp module. From then on, if their firewall
>> doesn't block incoming packets for sctp, they may be prone to some
>> remotely triggerable issue on sctp code, without even actually using
>> sctp.
>
> For that reason, we have disabled autoloading of SCTP.
> ( removing the
>  MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132");
>  MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132");
> )
> root must modprobe the module before it is accessible.
>
> However inet_diag is a way to have the module loaded anyway.
>
> This is why I like your patch Xin.
>
> David is only saying that your patch alone is not enough to prevent a
> user to use socket() to autoload SCTP.
Using  socket() to autoload SCTP should be fine, cause users would
use SCTP, no ?

"ss" doesn't mean users intend to use SCTP, "ss" may make users
not aware that SCTP module would be loaded, unlike socket(SCTP).

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[Eric Dumazet]

On Sat, 2017-10-21 at 14:51 +0800, Xin Long wrote:
> On Sat, Oct 21, 2017 at 2:18 PM, Eric Dumazet <edumazet@google.com> wrote:
> > On Fri, Oct 20, 2017 at 11:06 PM, Xin Long <lucien.xin@gmail.com> wrote:
> >>
> >>
> >> On Sat, Oct 21, 2017 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
> >>>
> >>> From: Xin Long <lucien.xin@gmail.com>
> >>> Date: Thu, 19 Oct 2017 15:32:23 +0800
> >>>
> >>> > This patch is to void the potential security issue that the family
> >>> > or protocol modules are autoloaded when requesting _diag module by
> >>> > not requesting _diag module if the family or protocol is not added
> >>> > or registered in sock_diag and inet_diag.
> >>> >
> >>> > As the repost of the patch '[PATCH net] sock_diag: request _diag
> >>> > module only when the family or proto has been registered', this
> >>> > patchset fixes the compiling errors when INET is not set, and
> >>> > also split into two patches to make it clear to review.
> >>>
> >>> This makes no sense to me.
> >>>
> >>> Any user can just open a socket() in the appropriate protocol
> >>> family to cause the module to be loaded.
> >>>
> >>> If someone wants modules to not be loaded, block them using
> >>> traditional module loading infrastructure mechanisms.  Or
> >>> don't load the module at all.
> >>>
> >>> Sorry I am not applying this.
> >>
> >> Hi David,
> >>
> >> I'm still thinking it's not good after 'ss', sctp, dccp,
> >> af_packet ... are just loaded, in which case, no one actually
> >> open any socket with these family or proto.
> >>
> >> I talked with Marcelo before, one scenario as he said:
> >>
> >> Imagine a customer generates a sosreport on their system, and
> >> with that, it loads sctp module. From then on, if their firewall
> >> doesn't block incoming packets for sctp, they may be prone to some
> >> remotely triggerable issue on sctp code, without even actually using
> >> sctp.
> >
> > For that reason, we have disabled autoloading of SCTP.
> > ( removing the
> >  MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132");
> >  MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132");
> > )
> > root must modprobe the module before it is accessible.
> >
> > However inet_diag is a way to have the module loaded anyway.
> >
> > This is why I like your patch Xin.
> >
> > David is only saying that your patch alone is not enough to prevent a
> > user to use socket() to autoload SCTP.
> Using  socket() to autoload SCTP should be fine, cause users would
> use SCTP, no ?
> 
> "ss" doesn't mean users intend to use SCTP, "ss" may make users
> not aware that SCTP module would be loaded, unlike socket(SCTP).

Your changelog mentions a security issue.

How have you prevented user using socket() to bypass your 'security'
feature ?

If you have not yet, this security claim is simply false.

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[Xin Long]

On Sat, Oct 21, 2017 at 3:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2017-10-21 at 14:51 +0800, Xin Long wrote:
>> On Sat, Oct 21, 2017 at 2:18 PM, Eric Dumazet <edumazet@google.com> wrote:
>> > On Fri, Oct 20, 2017 at 11:06 PM, Xin Long <lucien.xin@gmail.com> wrote:
>> >>
>> >>
>> >> On Sat, Oct 21, 2017 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
>> >>>
>> >>> From: Xin Long <lucien.xin@gmail.com>
>> >>> Date: Thu, 19 Oct 2017 15:32:23 +0800
>> >>>
>> >>> > This patch is to void the potential security issue that the family
>> >>> > or protocol modules are autoloaded when requesting _diag module by
>> >>> > not requesting _diag module if the family or protocol is not added
>> >>> > or registered in sock_diag and inet_diag.
>> >>> >
>> >>> > As the repost of the patch '[PATCH net] sock_diag: request _diag
>> >>> > module only when the family or proto has been registered', this
>> >>> > patchset fixes the compiling errors when INET is not set, and
>> >>> > also split into two patches to make it clear to review.
>> >>>
>> >>> This makes no sense to me.
>> >>>
>> >>> Any user can just open a socket() in the appropriate protocol
>> >>> family to cause the module to be loaded.
>> >>>
>> >>> If someone wants modules to not be loaded, block them using
>> >>> traditional module loading infrastructure mechanisms.  Or
>> >>> don't load the module at all.
>> >>>
>> >>> Sorry I am not applying this.
>> >>
>> >> Hi David,
>> >>
>> >> I'm still thinking it's not good after 'ss', sctp, dccp,
>> >> af_packet ... are just loaded, in which case, no one actually
>> >> open any socket with these family or proto.
>> >>
>> >> I talked with Marcelo before, one scenario as he said:
>> >>
>> >> Imagine a customer generates a sosreport on their system, and
>> >> with that, it loads sctp module. From then on, if their firewall
>> >> doesn't block incoming packets for sctp, they may be prone to some
>> >> remotely triggerable issue on sctp code, without even actually using
>> >> sctp.
>> >
>> > For that reason, we have disabled autoloading of SCTP.
>> > ( removing the
>> >  MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132");
>> >  MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132");
>> > )
>> > root must modprobe the module before it is accessible.
>> >
>> > However inet_diag is a way to have the module loaded anyway.
>> >
>> > This is why I like your patch Xin.
>> >
>> > David is only saying that your patch alone is not enough to prevent a
>> > user to use socket() to autoload SCTP.
>> Using  socket() to autoload SCTP should be fine, cause users would
>> use SCTP, no ?
>>
>> "ss" doesn't mean users intend to use SCTP, "ss" may make users
>> not aware that SCTP module would be loaded, unlike socket(SCTP).
>
> Your changelog mentions a security issue.
>
> How have you prevented user using socket() to bypass your 'security'
> feature ?
I think the hook in __sock_create():

        err = security_socket_create(family, type, protocol, kern);
        if (err)
                return err;

could work for this, no ?

like, users add security rules to not allow to create SCTP socket
(namely, not allow sctp module to be loaded)

But, 'ss' could bypass it to load SCTP module.

In this way, can this patch be considered a security issue ?

>
> If you have not yet, this security claim is simply false.

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[Xin Long]

On Sat, Oct 21, 2017 at 4:45 PM, Xin Long <lucien.xin@gmail.com> wrote:
> On Sat, Oct 21, 2017 at 3:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Sat, 2017-10-21 at 14:51 +0800, Xin Long wrote:
>>> On Sat, Oct 21, 2017 at 2:18 PM, Eric Dumazet <edumazet@google.com> wrote:
>>> > On Fri, Oct 20, 2017 at 11:06 PM, Xin Long <lucien.xin@gmail.com> wrote:
>>> >>
>>> >>
>>> >> On Sat, Oct 21, 2017 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
>>> >>>
>>> >>> From: Xin Long <lucien.xin@gmail.com>
>>> >>> Date: Thu, 19 Oct 2017 15:32:23 +0800
>>> >>>
>>> >>> > This patch is to void the potential security issue that the family
>>> >>> > or protocol modules are autoloaded when requesting _diag module by
>>> >>> > not requesting _diag module if the family or protocol is not added
>>> >>> > or registered in sock_diag and inet_diag.
>>> >>> >
>>> >>> > As the repost of the patch '[PATCH net] sock_diag: request _diag
>>> >>> > module only when the family or proto has been registered', this
>>> >>> > patchset fixes the compiling errors when INET is not set, and
>>> >>> > also split into two patches to make it clear to review.
>>> >>>
>>> >>> This makes no sense to me.
>>> >>>
>>> >>> Any user can just open a socket() in the appropriate protocol
>>> >>> family to cause the module to be loaded.
>>> >>>
>>> >>> If someone wants modules to not be loaded, block them using
>>> >>> traditional module loading infrastructure mechanisms.  Or
>>> >>> don't load the module at all.
>>> >>>
>>> >>> Sorry I am not applying this.
>>> >>
>>> >> Hi David,
>>> >>
>>> >> I'm still thinking it's not good after 'ss', sctp, dccp,
>>> >> af_packet ... are just loaded, in which case, no one actually
>>> >> open any socket with these family or proto.
>>> >>
>>> >> I talked with Marcelo before, one scenario as he said:
>>> >>
>>> >> Imagine a customer generates a sosreport on their system, and
>>> >> with that, it loads sctp module. From then on, if their firewall
>>> >> doesn't block incoming packets for sctp, they may be prone to some
>>> >> remotely triggerable issue on sctp code, without even actually using
>>> >> sctp.
>>> >
>>> > For that reason, we have disabled autoloading of SCTP.
>>> > ( removing the
>>> >  MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132");
>>> >  MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132");
>>> > )
>>> > root must modprobe the module before it is accessible.
>>> >
>>> > However inet_diag is a way to have the module loaded anyway.
>>> >
>>> > This is why I like your patch Xin.
>>> >
>>> > David is only saying that your patch alone is not enough to prevent a
>>> > user to use socket() to autoload SCTP.
>>> Using  socket() to autoload SCTP should be fine, cause users would
>>> use SCTP, no ?
>>>
>>> "ss" doesn't mean users intend to use SCTP, "ss" may make users
>>> not aware that SCTP module would be loaded, unlike socket(SCTP).
>>
>> Your changelog mentions a security issue.
>>
>> How have you prevented user using socket() to bypass your 'security'
>> feature ?
> I think the hook in __sock_create():
>
>         err = security_socket_create(family, type, protocol, kern);
>         if (err)
>                 return err;
>
> could work for this, no ?
Sorry, Eric, this may not be an inappropriate example, after all
security_socket_create is not supposed to do these things.
thanks for your review.

Let's just see if David could accept the patches if I will
remove the "security claim" from changelog, considering
it as an improvement of sock diag.

David ?

>
> like, users add security rules to not allow to create SCTP socket
> (namely, not allow sctp module to be loaded)
>
> But, 'ss' could bypass it to load SCTP module.
>
> In this way, can this patch be considered a security issue ?
>
>>
>> If you have not yet, this security claim is simply false.

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Sat, 21 Oct 2017 14:06:27 +0800

> Imagine a customer generates a sosreport on their system, and
> with that, it loads sctp module. From then on, if their firewall
> doesn't block incoming packets for sctp, they may be prone to some
> remotely triggerable issue on sctp code, without even actually using
> sctp.

Like I said, if the protocol is so unsafe, block it in the
modules.conf file.

Block all "I don't use this" protocols in netfilter.

Otherwise, like I said, any user on their system can open a socket of
the indicated protocol.

There are many options.

Furthermore, "ss" should not signal an error because the protocol
module happens to not be open yet and as I understand it this is what
your patch does since it chooses to not load the module in this
situation.

Re: [PATCH net 0/2] net: diag: fix a potential security issue

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Sat, 21 Oct 2017 17:45:09 +0800

> Let's just see if David could accept the patches if I will
> remove the "security claim" from changelog, considering
> it as an improvement of sock diag.
> 
> David ?

No I won't.  See my other response.

Use modules.conf or netfilter rules to block "scary networking
protocols" if you are so paranoid about this happening.

Thank you.