lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANn89iJ1qoP9PpJZVcatvdtRX4SqUrKrWDfer1hdid+gxYQXhA@mail.gmail.com> Date: Wed, 29 May 2019 08:44:27 -0700 From: Eric Dumazet <edumazet@...gle.com> To: Yang Xiao <92siuyang@...il.com> Cc: David Miller <davem@...emloft.net>, Alexey Kuznetsov <kuznet@....inr.ac.ru>, Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>, netdev <netdev@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] ipv4: tcp_input: fix stack out of bounds when parsing TCP options. On Wed, May 29, 2019 at 8:11 AM Yang Xiao <92siuyang@...il.com> wrote: > > Indeed, condition opsize < 2 and opsize > length can deduce that length >= 2. > However, before the condition (if opsize < 2), there may be one-byte > out-of-bound access in line 12. > I'm not sure whether I have put it very clearly. Maybe I should have been clear about the 320 bytes we have at the end of skb->head This is the struct skb_shared_info So reading one byte, 'out-of-bound' here is harmless. Whatever value is read, we will return early without ever looking at a following byte. > > On Wed, May 29, 2019 at 10:20 PM Eric Dumazet <edumazet@...gle.com> wrote: > > > > On Wed, May 29, 2019 at 1:10 AM Young Xiao <92siuyang@...il.com> wrote: > > > > > > The TCP option parsing routines in tcp_parse_options function could > > > read one byte out of the buffer of the TCP options. > > > > > > 1 while (length > 0) { > > > 2 int opcode = *ptr++; > > > 3 int opsize; > > > 4 > > > 5 switch (opcode) { > > > 6 case TCPOPT_EOL: > > > 7 return; > > > 8 case TCPOPT_NOP: /* Ref: RFC 793 section 3.1 */ > > > 9 length--; > > > 10 continue; > > > 11 default: > > > 12 opsize = *ptr++; //out of bound access > > > > > > If length = 1, then there is an access in line2. > > > And another access is occurred in line 12. > > > This would lead to out-of-bound access. > > > > > > Therefore, in the patch we check that the available data length is > > > larger enough to pase both TCP option code and size. > > > > > > Signed-off-by: Young Xiao <92siuyang@...il.com> > > > --- > > > net/ipv4/tcp_input.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > > > index 20f6fac..9775825 100644 > > > --- a/net/ipv4/tcp_input.c > > > +++ b/net/ipv4/tcp_input.c > > > @@ -3791,6 +3791,8 @@ void tcp_parse_options(const struct net *net, > > > length--; > > > continue; > > > default: > > > + if (length < 2) > > > + return; > > > opsize = *ptr++; > > > if (opsize < 2) /* "silly options" */ > > > return; > > > > In practice we are good, since we have at least 320 bytes of room there, > > and the test done later catches silly options. > > > > if (opsize < 2) /* "silly options" */ > > return; > > if (opsize > length) /* remember, opsize >= 2 here */ > > return; /* don't parse partial options */ > > > > I guess adding yet another conditional will make this code obviously > > correct for all eyes > > and various tools. > > > > Thanks. > > > > Signed-off-by: Eric Dumazet <edumazet@...gle.com> > > > > -- > Best regards! > > Young > -----------------------------------------------------------
Powered by blists - more mailing lists