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 D3A0EAC1BEB for ; Thu, 25 Jan 2024 23:06:37 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=BkmoiSCYLVLDIUOgQVwPOkEbjmgswil/GkejUsvTyn0=; c=relaxed/simple; d=groups.io; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Transfer-Encoding; s=20140610; t=1706223996; v=1; b=rEKxwHbRcu189KaN7w0++fgtVxj8blfzrvloOQRjHTfkj0znNP7CTNFL26hs9IrIHM1LzQAd UXHv+/rwgMineTuFtwsZkPvDRSkZ3GcBF4kEqTetbgSdQy1SMSNIFFmRNs1bq+IiSBqhAsI4TfU uF8EOfAknQnL+9dgcvLiVV2g= X-Received: by 127.0.0.2 with SMTP id Ur3sYY7687511xOClV6pINwI; Thu, 25 Jan 2024 15:06:36 -0800 X-Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) by mx.groups.io with SMTP id smtpd.web11.784.1706223995043224946 for ; Thu, 25 Jan 2024 15:06:35 -0800 X-Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-1d7858a469aso16590785ad.2 for ; Thu, 25 Jan 2024 15:06:34 -0800 (PST) X-Gm-Message-State: Se6asekL7CRBDtTWxpwKXfoVx7686176AA= X-Google-Smtp-Source: AGHT+IH3AZr5ObGDzXf7erG3nRJWroCFKITX8k9wXN+QGSmhF/xQZc4ZlTMul+qKIgchIbL0BboKZg== X-Received: by 2002:a17:902:d34c:b0:1d5:7690:3ae6 with SMTP id l12-20020a170902d34c00b001d576903ae6mr414898plk.17.1706223994126; Thu, 25 Jan 2024 15:06:34 -0800 (PST) X-Received: from localhost.localdomain ([24.17.138.83]) by smtp.gmail.com with ESMTPSA id jh1-20020a170903328100b001d752c4f180sm16779plb.94.2024.01.25.15.06.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jan 2024 15:06:33 -0800 (PST) From: "Doug Flick via groups.io" To: devel@edk2.groups.io Cc: Doug Flick , Saloni Kasbekar , Zachary Clark-williams , "Doug Flick [MSFT]" Subject: [edk2-devel] [PATCH v2 08/15] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch Date: Thu, 25 Jan 2024 13:54:50 -0800 Message-ID: In-Reply-To: References: MIME-Version: 1.0 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,dougflick@microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: 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=rEKxwHbR; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io From: Doug Flick REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D4537 REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D4538 Bug Details: PixieFail Bug #4 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') Infinite loop when parsing unknown options in the Destination Options header PixieFail Bug #5 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') Infinite loop when parsing a PadN option in the Destination Options header Change Overview: Most importantly this change corrects the following incorrect math and cleans up the code. > // It is a PadN option > // > - Offset =3D (UINT8)(Offset + *(Option + Offset + 1) + 2); > + OptDataLen =3D ((EFI_IP6_OPTION *)(Option + Offset))->Length; > + Offset =3D IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); > case Ip6OptionSkip: > - Offset =3D (UINT8)(Offset + *(Option + Offset + 1)); > OptDataLen =3D ((EFI_IP6_OPTION *)(Option + Offset))->Length; > Offset =3D IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); Additionally, this change also corrects incorrect math where the calling function was calculating the HDR EXT optionLen as a uint8 instead of a uint16 > - OptionLen =3D (UINT8)((*Option + 1) * 8 - 2); > + OptionLen =3D IP6_HDR_EXT_LEN (*Option) - IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN; Additionally this check adds additional logic to santize the incoming data Cc: Saloni Kasbekar Cc: Zachary Clark-williams Signed-off-by: Doug Flick [MSFT] --- NetworkPkg/Ip6Dxe/Ip6Nd.h | 35 ++++++++++++++++ NetworkPkg/Ip6Dxe/Ip6Option.h | 71 ++++++++++++++++++++++++++++++++ NetworkPkg/Ip6Dxe/Ip6Option.c | 76 ++++++++++++++++++++++++++++++----- 3 files changed, 171 insertions(+), 11 deletions(-) diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h index 860934a167eb..bf64e9114e13 100644 --- a/NetworkPkg/Ip6Dxe/Ip6Nd.h +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h @@ -56,13 +56,48 @@ VOID VOID *Context=0D );=0D =0D +//=0D +// Per RFC8200 Section 4.2=0D +//=0D +// Two of the currently-defined extension headers -- the Hop-by-Hop=0D +// Options header and the Destination Options header -- carry a variable= =0D +// number of type-length-value (TLV) encoded "options", of the following= =0D +// format:=0D +//=0D +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - -=0D +// | Option Type | Opt Data Len | Option Data=0D +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - -=0D +//=0D +// Option Type 8-bit identifier of the type of option.=0D +//=0D +// Opt Data Len 8-bit unsigned integer. Length of the Option= =0D +// Data field of this option, in octets.=0D +//=0D +// Option Data Variable-length field. Option-Type-specific= =0D +// data.=0D +//=0D typedef struct _IP6_OPTION_HEADER {=0D + ///=0D + /// identifier of the type of option.=0D + ///=0D UINT8 Type;=0D + ///=0D + /// Length of the Option Data field of this option, in octets.=0D + ///=0D UINT8 Length;=0D + ///=0D + /// Option-Type-specific data.=0D + ///=0D } IP6_OPTION_HEADER;=0D =0D STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) =3D=3D 2, "IP6_OPTION_HEADER is = expected to be exactly 2 bytes long.");=0D =0D +#define IP6_NEXT_OPTION_OFFSET(offset, length) (offset + sizeof(IP6_OPTIO= N_HEADER) + length)=0D +STATIC_ASSERT (=0D + IP6_NEXT_OPTION_OFFSET (0, 0) =3D=3D 2,=0D + "The next option is minimally the combined size of the option tag and le= ngth"=0D + );=0D +=0D typedef struct _IP6_ETHE_ADDR_OPTION {=0D UINT8 Type;=0D UINT8 Length;=0D diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.h b/NetworkPkg/Ip6Dxe/Ip6Option.h index bd8e223c8a67..fb07c28f5ad7 100644 --- a/NetworkPkg/Ip6Dxe/Ip6Option.h +++ b/NetworkPkg/Ip6Dxe/Ip6Option.h @@ -12,6 +12,77 @@ =0D #define IP6_FRAGMENT_OFFSET_MASK (~0x3)=0D =0D +//=0D +// For more information see RFC 8200, Section 4.3, 4.4, and 4.6=0D +//=0D +// This example format is from section 4.6=0D +// This does not apply to fragment headers=0D +//=0D +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D +// | Next Header | Hdr Ext Len | |=0D +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +=0D +// | |=0D +// . .=0D +// . Header-Specific Data .=0D +// . .=0D +// | |=0D +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D +//=0D +// Next Header 8-bit selector. Identifies the type of=0D +// header immediately following the extension=0D +// header. Uses the same values as the IPv4=0D +// Protocol field [IANA-PN].=0D +//=0D +// Hdr Ext Len 8-bit unsigned integer. Length of the=0D +// Destination Options header in 8-octet units,= =0D +// not including the first 8 octets.=0D +=0D +//=0D +// These defines apply to the following:=0D +// 1. Hop by Hop=0D +// 2. Routing=0D +// 3. Destination=0D +//=0D +typedef struct _IP6_EXT_HDR {=0D + ///=0D + /// The Next Header field identifies the type of header immediately=0D + ///=0D + UINT8 NextHeader;=0D + ///=0D + /// The Hdr Ext Len field specifies the length of the Hop-by-Hop Options= =0D + ///=0D + UINT8 HdrExtLen;=0D + ///=0D + /// Header-Specific Data=0D + ///=0D +} IP6_EXT_HDR;=0D +=0D +STATIC_ASSERT (=0D + sizeof (IP6_EXT_HDR) =3D=3D 2,=0D + "The combined size of Next Header and Len is two 8 bit fields"=0D + );=0D +=0D +//=0D +// IPv6 extension headers contain an 8-bit length field which describes th= e size of=0D +// the header. However, the length field only includes the size of the ext= ension=0D +// header options, not the size of the first 8 bytes of the header. Theref= ore, in=0D +// order to calculate the full size of the extension header, we add 1 (to = account=0D +// for the first 8 bytes omitted by the length field reporting) and then m= ultiply=0D +// by 8 (since the size is represented in 8-byte units).=0D +//=0D +// a is the length field of the extension header (UINT8)=0D +// The result may be up to 2046 octets (UINT16)=0D +//=0D +#define IP6_HDR_EXT_LEN(a) (((UINT16)((UINT8)(a)) + 1) * 8)=0D +=0D +// This is the maxmimum length permissible by a extension header=0D +// Length is UINT8 of 8 octets not including the first 8 octets=0D +#define IP6_MAX_EXT_DATA_LENGTH (IP6_HDR_EXT_LEN (MAX_UINT8) - sizeof(IP6= _EXT_HDR))=0D +STATIC_ASSERT (=0D + IP6_MAX_EXT_DATA_LENGTH =3D=3D 2046,=0D + "Maximum data length is ((MAX_UINT8 + 1) * 8) - 2"=0D + );=0D +=0D typedef struct _IP6_FRAGMENT_HEADER {=0D UINT8 NextHeader;=0D UINT8 Reserved;=0D diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c index 8718d5d8756a..fd97ce116f98 100644 --- a/NetworkPkg/Ip6Dxe/Ip6Option.c +++ b/NetworkPkg/Ip6Dxe/Ip6Option.c @@ -17,7 +17,8 @@ @param[in] IpSb The IP6 service data.=0D @param[in] Packet The to be validated packet.=0D @param[in] Option The first byte of the option.=0D - @param[in] OptionLen The length of the whole option.=0D + @param[in] OptionLen The length of all options, expressed in by= te length of octets.=0D + Maximum length is 2046 bytes or ((n + 1) *= 8) - 2 where n is 255.=0D @param[in] Pointer Identifies the octet offset within=0D the invoking packet where the error was de= tected.=0D =0D @@ -31,12 +32,33 @@ Ip6IsOptionValid ( IN IP6_SERVICE *IpSb,=0D IN NET_BUF *Packet,=0D IN UINT8 *Option,=0D - IN UINT8 OptionLen,=0D + IN UINT16 OptionLen,=0D IN UINT32 Pointer=0D )=0D {=0D - UINT8 Offset;=0D - UINT8 OptionType;=0D + UINT16 Offset;=0D + UINT8 OptionType;=0D + UINT8 OptDataLen;=0D +=0D + if (Option =3D=3D NULL) {=0D + ASSERT (Option !=3D NULL);=0D + return FALSE;=0D + }=0D +=0D + if ((OptionLen <=3D 0) || (OptionLen > IP6_MAX_EXT_DATA_LENGTH)) {=0D + ASSERT (OptionLen > 0 && OptionLen <=3D IP6_MAX_EXT_DATA_LENGTH);=0D + return FALSE;=0D + }=0D +=0D + if (Packet =3D=3D NULL) {=0D + ASSERT (Packet !=3D NULL);=0D + return FALSE;=0D + }=0D +=0D + if (IpSb =3D=3D NULL) {=0D + ASSERT (IpSb !=3D NULL);=0D + return FALSE;=0D + }=0D =0D Offset =3D 0;=0D =0D @@ -54,7 +76,8 @@ Ip6IsOptionValid ( //=0D // It is a PadN option=0D //=0D - Offset =3D (UINT8)(Offset + *(Option + Offset + 1) + 2);=0D + OptDataLen =3D ((IP6_OPTION_HEADER *)(Option + Offset))->Length;=0D + Offset =3D IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen);=0D break;=0D case Ip6OptionRouterAlert:=0D //=0D @@ -69,7 +92,8 @@ Ip6IsOptionValid ( //=0D switch (OptionType & Ip6OptionMask) {=0D case Ip6OptionSkip:=0D - Offset =3D (UINT8)(Offset + *(Option + Offset + 1));=0D + OptDataLen =3D ((IP6_OPTION_HEADER *)(Option + Offset))->Lengt= h;=0D + Offset =3D IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen);=0D break;=0D case Ip6OptionDiscard:=0D return FALSE;=0D @@ -308,7 +332,7 @@ Ip6IsExtsValid ( UINT32 Pointer;=0D UINT32 Offset;=0D UINT8 *Option;=0D - UINT8 OptionLen;=0D + UINT16 OptionLen;=0D BOOLEAN Flag;=0D UINT8 CountD;=0D UINT8 CountA;=0D @@ -385,6 +409,36 @@ Ip6IsExtsValid ( // Fall through=0D //=0D case IP6_DESTINATION:=0D + //=0D + // See https://www.rfc-editor.org/rfc/rfc2460#section-4.2 page 23= =0D + //=0D + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+= -+=0D + // | Next Header | Hdr Ext Len | = |=0D + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ = +=0D + // | = |=0D + // . = .=0D + // . Options = .=0D + // . = .=0D + // | = |=0D + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+= -+=0D + //=0D + //=0D + // Next Header 8-bit selector. Identifies the type of header= =0D + // immediately following the Destination Options= =0D + // header. Uses the same values as the IPv4=0D + // Protocol field [RFC-1700 et seq.].=0D + //=0D + // Hdr Ext Len 8-bit unsigned integer. Length of the=0D + // Destination Options header in 8-octet units, n= ot=0D + // including the first 8 octets.=0D + //=0D + // Options Variable-length field, of length such that the= =0D + // complete Destination Options header is an=0D + // integer multiple of 8 octets long. Contains o= ne=0D + // or more TLV-encoded options, as described in= =0D + // section 4.2.=0D + //=0D +=0D if (*NextHeader =3D=3D IP6_DESTINATION) {=0D CountD++;=0D }=0D @@ -398,7 +452,7 @@ Ip6IsExtsValid ( =0D Offset++;=0D Option =3D ExtHdrs + Offset;=0D - OptionLen =3D (UINT8)((*Option + 1) * 8 - 2);=0D + OptionLen =3D IP6_HDR_EXT_LEN (*Option) - sizeof (IP6_EXT_HDR);=0D Option++;=0D Offset++;=0D =0D @@ -430,7 +484,7 @@ Ip6IsExtsValid ( //=0D // Ignore the routing header and proceed to process the next hea= der.=0D //=0D - Offset =3D Offset + (RoutingHead->HeaderLen + 1) * 8;=0D + Offset =3D Offset + IP6_HDR_EXT_LEN (RoutingHead->HeaderLen);=0D =0D if (UnFragmentLen !=3D NULL) {=0D *UnFragmentLen =3D Offset;=0D @@ -441,7 +495,7 @@ Ip6IsExtsValid ( // to the packet's source address, pointing to the unrecognized = routing=0D // type.=0D //=0D - Pointer =3D Offset + 2 + sizeof (EFI_IP6_HEADER);=0D + Pointer =3D Offset + sizeof (IP6_EXT_HDR) + sizeof (EFI_IP6_HEAD= ER);=0D if ((IpSb !=3D NULL) && (Packet !=3D NULL) &&=0D !IP6_IS_MULTICAST (&Packet->Ip.Ip6->DestinationAddress))=0D {=0D @@ -527,7 +581,7 @@ Ip6IsExtsValid ( //=0D // RFC2402, Payload length is specified in 32-bit words, minus "2"= .=0D //=0D - OptionLen =3D (UINT8)((*Option + 2) * 4);=0D + OptionLen =3D ((UINT16)(*Option + 2) * 4);=0D Offset =3D Offset + OptionLen;=0D break;=0D =0D --=20 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114470): https://edk2.groups.io/g/devel/message/114470 Mute This Topic: https://groups.io/mt/103964983/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-