From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id EA7AAD807AB for ; Wed, 24 Jan 2024 10:39:44 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=TyNNVle2Q/r9iSZ9GgKELGfeN2IiMMFiVm7uAVbEt54=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706092783; v=1; b=W8y1L2+cCUz2bmK4JTJ9zlHHdroI9hHU2pH+rxj4DpC8OenVO8UoicquNIPHP3pWFSK8/Y1g 0/wBf1SJYSnsMpnnhPRZf2F1VQ+Kk/9rGYOl8zgQrRGllNDWYL4p+yDfq8oHhp27iHll4jpLS+E jZ2OY1DnDBA5x4zzpdws3Qyk= X-Received: by 127.0.0.2 with SMTP id TG73YY7687511xUKat9J6kXN; Wed, 24 Jan 2024 02:39:43 -0800 X-Received: from mail-vk1-f172.google.com (mail-vk1-f172.google.com [209.85.221.172]) by mx.groups.io with SMTP id smtpd.web10.19611.1706092782954656537 for ; Wed, 24 Jan 2024 02:39:43 -0800 X-Received: by mail-vk1-f172.google.com with SMTP id 71dfb90a1353d-4affeacaff9so988843e0c.3 for ; Wed, 24 Jan 2024 02:39:42 -0800 (PST) X-Gm-Message-State: J5qdPDpwYd87hUbkHSqtAPmVx7686176AA= X-Google-Smtp-Source: AGHT+IHqmGTfqWg4jXrTRqKnJl8KZDLpfX5i2ScTYyKTJg2xpnZ4OPQE4McFwNhF6v2xIOmT2u6Nxa38xWDZ6Il9iII= X-Received: by 2002:ac5:c2c6:0:b0:4bd:4356:40bc with SMTP id i6-20020ac5c2c6000000b004bd435640bcmr1386012vkk.5.1706092781718; Wed, 24 Jan 2024 02:39:41 -0800 (PST) MIME-Version: 1.0 References: <53d416e177a558d2d95e0fe7b2aecb52e6745bf3.1706062164.git.doug.edk2@gmail.com> In-Reply-To: <53d416e177a558d2d95e0fe7b2aecb52e6745bf3.1706062164.git.doug.edk2@gmail.com> From: "Pedro Falcato" Date: Wed, 24 Jan 2024 10:39:30 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH 07/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch To: devel@edk2.groups.io, dougflick@microsoft.com Cc: Saloni Kasbekar , Zachary Clark-williams , "Doug Flick [MSFT]" Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pedro.falcato@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=W8y1L2+c; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none) On Wed, Jan 24, 2024 at 5:20=E2=80=AFAM Doug Flick via groups.io wrote: > > From: Doug Flick > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D4537 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D4538 > > SECURITY PATCH - Patch > > TCBZ4537 > CVE-2023-45232 > CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H > CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop') > > TCBZ4538 > CVE-2023-45233 > CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H > CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop') > > Cc: Saloni Kasbekar > Cc: Zachary Clark-williams > > Signed-off-by: Doug Flick [MSFT] > --- > NetworkPkg/Ip6Dxe/Ip6Option.h | 89 +++++++++++++++++++++++++++++++++++ > NetworkPkg/Ip6Dxe/Ip6Option.c | 76 +++++++++++++++++++++++++----- > 2 files changed, 154 insertions(+), 11 deletions(-) > > diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.h b/NetworkPkg/Ip6Dxe/Ip6Option.= h > index bd8e223c8a67..5d786073ebcb 100644 > --- a/NetworkPkg/Ip6Dxe/Ip6Option.h > +++ b/NetworkPkg/Ip6Dxe/Ip6Option.h > @@ -12,6 +12,95 @@ > > #define IP6_FRAGMENT_OFFSET_MASK (~0x3) > > +// > +// Per RFC8200 Section 4.2 > +// > +// Two of the currently-defined extension headers -- the Hop-by-Hop > +// Options header and the Destination Options header -- carry a variab= le > +// number of type-length-value (TLV) encoded "options", of the followi= ng > +// format: > +// > +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - - > +// | Option Type | Opt Data Len | Option Data > +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - - > +// > +// Option Type 8-bit identifier of the type of option. > +// > +// Opt Data Len 8-bit unsigned integer. Length of the Opti= on > +// Data field of this option, in octets. > +// > +// Option Data Variable-length field. Option-Type-specifi= c > +// data. > +// Why isn't this just a struct Ipv6Option { UINT8 OptionType; UINT8 OptionLength; UINT8 OptionData[]; }; ? You'd skip all of the weird obfuscated math below. > +#define IP6_SIZE_OF_OPT_TYPE (sizeof(UINT8)) > +#define IP6_SIZE_OF_OPT_LEN (sizeof(UINT8)) sizeof(UINT8) can just be replaced by 1 > +#define IP6_COMBINED_SIZE_OF_OPT_TAG_AND_LEN (IP6_SIZE_OF_OPT_TYPE + IP= 6_SIZE_OF_OPT_LEN) > +#define IP6_OFFSET_OF_OPT_LEN(a) (a + IP6_SIZE_OF_OPT_TYPE) > +STATIC_ASSERT ( > + IP6_OFFSET_OF_OPT_LEN (0) =3D=3D 1, > + "The Length field should be 1 octet (8 bits) past the start of the opt= ion" > + ); > + > +#define IP6_NEXT_OPTION_OFFSET(offset, length) (offset + IP6_COMBINED_S= IZE_OF_OPT_TAG_AND_LEN + length) > +STATIC_ASSERT ( > + IP6_NEXT_OPTION_OFFSET (0, 0) =3D=3D 2, > + "The next option is minimally the combined size of the option tag and = length" > + ); > + > +// > +// For more information see RFC 8200, Section 4.3, 4.4, and 4.6 > +// > +// This example format is from section 4.6 > +// This does not apply to fragment headers > +// > +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > +// | Next Header | Hdr Ext Len | | > +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + > +// | | > +// . . > +// . Header-Specific Data . > +// . . > +// | | > +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > +// > +// Next Header 8-bit selector. Identifies the type of > +// header immediately following the extension > +// header. Uses the same values as the IPv4 > +// Protocol field [IANA-PN]. > +// > +// Hdr Ext Len 8-bit unsigned integer. Length of the > +// Destination Options header in 8-octet unit= s, > +// not including the first 8 octets. > + > +// > +// These defines apply to the following: > +// 1. Hop by Hop > +// 2. Routing > +// 3. Destination > +// Same comment as above (why is this not a struct?) > +#define IP6_SIZE_OF_EXT_NEXT_HDR (sizeof(UINT8)) > +#define IP6_SIZE_OF_HDR_EXT_LEN (sizeof(UINT8)) Same for sizeof(UINT8) here. > + > +#define IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN (IP6_SIZE_OF_EXT_NEXT_HDR= + IP6_SIZE_OF_HDR_EXT_LEN) > +STATIC_ASSERT ( > + IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN =3D=3D 2, > + "The combined size of Next Header and Len is two 8 bit fields" > + ); > + > +// > +// The "+ 1" in this calculation is because of the "not including the fi= rst 8 octets" > +// part of the definition (meaning the value of 0 represents 64 bits) > +// > +#define IP6_HDR_EXT_LEN(a) (((UINT16)(UINT8)(a) + 1) * 8) This expression is remarkably hard to understand correctly, operator precedence is hard :( . > + > +// This is the maxmimum length permissible by a extension header typo: maximum > +// Length is UINT8 of 8 octets not including the first 8 octets > +#define IP6_MAX_EXT_DATA_LENGTH (IP6_HDR_EXT_LEN (MAX_UINT8) - IP6_COMB= INED_SIZE_OF_NEXT_HDR_AND_LEN) > +STATIC_ASSERT ( > + IP6_MAX_EXT_DATA_LENGTH =3D=3D 2046, > + "Maximum data length is ((MAX_UINT8 + 1) * 8) - 2" > + ); > + > typedef struct _IP6_FRAGMENT_HEADER { > UINT8 NextHeader; > UINT8 Reserved; > diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.= c > index 8718d5d8756a..144f8d34dead 100644 > --- a/NetworkPkg/Ip6Dxe/Ip6Option.c > +++ b/NetworkPkg/Ip6Dxe/Ip6Option.c > @@ -17,7 +17,8 @@ > @param[in] IpSb The IP6 service data. > @param[in] Packet The to be validated packet. > @param[in] Option The first byte of the option. > - @param[in] OptionLen The length of the whole option. > + @param[in] OptionLen The length of all options, expressed in = byte length of octets. > + Maximum length is 2046 bytes or ((n + 1)= * 8) - 2 where n is 255. > @param[in] Pointer Identifies the octet offset within > the invoking packet where the error was = detected. > > @@ -31,12 +32,33 @@ Ip6IsOptionValid ( > IN IP6_SERVICE *IpSb, > IN NET_BUF *Packet, > IN UINT8 *Option, > - IN UINT8 OptionLen, > + IN UINT16 OptionLen, > IN UINT32 Pointer > ) > { > - UINT8 Offset; > - UINT8 OptionType; > + UINT16 Offset; > + UINT8 OptionType; > + UINT8 OptDataLen; > + > + if (Option =3D=3D NULL) { > + ASSERT (Option !=3D NULL); > + return FALSE; > + } > + > + if ((OptionLen <=3D 0) || (OptionLen > IP6_MAX_EXT_DATA_LENGTH)) { OptionLen is unsigned hence < 0 is not a valid check --=20 Pedro -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114274): https://edk2.groups.io/g/devel/message/114274 Mute This Topic: https://groups.io/mt/103926738/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-