From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1AB76C433F5 for ; Thu, 26 May 2022 17:24:37 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 255A58430F; Thu, 26 May 2022 19:24:35 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=amarulasolutions.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=amarulasolutions.com header.i=@amarulasolutions.com header.b="PC1pappn"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2B1A1842C1; Thu, 26 May 2022 19:24:33 +0200 (CEST) Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 731B78430F for ; Thu, 26 May 2022 19:24:27 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=amarulasolutions.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=michael@amarulasolutions.com Received: by mail-pf1-x434.google.com with SMTP id c14so2236804pfn.2 for ; Thu, 26 May 2022 10:24:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amarulasolutions.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=92btLykCqhBGXCLKYzOEgCeLCBHoG6MBTdOrluDhn/c=; b=PC1pappnI1RM/t1rzQCM3rK+4B4tS5K/gkkVrRyNjtOFymg/4YY+niNRj92d9IrgG6 sEMty9sUsCO3fugRJDpcmRqX4mIF1tXRwc4jBGyZ0VAaWHN2jRj89lofKRsRqIpL1Xnd uDInLTfgM72BDSjQUai3dupjCA1zytd2seKOI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=92btLykCqhBGXCLKYzOEgCeLCBHoG6MBTdOrluDhn/c=; b=BKZHdq+C5gJinZuGr/7yz6r6LF2uXSvD2RTlDrVskvAK+/dUhEFxL/oUsG7QpoPPiN lLTlcpWB5nOBdIDH1zBxNJ/xXYplVh2I/+4Kfx/3XPI5L3GZhGW65idHnbRAt00lxqrT Bqv0r+dK0iPbMvo2BI7/SWZ85NJ55zqmSIYsP92pxiibcVaJeuIwGOmNL4jEoSwI1KHI UFQ9JQ0THi+H/2JdeU1UHJ/yWXDmoOxKPuRvylqDBmc/KHaYgwAOgBeRXZoptMiI8BX4 ge394ErBUA+6hjh1DKuIGgw0G72I6/mPuKnKFqiUEW/7C2dDgzqA7bOxADqE4v9Qq7c7 wTNw== X-Gm-Message-State: AOAM530b9VsCLAdIwZmqb29eGlXbRAbs1oGVdJrS8C+xPV1IFmZhM/z7 UVLCuU9SQjLHZ48RaB1XytS4xRbsNBdwPaJekSCwfA== X-Google-Smtp-Source: ABdhPJwG7K67VZA1BfABnj52TDydPt5ImHCl8ro3jjYoLuLy7enb6gJ735ttzh3ReqyRH3J4OOsBZ9PJk3GYEO8rjME= X-Received: by 2002:a05:6a00:1897:b0:518:8bc7:9a82 with SMTP id x23-20020a056a00189700b005188bc79a82mr25913053pfh.26.1653585865564; Thu, 26 May 2022 10:24:25 -0700 (PDT) MIME-Version: 1.0 References: <20220526163648.1269502-1-festevam@gmail.com> In-Reply-To: <20220526163648.1269502-1-festevam@gmail.com> From: Michael Nazzareno Trimarchi Date: Thu, 26 May 2022 19:24:12 +0200 Message-ID: Subject: Re: [PATCH v3] net: Check for the minimum IP header total length To: Fabio Estevam Cc: Ramon Fried , Joe Hershberger , U-Boot-Denx , nicolas.bidron@nccgroup.com, mbrugger@suse.com, Tom Rini , Fabio Estevam Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.39 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi Il gio 26 mag 2022, 18:36 Fabio Estevam ha scritto: > From: Fabio Estevam > > Nicolas Bidron and Nicolas Guigo reported the two bugs below: > > " > ----------BUG 1---------- > > In compiled versions of U-Boot that define CONFIG_IP_DEFRAG, a value of > `ip->ip_len` (IP packet header's Total Length) higher than `IP_HDR_SIZE` > and strictly lower than `IP_HDR_SIZE+8` will lead to a value for `len` > comprised between `0` and `7`. This will ultimately result in a > truncated division by `8` resulting value of `0` forcing the hole > metadata and fragment to point to the same location. The subsequent > memcopy will overwrite the hole metadata with the fragment data. Through > a second fragment, this can be exploited to write to an arbitrary offset > controlled by that overwritten hole metadata value. > > This bug is only exploitable locally as it requires crafting two packets > the first of which would most likely be dropped through routing due to > its unexpectedly low Total Length. However, this bug can potentially be > exploited to root linux based embedded devices locally. > > ```C > static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int > *lenp) > { > static uchar pkt_buff[IP_PKTSIZE] __aligned(PKTALIGN); > static u16 first_hole, total_len; > struct hole *payload, *thisfrag, *h, *newh; > struct ip_udp_hdr *localip = (struct ip_udp_hdr *)pkt_buff; > uchar *indata = (uchar *)ip; > int offset8, start, len, done = 0; > u16 ip_off = ntohs(ip->ip_off); > > /* payload starts after IP header, this fragment is in there */ > payload = (struct hole *)(pkt_buff + IP_HDR_SIZE); > offset8 = (ip_off & IP_OFFS); > thisfrag = payload + offset8; > start = offset8 * 8; > len = ntohs(ip->ip_len) - IP_HDR_SIZE; > ``` > > The last line of the previous excerpt from `u-boot/net/net.c` shows how > the attacker can control the value of `len` to be strictly lower than > `8` by issuing a packet with `ip_len` between `21` and `27` > (`IP_HDR_SIZE` has a value of `20`). > > Also note that `offset8` here is `0` which leads to `thisfrag = payload`. > > ```C > } else if (h >= thisfrag) { > /* overlaps with initial part of the hole: move this hole */ > newh = thisfrag + (len / 8); > *newh = *h; > h = newh; > if (h->next_hole) > payload[h->next_hole].prev_hole = (h - payload); > if (h->prev_hole) > payload[h->prev_hole].next_hole = (h - payload); > else > first_hole = (h - payload); > > } else { > ``` > > Lower down the same function, execution reaches the above code path. > Here, `len / 8` evaluates to `0` leading to `newh = thisfrag`. Also note > that `first_hole` here is `0` since `h` and `payload` point to the same > location. > > ```C > /* finally copy this fragment and possibly return whole packet */ > memcpy((uchar *)thisfrag, indata + IP_HDR_SIZE, len); > ``` > > Finally, in the above excerpt the `memcpy` overwrites the hole metadata > since `thisfrag` and `h` both point to the same location. The hole > metadata is effectively overwritten with arbitrary data from the > fragmented IP packet data. If `len` was crafted to be `6`, `last_byte`, > `next_hole`, and `prev_hole` of the `first_hole` can be controlled by > the attacker. > > Finally the arbitrary offset write occurs through a second fragment that > only needs to be crafted to write data in the hole pointed to by the > previously controlled hole metadata (`next_hole`) from the first packet. > > ### Recommendation > > Handle cases where `len` is strictly lower than 8 by preventing the > overwrite of the hole metadata during the memcpy of the fragment. This > could be achieved by either: > * Moving the location where the hole metadata is stored when `len` is > lower than `8`. > * Or outright rejecting fragmented IP datagram with a Total Length > (`ip_len`) lower than 28 bytes which is the minimum valid fragmented IP > datagram size (as defined as the minimum fragment of 8 octets in the IP > Specification Document: > [RFC791](https://datatracker.ietf.org/doc/html/rfc791) page 25). > > ----------BUG 2---------- > > In compiled versions of U-Boot that define CONFIG_IP_DEFRAG, a value of > `ip->ip_len` (IP packet header's Total Length) lower than `IP_HDR_SIZE` > will lead to a negative value for `len` which will ultimately result in > a buffer overflow during the subsequent `memcpy` that uses `len` as it's > `count` parameter. > > This bug is only exploitable on local ethernet as it requires crafting > an invalid packet to include an unexpected `ip_len` value in the IP UDP > header that's lower than the minimum accepted Total Length of a packet > (21 as defined in the IP Specification Document: > [RFC791](https://datatracker.ietf.org/doc/html/rfc791)). Such packet > would in all likelihood be dropped while being routed to its final > destination through most routing equipment and as such requires the > attacker to be in a local position in order to be exploited. > > ```C > static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int > *lenp) > { > static uchar pkt_buff[IP_PKTSIZE] __aligned(PKTALIGN); > static u16 first_hole, total_len; > struct hole *payload, *thisfrag, *h, *newh; > struct ip_udp_hdr *localip = (struct ip_udp_hdr *)pkt_buff; > uchar *indata = (uchar *)ip; > int offset8, start, len, done = 0; > u16 ip_off = ntohs(ip->ip_off); > > /* payload starts after IP header, this fragment is in there */ > payload = (struct hole *)(pkt_buff + IP_HDR_SIZE); > offset8 = (ip_off & IP_OFFS); > thisfrag = payload + offset8; > start = offset8 * 8; > len = ntohs(ip->ip_len) - IP_HDR_SIZE; > ``` > > The last line of the previous excerpt from `u-boot/net/net.c` shows > where the underflow to a negative `len` value occurs if `ip_len` is set > to a value strictly lower than 20 (`IP_HDR_SIZE` being 20). Also note > that in the above excerpt the `pkt_buff` buffer has a size of > `CONFIG_NET_MAXDEFRAG` which defaults to 16 KB but can range from 1KB to > 64 KB depending on configurations. > > ```C > /* finally copy this fragment and possibly return whole packet */ > memcpy((uchar *)thisfrag, indata + IP_HDR_SIZE, len); > ``` > > In the above excerpt the `memcpy` overflows the destination by > attempting to make a copy of nearly 4 gigabytes in a buffer that's > designed to hold `CONFIG_NET_MAXDEFRAG` bytes at most which leads to a DoS. > > ### Recommendation > > Stop processing of the packet if `ip_len` is lower than 21 (as defined > by the minimum length of a data carrying datagram in the IP > Specification Document: > [RFC791](https://datatracker.ietf.org/doc/html/rfc791) page 34)." > > Add a check for ip_len being less than 21 and stop processing the packet > in this case. > > Such a check covers the two reported bugs. > > Reported-by: Nicolas Bidron > Suggested-by: Nicolas Bidron > Signed-off-by: Fabio Estevam > --- > Changes since v2: > - Check ip_len against 21 (IP_MIN_TOTAL_LENGTH). > > include/net.h | 2 ++ > net/net.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/include/net.h b/include/net.h > index 675bf4171b17..8933b976fa07 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -391,6 +391,8 @@ struct ip_hdr { > > #define IP_HDR_SIZE (sizeof(struct ip_hdr)) > > +#define IP_MIN_TOTAL_LENGTH (IP_HDR_SIZE + 1) > + > Not needed /* > * Internet Protocol (IP) + UDP header. > */ > diff --git a/net/net.c b/net/net.c > index 034a5d6e67c0..81905f631592 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -907,6 +907,9 @@ static struct ip_udp_hdr *__net_defragment(struct > ip_udp_hdr *ip, int *lenp) > int offset8, start, len, done = 0; > u16 ip_off = ntohs(ip->ip_off); > > + if (ip->ip_len < IP_MIN_TOTAL_LENGTH) > + return NULL; > + > <= Michael > /* payload starts after IP header, this fragment is in there */ > payload = (struct hole *)(pkt_buff + IP_HDR_SIZE); > offset8 = (ip_off & IP_OFFS); > -- > 2.25.1 > >