* [PATCH net 0/2] net: diag: fix a potential security issue
@ 2017-10-19 7:32 Xin Long
2017-10-19 7:32 ` [PATCH net 1/2] sock_diag: request _diag module only when the family has been registered Xin Long
2017-10-21 1:27 ` [PATCH net 0/2] net: diag: fix a potential security issue David Miller
0 siblings, 2 replies; 11+ messages in thread
From: Xin Long @ 2017-10-19 7:32 UTC (permalink / raw)
To: network dev; +Cc: davem, Eric Dumazet, Marcelo Ricardo Leitner, Sabrina Dubroca
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH net 1/2] sock_diag: request _diag module only when the family has been registered 2017-10-19 7:32 [PATCH net 0/2] net: diag: fix a potential security issue Xin Long @ 2017-10-19 7:32 ` Xin Long 2017-10-19 7:32 ` [PATCH net 2/2] inet_diag: request _diag module only when the proto " Xin Long 2017-10-21 1:27 ` [PATCH net 0/2] net: diag: fix a potential security issue David Miller 1 sibling, 1 reply; 11+ messages in thread From: Xin Long @ 2017-10-19 7:32 UTC (permalink / raw) To: network dev; +Cc: davem, Eric Dumazet, Marcelo Ricardo Leitner, Sabrina Dubroca 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net 2/2] inet_diag: request _diag module only when the proto has been registered 2017-10-19 7:32 ` [PATCH net 1/2] sock_diag: request _diag module only when the family has been registered Xin Long @ 2017-10-19 7:32 ` Xin Long 0 siblings, 0 replies; 11+ messages in thread From: Xin Long @ 2017-10-19 7:32 UTC (permalink / raw) To: network dev; +Cc: davem, Eric Dumazet, Marcelo Ricardo Leitner, Sabrina Dubroca 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue 2017-10-19 7:32 [PATCH net 0/2] net: diag: fix a potential security issue Xin Long 2017-10-19 7:32 ` [PATCH net 1/2] sock_diag: request _diag module only when the family has been registered Xin Long @ 2017-10-21 1:27 ` David Miller [not found] ` <CADvbK_fWmmC3ggpoT--Pxk3GxZ8Gq_rbdFGTuXk-BuTHTO=eXw@mail.gmail.com> 1 sibling, 1 reply; 11+ messages in thread From: David Miller @ 2017-10-21 1:27 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, edumazet, marcelo.leitner, sd 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CADvbK_fWmmC3ggpoT--Pxk3GxZ8Gq_rbdFGTuXk-BuTHTO=eXw@mail.gmail.com>]
* Re: [PATCH net 0/2] net: diag: fix a potential security issue [not found] ` <CADvbK_fWmmC3ggpoT--Pxk3GxZ8Gq_rbdFGTuXk-BuTHTO=eXw@mail.gmail.com> @ 2017-10-21 6:18 ` Eric Dumazet 2017-10-21 6:51 ` Xin Long 2017-10-21 11:14 ` David Miller 1 sibling, 1 reply; 11+ messages in thread From: Eric Dumazet @ 2017-10-21 6:18 UTC (permalink / raw) To: Xin Long Cc: David Miller, network dev, Marcelo Ricardo Leitner, Sabrina Dubroca 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue 2017-10-21 6:18 ` Eric Dumazet @ 2017-10-21 6:51 ` Xin Long 2017-10-21 7:45 ` Eric Dumazet 0 siblings, 1 reply; 11+ messages in thread From: Xin Long @ 2017-10-21 6:51 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, network dev, Marcelo Ricardo Leitner, Sabrina Dubroca 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). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue 2017-10-21 6:51 ` Xin Long @ 2017-10-21 7:45 ` Eric Dumazet 2017-10-21 8:45 ` Xin Long 0 siblings, 1 reply; 11+ messages in thread From: Eric Dumazet @ 2017-10-21 7:45 UTC (permalink / raw) To: Xin Long Cc: Eric Dumazet, David Miller, network dev, Marcelo Ricardo Leitner, Sabrina Dubroca 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue 2017-10-21 7:45 ` Eric Dumazet @ 2017-10-21 8:45 ` Xin Long 2017-10-21 9:45 ` Xin Long 0 siblings, 1 reply; 11+ messages in thread From: Xin Long @ 2017-10-21 8:45 UTC (permalink / raw) To: Eric Dumazet Cc: Eric Dumazet, David Miller, network dev, Marcelo Ricardo Leitner, Sabrina Dubroca 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue 2017-10-21 8:45 ` Xin Long @ 2017-10-21 9:45 ` Xin Long 2017-10-21 11:16 ` David Miller 0 siblings, 1 reply; 11+ messages in thread From: Xin Long @ 2017-10-21 9:45 UTC (permalink / raw) To: Eric Dumazet Cc: Eric Dumazet, David Miller, network dev, Marcelo Ricardo Leitner, Sabrina Dubroca 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue 2017-10-21 9:45 ` Xin Long @ 2017-10-21 11:16 ` David Miller 0 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2017-10-21 11:16 UTC (permalink / raw) To: lucien.xin; +Cc: eric.dumazet, edumazet, netdev, marcelo.leitner, sd 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] net: diag: fix a potential security issue [not found] ` <CADvbK_fWmmC3ggpoT--Pxk3GxZ8Gq_rbdFGTuXk-BuTHTO=eXw@mail.gmail.com> 2017-10-21 6:18 ` Eric Dumazet @ 2017-10-21 11:14 ` David Miller 1 sibling, 0 replies; 11+ messages in thread From: David Miller @ 2017-10-21 11:14 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, edumazet, marcelo.leitner, sd 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-10-21 11:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 7:32 [PATCH net 0/2] net: diag: fix a potential security issue Xin Long
2017-10-19 7:32 ` [PATCH net 1/2] sock_diag: request _diag module only when the family has been registered Xin Long
2017-10-19 7:32 ` [PATCH net 2/2] inet_diag: request _diag module only when the proto " Xin Long
2017-10-21 1:27 ` [PATCH net 0/2] net: diag: fix a potential security issue David Miller
[not found] ` <CADvbK_fWmmC3ggpoT--Pxk3GxZ8Gq_rbdFGTuXk-BuTHTO=eXw@mail.gmail.com>
2017-10-21 6:18 ` Eric Dumazet
2017-10-21 6:51 ` Xin Long
2017-10-21 7:45 ` Eric Dumazet
2017-10-21 8:45 ` Xin Long
2017-10-21 9:45 ` Xin Long
2017-10-21 11:16 ` David Miller
2017-10-21 11:14 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.