netdev - Re: [PATCH net 3/3] sch_cake: Fix out of bounds when parsing TCP options

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87a6nxvlfx.fsf@toke.dk>
Date:   Thu, 10 Jun 2021 16:33:22 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Maxim Mikityanskiy <maximmi@...dia.com>,
        Florian Westphal <fw@...len.de>
Cc:     Young Xiao <92siuyang@...il.com>, netdev@...r.kernel.org,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Matthieu Baerts <matthieu.baerts@...sares.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...filter.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Patrick McHardy <kaber@...sh.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Peter Krystad <peter.krystad@...ux.intel.com>
Subject: Re: [PATCH net 3/3] sch_cake: Fix out of bounds when parsing TCP
 options

Maxim Mikityanskiy <maximmi@...dia.com> writes:

> On 2021-06-10 00:51, Toke Høiland-Jørgensen wrote:
>> Maxim Mikityanskiy <maximmi@...dia.com> writes:
>> 
>>> The TCP option parser in cake qdisc (cake_get_tcpopt and
>>> cake_tcph_may_drop) could read one byte out of bounds. When the length
>>> is 1, the execution flow gets into the loop, reads one byte of the
>>> opcode, and if the opcode is neither TCPOPT_EOL nor TCPOPT_NOP, it reads
>>> one more byte, which exceeds the length of 1.
>>>
>>> This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack
>>> out of bounds when parsing TCP options.").
>>>
>>> Cc: Young Xiao <92siuyang@...il.com>
>>> Fixes: 8b7138814f29 ("sch_cake: Add optional ACK filter")
>>> Signed-off-by: Maxim Mikityanskiy <maximmi@...dia.com>
>> 
>> Thanks for fixing this!
>> 
>> Acked-by: Toke Høiland-Jørgensen <toke@...e.dk>
>> 
>
> Could you also review whether Florian's comment on patch 1 is relevant 
> to this patch too? I have concerns about cake_get_tcphdr, which returns 
> `skb_header_pointer(skb, offset, min(__tcp_hdrlen(tcph), bufsize), 
> buf)`. Although I don't see a way for it to get out of bounds (it will 
> read garbage instead of TCP header in the worst case), such code doesn't 
> look robust.
>
> It's not possible for it to get out of bounds, because there is a call 
> to skb_header_pointer above with sizeof(_tcph), which ensures that the 
> SKB has at least 20 bytes after the beginning of the TCP header, which 
> means that the second skb_header_pointer will either point to SKB (where 
> we have at least 20 bytes) or to buf (which is allocated by the caller, 
> so the caller shouldn't overflow its own buffer).
>
> On the other hand, parsing garbage doesn't look like a valid behavior 
> compared to dropping/ignoring/whatever-cake-does-with-bad-packets, so we 
> may want to handle it, for example:
>
>           return skb_header_pointer(skb, offset,
> -                                  min(__tcp_hdrlen(tcph), bufsize), buf);
> +                                  min(max(sizeof(struct tcphdr), 
> __tcp_hdrlen(tcph)), bufsize), buf);
>
> What do you think? Or did I just miss some early check for doff?

No, I think your analysis is correct: It won't lead to any out-of-bounds
reads, but I suppose we could end up trying to parse garbage. However,
if we do get a packet that sets doff to an invalid value, and we try to
parse it, we're essentially parsing garbage anyway. So I think the fix
should rather be something like:

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 7d37638ee1c7..d312d75ab698 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -943,7 +943,7 @@ static struct tcphdr *cake_get_tcphdr(const struct sk_buff *skb,
        }
 
        tcph = skb_header_pointer(skb, offset, sizeof(_tcph), &_tcph);
-       if (!tcph)
+       if (!tcph || tcph->doff < 5)
                return NULL;
 
        return skb_header_pointer(skb, offset,

> (I realize it's egress path and the packets produced by the system 
> itself are unlikely to have bad doff, but it's not impossible, for 
> example, with AF_PACKET, BPF hooks in tc, etc.)

Most CAKE deployments primarily handles forwarded packets, and I suppose
malformed TCP packets could make it through the forwarding path as
well...

-Toke