* [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
@ 2017-10-31 16:14 Kees Cook
2017-10-31 17:31 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Kees Cook @ 2017-10-31 16:14 UTC (permalink / raw)
To: David S. Miller
Cc: Alexander Potapenko, Kostya Serebryany, Andrey Konovalov,
Eric Dumazet, netdev, linux-kernel, security
Some protocols do not correctly wipe the contents of the on-stack
struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
kernel stack contents to userspace. This wipes it unconditionally before
per-protocol handlers run.
Note that leaks like this are mitigated by building with
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
Reported-by: Alexander Potapenko <glider@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
net/socket.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/socket.c b/net/socket.c
index c729625eb5d3..34183f4fbdf8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
struct sockaddr __user *uaddr;
int __user *uaddr_len = COMPAT_NAMELEN(msg);
+ memset(&addr, 0, sizeof(addr));
msg_sys->msg_name = &addr;
if (MSG_CMSG_COMPAT & flags)
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook @ 2017-10-31 17:31 ` Eric Dumazet 2017-11-01 12:48 ` Eric W. Biederman 2017-10-31 17:35 ` Ben Hutchings 2017-11-01 6:49 ` Willy Tarreau 2 siblings, 1 reply; 11+ messages in thread From: Eric Dumazet @ 2017-10-31 17:31 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Alexander Potapenko, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, netdev, linux-kernel, security On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: > Some protocols do not correctly wipe the contents of the on-stack > struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak > kernel stack contents to userspace. This wipes it unconditionally before > per-protocol handlers run. > > Note that leaks like this are mitigated by building with > CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y > > Reported-by: Alexander Potapenko <glider@google.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > net/socket.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/socket.c b/net/socket.c > index c729625eb5d3..34183f4fbdf8 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, > struct sockaddr __user *uaddr; > int __user *uaddr_len = COMPAT_NAMELEN(msg); > > + memset(&addr, 0, sizeof(addr)); > msg_sys->msg_name = &addr; > This kind of patch comes every year. Standard answer is : We fix the buggy protocol, we do not make everything slower just because we are lazy. struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it. Also memset() is using long word stores, so next 4-byte or 2-byte stores on same location hit a performance problem on x86. By adding all these defensive programming, we would give strong incentives to bypass the kernel for networking. That would be bad. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-10-31 17:31 ` Eric Dumazet @ 2017-11-01 12:48 ` Eric W. Biederman 2017-11-01 18:23 ` Kees Cook 2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook 0 siblings, 2 replies; 11+ messages in thread From: Eric W. Biederman @ 2017-11-01 12:48 UTC (permalink / raw) To: Eric Dumazet Cc: Kees Cook, David S. Miller, Alexander Potapenko, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, netdev, linux-kernel, security Eric Dumazet <eric.dumazet@gmail.com> writes: > On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: >> Some protocols do not correctly wipe the contents of the on-stack >> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak >> kernel stack contents to userspace. This wipes it unconditionally before >> per-protocol handlers run. >> >> Note that leaks like this are mitigated by building with >> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y >> >> Reported-by: Alexander Potapenko <glider@google.com> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: netdev@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> net/socket.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/socket.c b/net/socket.c >> index c729625eb5d3..34183f4fbdf8 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, >> struct sockaddr __user *uaddr; >> int __user *uaddr_len = COMPAT_NAMELEN(msg); >> >> + memset(&addr, 0, sizeof(addr)); >> msg_sys->msg_name = &addr; >> > > This kind of patch comes every year. > > Standard answer is : We fix the buggy protocol, we do not make > everything slower just because we are lazy. > > struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it. > > Also memset() is using long word stores, so next 4-byte or 2-byte stores > on same location hit a performance problem on x86. > > By adding all these defensive programming, we would give strong > incentives to bypass the kernel for networking. That would be bad. In this case it looks like the root cause is something in sctp not filling in the ipv6 sin6_scope_id. Which is not only a leak but a correctness bug. I ran the reproducer test program and while none of the leak checkers are telling me anything I have gotten as far as seeing that the returned length is correct and sometimes nonsense. Hmm. At a quick look it looks like all that is necessary is to do this: diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c index 51c488769590..6301913d0516 100644 --- a/net/sctp/ipv6.c +++ b/net/sctp/ipv6.c @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, addr->v6.sin6_flowinfo = 0; addr->v6.sin6_port = sh->source; addr->v6.sin6_addr = ipv6_hdr(skb)->saddr; - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) { + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb); - } + else + addr->v6.sin6_scope_id = 0; } *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr); Eric ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-11-01 12:48 ` Eric W. Biederman @ 2017-11-01 18:23 ` Kees Cook 2017-11-15 8:22 ` Alexander Potapenko 2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook 1 sibling, 1 reply; 11+ messages in thread From: Kees Cook @ 2017-11-01 18:23 UTC (permalink / raw) To: Eric W. Biederman, Alexander Potapenko Cc: Eric Dumazet, David S. Miller, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, Network Development, LKML, security On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Eric Dumazet <eric.dumazet@gmail.com> writes: > >> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: >>> Some protocols do not correctly wipe the contents of the on-stack >>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak >>> kernel stack contents to userspace. This wipes it unconditionally before >>> per-protocol handlers run. >>> >>> Note that leaks like this are mitigated by building with >>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y >>> >>> Reported-by: Alexander Potapenko <glider@google.com> >>> Cc: "David S. Miller" <davem@davemloft.net> >>> Cc: netdev@vger.kernel.org >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> net/socket.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/net/socket.c b/net/socket.c >>> index c729625eb5d3..34183f4fbdf8 100644 >>> --- a/net/socket.c >>> +++ b/net/socket.c >>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, >>> struct sockaddr __user *uaddr; >>> int __user *uaddr_len = COMPAT_NAMELEN(msg); >>> >>> + memset(&addr, 0, sizeof(addr)); >>> msg_sys->msg_name = &addr; >>> >> >> This kind of patch comes every year. >> >> Standard answer is : We fix the buggy protocol, we do not make >> everything slower just because we are lazy. >> >> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it. >> >> Also memset() is using long word stores, so next 4-byte or 2-byte stores >> on same location hit a performance problem on x86. >> >> By adding all these defensive programming, we would give strong >> incentives to bypass the kernel for networking. That would be bad. > > In this case it looks like the root cause is something in sctp > not filling in the ipv6 sin6_scope_id. > > Which is not only a leak but a correctness bug. > > I ran the reproducer test program and while none of the leak checkers > are telling me anything I have gotten as far as seeing that the returned > length is correct and sometimes nonsense. > > Hmm. > > At a quick look it looks like all that is necessary is to do this: > > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c > index 51c488769590..6301913d0516 100644 > --- a/net/sctp/ipv6.c > +++ b/net/sctp/ipv6.c > @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, > addr->v6.sin6_flowinfo = 0; > addr->v6.sin6_port = sh->source; > addr->v6.sin6_addr = ipv6_hdr(skb)->saddr; > - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) { > + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) > addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb); > - } > + else > + addr->v6.sin6_scope_id = 0; > } > > *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr); > > Eric > Thanks for digging into this Eric! Alexander, can you confirm this fixes it for you when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not enabled? -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-11-01 18:23 ` Kees Cook @ 2017-11-15 8:22 ` Alexander Potapenko 2017-11-16 4:17 ` [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname Eric W. Biederman 0 siblings, 1 reply; 11+ messages in thread From: Alexander Potapenko @ 2017-11-15 8:22 UTC (permalink / raw) To: Kees Cook Cc: Eric W. Biederman, Eric Dumazet, David S. Miller, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, Network Development, LKML, security On Wed, Nov 1, 2017 at 7:23 PM, Kees Cook <keescook@chromium.org> wrote: > On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: >> Eric Dumazet <eric.dumazet@gmail.com> writes: >> >>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: >>>> Some protocols do not correctly wipe the contents of the on-stack >>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak >>>> kernel stack contents to userspace. This wipes it unconditionally before >>>> per-protocol handlers run. >>>> >>>> Note that leaks like this are mitigated by building with >>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y >>>> >>>> Reported-by: Alexander Potapenko <glider@google.com> >>>> Cc: "David S. Miller" <davem@davemloft.net> >>>> Cc: netdev@vger.kernel.org >>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>> --- >>>> net/socket.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/net/socket.c b/net/socket.c >>>> index c729625eb5d3..34183f4fbdf8 100644 >>>> --- a/net/socket.c >>>> +++ b/net/socket.c >>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, >>>> struct sockaddr __user *uaddr; >>>> int __user *uaddr_len = COMPAT_NAMELEN(msg); >>>> >>>> + memset(&addr, 0, sizeof(addr)); >>>> msg_sys->msg_name = &addr; >>>> >>> >>> This kind of patch comes every year. >>> >>> Standard answer is : We fix the buggy protocol, we do not make >>> everything slower just because we are lazy. >>> >>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it. >>> >>> Also memset() is using long word stores, so next 4-byte or 2-byte stores >>> on same location hit a performance problem on x86. >>> >>> By adding all these defensive programming, we would give strong >>> incentives to bypass the kernel for networking. That would be bad. >> >> In this case it looks like the root cause is something in sctp >> not filling in the ipv6 sin6_scope_id. >> >> Which is not only a leak but a correctness bug. >> >> I ran the reproducer test program and while none of the leak checkers >> are telling me anything I have gotten as far as seeing that the returned >> length is correct and sometimes nonsense. >> >> Hmm. >> >> At a quick look it looks like all that is necessary is to do this: >> >> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c >> index 51c488769590..6301913d0516 100644 >> --- a/net/sctp/ipv6.c >> +++ b/net/sctp/ipv6.c >> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, >> addr->v6.sin6_flowinfo = 0; >> addr->v6.sin6_port = sh->source; >> addr->v6.sin6_addr = ipv6_hdr(skb)->saddr; >> - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) { >> + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) >> addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb); >> - } >> + else >> + addr->v6.sin6_scope_id = 0; >> } >> >> *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr); >> >> Eric >> > > Thanks for digging into this Eric! Alexander, can you confirm this > fixes it for you when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not > enabled? Sorry, I accidentally missed this patch. Yes, I confirm it fixes the problem with CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL disabled. > -Kees > > -- > Kees Cook > Pixel Security -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname 2017-11-15 8:22 ` Alexander Potapenko @ 2017-11-16 4:17 ` Eric W. Biederman 2017-11-16 14:00 ` David Miller 0 siblings, 1 reply; 11+ messages in thread From: Eric W. Biederman @ 2017-11-16 4:17 UTC (permalink / raw) To: David S. Miller Cc: Kees Cook, Eric Dumazet, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, Network Development, LKML, security, Alexander Potapenko, linux-sctp, Neil Horman, Vlad Yasevich Alexandar Potapenko while testing the kernel with KMSAN and syzkaller discovered that in some configurations sctp would leak 4 bytes of kernel stack. Working with his reproducer I discovered that those 4 bytes that are leaked is the scope id of an ipv6 address returned by recvmsg. With a little code inspection and a shrewd guess I discovered that sctp_inet6_skb_msgname only initializes the scope_id field for link local ipv6 addresses to the interface index the link local address pertains to instead of initializing the scope_id field for all ipv6 addresses. That is almost reasonable as scope_id's are meaniningful only for link local addresses. Set the scope_id in all other cases to 0 which is not a valid interface index to make it clear there is nothing useful in the scope_id field. There should be no danger of breaking userspace as the stack leak guaranteed that previously meaningless random data was being returned. Cc: stable@vger.kernel.org Fixes: 372f525b495c ("SCTP: Resync with LKSCTP tree.") History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git Reported-by: Alexander Potapenko <glider@google.com> Tested-by: Alexander Potapenko <glider@google.com> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- net/sctp/ipv6.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c index a6dfa86c0201..3b18085e3b10 100644 --- a/net/sctp/ipv6.c +++ b/net/sctp/ipv6.c @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, addr->v6.sin6_flowinfo = 0; addr->v6.sin6_port = sh->source; addr->v6.sin6_addr = ipv6_hdr(skb)->saddr; - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) { + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb); - } + else + addr->v6.sin6_scope_id = 0; } *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr); -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname 2017-11-16 4:17 ` [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname Eric W. Biederman @ 2017-11-16 14:00 ` David Miller 0 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2017-11-16 14:00 UTC (permalink / raw) To: ebiederm Cc: keescook, eric.dumazet, kcc, andreyknvl, edumazet, netdev, linux-kernel, security, glider, linux-sctp, nhorman, vyasevich From: ebiederm@xmission.com (Eric W. Biederman) Date: Wed, 15 Nov 2017 22:17:48 -0600 > > Alexandar Potapenko while testing the kernel with KMSAN and syzkaller > discovered that in some configurations sctp would leak 4 bytes of > kernel stack. > > Working with his reproducer I discovered that those 4 bytes that > are leaked is the scope id of an ipv6 address returned by recvmsg. > > With a little code inspection and a shrewd guess I discovered that > sctp_inet6_skb_msgname only initializes the scope_id field for link > local ipv6 addresses to the interface index the link local address > pertains to instead of initializing the scope_id field for all ipv6 > addresses. > > That is almost reasonable as scope_id's are meaniningful only for link > local addresses. Set the scope_id in all other cases to 0 which is > not a valid interface index to make it clear there is nothing useful > in the scope_id field. > > There should be no danger of breaking userspace as the stack leak > guaranteed that previously meaningless random data was being returned. > > Fixes: 372f525b495c ("SCTP: Resync with LKSCTP tree.") > History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git > Reported-by: Alexander Potapenko <glider@google.com> > Tested-by: Alexander Potapenko <glider@google.com> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Applied and queued up for -stable, thanks Eric. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-11-01 12:48 ` Eric W. Biederman 2017-11-01 18:23 ` Kees Cook @ 2017-11-15 2:13 ` Kees Cook 2017-11-15 18:37 ` Eric W. Biederman 1 sibling, 1 reply; 11+ messages in thread From: Kees Cook @ 2017-11-15 2:13 UTC (permalink / raw) To: Eric W. Biederman Cc: Eric Dumazet, David S. Miller, Alexander Potapenko, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, Network Development, LKML, security On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Eric Dumazet <eric.dumazet@gmail.com> writes: > >> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: >>> Some protocols do not correctly wipe the contents of the on-stack >>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak >>> kernel stack contents to userspace. This wipes it unconditionally before >>> per-protocol handlers run. >>> >>> Note that leaks like this are mitigated by building with >>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y >>> >>> Reported-by: Alexander Potapenko <glider@google.com> >>> Cc: "David S. Miller" <davem@davemloft.net> >>> Cc: netdev@vger.kernel.org >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> net/socket.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/net/socket.c b/net/socket.c >>> index c729625eb5d3..34183f4fbdf8 100644 >>> --- a/net/socket.c >>> +++ b/net/socket.c >>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, >>> struct sockaddr __user *uaddr; >>> int __user *uaddr_len = COMPAT_NAMELEN(msg); >>> >>> + memset(&addr, 0, sizeof(addr)); >>> msg_sys->msg_name = &addr; >>> >> >> This kind of patch comes every year. >> >> Standard answer is : We fix the buggy protocol, we do not make >> everything slower just because we are lazy. >> >> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it. >> >> Also memset() is using long word stores, so next 4-byte or 2-byte stores >> on same location hit a performance problem on x86. >> >> By adding all these defensive programming, we would give strong >> incentives to bypass the kernel for networking. That would be bad. > > In this case it looks like the root cause is something in sctp > not filling in the ipv6 sin6_scope_id. > > Which is not only a leak but a correctness bug. > > I ran the reproducer test program and while none of the leak checkers > are telling me anything I have gotten as far as seeing that the returned > length is correct and sometimes nonsense. > > Hmm. > > At a quick look it looks like all that is necessary is to do this: > > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c > index 51c488769590..6301913d0516 100644 > --- a/net/sctp/ipv6.c > +++ b/net/sctp/ipv6.c > @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, > addr->v6.sin6_flowinfo = 0; > addr->v6.sin6_port = sh->source; > addr->v6.sin6_addr = ipv6_hdr(skb)->saddr; > - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) { > + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) > addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb); > - } > + else > + addr->v6.sin6_scope_id = 0; > } > > *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr); > It looks like this never landed anywhere? Eric, are you able to resend this as a full patch? Thanks! -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook @ 2017-11-15 18:37 ` Eric W. Biederman 0 siblings, 0 replies; 11+ messages in thread From: Eric W. Biederman @ 2017-11-15 18:37 UTC (permalink / raw) To: Kees Cook Cc: Eric Dumazet, David S. Miller, Alexander Potapenko, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, Network Development, LKML, security Kees Cook <keescook@chromium.org> writes: > On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: >> Eric Dumazet <eric.dumazet@gmail.com> writes: >> >>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: >>>> Some protocols do not correctly wipe the contents of the on-stack >>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak >>>> kernel stack contents to userspace. This wipes it unconditionally before >>>> per-protocol handlers run. >>>> >>>> Note that leaks like this are mitigated by building with >>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y >>>> >>>> Reported-by: Alexander Potapenko <glider@google.com> >>>> Cc: "David S. Miller" <davem@davemloft.net> >>>> Cc: netdev@vger.kernel.org >>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>> --- >>>> net/socket.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/net/socket.c b/net/socket.c >>>> index c729625eb5d3..34183f4fbdf8 100644 >>>> --- a/net/socket.c >>>> +++ b/net/socket.c >>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, >>>> struct sockaddr __user *uaddr; >>>> int __user *uaddr_len = COMPAT_NAMELEN(msg); >>>> >>>> + memset(&addr, 0, sizeof(addr)); >>>> msg_sys->msg_name = &addr; >>>> >>> >>> This kind of patch comes every year. >>> >>> Standard answer is : We fix the buggy protocol, we do not make >>> everything slower just because we are lazy. >>> >>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it. >>> >>> Also memset() is using long word stores, so next 4-byte or 2-byte stores >>> on same location hit a performance problem on x86. >>> >>> By adding all these defensive programming, we would give strong >>> incentives to bypass the kernel for networking. That would be bad. >> >> In this case it looks like the root cause is something in sctp >> not filling in the ipv6 sin6_scope_id. >> >> Which is not only a leak but a correctness bug. >> >> I ran the reproducer test program and while none of the leak checkers >> are telling me anything I have gotten as far as seeing that the returned >> length is correct and sometimes nonsense. >> >> Hmm. >> >> At a quick look it looks like all that is necessary is to do this: >> >> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c >> index 51c488769590..6301913d0516 100644 >> --- a/net/sctp/ipv6.c >> +++ b/net/sctp/ipv6.c >> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, >> addr->v6.sin6_flowinfo = 0; >> addr->v6.sin6_port = sh->source; >> addr->v6.sin6_addr = ipv6_hdr(skb)->saddr; >> - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) { >> + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) >> addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb); >> - } >> + else >> + addr->v6.sin6_scope_id = 0; >> } >> >> *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr); >> > > It looks like this never landed anywhere? Eric, are you able to resend > this as a full patch? I will take a look. I have not conducted a thorough review to make certain that is everything. I was hoping someone else would pick that change up and run with it. However the change seems obviously correct as is, so I don't have any problem sending just this bit. Eric ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook 2017-10-31 17:31 ` Eric Dumazet @ 2017-10-31 17:35 ` Ben Hutchings 2017-11-01 6:49 ` Willy Tarreau 2 siblings, 0 replies; 11+ messages in thread From: Ben Hutchings @ 2017-10-31 17:35 UTC (permalink / raw) To: Kees Cook, David S. Miller Cc: Alexander Potapenko, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, netdev, linux-kernel, security [-- Attachment #1: Type: text/plain, Size: 1612 bytes --] On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: > Some protocols do not correctly wipe the contents of the on-stack > struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak > kernel stack contents to userspace. This wipes it unconditionally before > per-protocol handlers run. > > Note that leaks like this are mitigated by building with > CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y > > Reported-by: Alexander Potapenko <glider@google.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > net/socket.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/socket.c b/net/socket.c > index c729625eb5d3..34183f4fbdf8 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, > struct sockaddr __user *uaddr; > int __user *uaddr_len = COMPAT_NAMELEN(msg); > > + memset(&addr, 0, sizeof(addr)); That's a fairly large structure (128 bytes), most of which won't normally be used. We already initialise msg_namelen to 0 before calling the per-protocol handler, which means by default nothing leaks. Only cases where msg_namelen is set but msg_name[] is not initialised up to that length are a problem. I would have thought they were not too hard to find and fix. Ben. > msg_sys->msg_name = &addr; > > if (MSG_CMSG_COMPAT & flags) > -- > 2.7.4 > > -- Ben Hutchings It is a miracle that curiosity survives formal education. - Albert Einstein [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage 2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook 2017-10-31 17:31 ` Eric Dumazet 2017-10-31 17:35 ` Ben Hutchings @ 2017-11-01 6:49 ` Willy Tarreau 2 siblings, 0 replies; 11+ messages in thread From: Willy Tarreau @ 2017-11-01 6:49 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Alexander Potapenko, Kostya Serebryany, Andrey Konovalov, Eric Dumazet, netdev, linux-kernel, security On Tue, Oct 31, 2017 at 09:14:45AM -0700, Kees Cook wrote: > diff --git a/net/socket.c b/net/socket.c > index c729625eb5d3..34183f4fbdf8 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, > struct sockaddr __user *uaddr; > int __user *uaddr_len = COMPAT_NAMELEN(msg); > > + memset(&addr, 0, sizeof(addr)); > msg_sys->msg_name = &addr; Isn't this going to cause a performance hit in the fast path ? Just checking, I have not read the whole code with the patch in its context. Willy ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-11-16 14:01 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook 2017-10-31 17:31 ` Eric Dumazet 2017-11-01 12:48 ` Eric W. Biederman 2017-11-01 18:23 ` Kees Cook 2017-11-15 8:22 ` Alexander Potapenko 2017-11-16 4:17 ` [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname Eric W. Biederman 2017-11-16 14:00 ` David Miller 2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook 2017-11-15 18:37 ` Eric W. Biederman 2017-10-31 17:35 ` Ben Hutchings 2017-11-01 6:49 ` Willy Tarreau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).