From d83ec916656f8af8342b18ddca088720d377415d Mon Sep 17 00:00:00 2001 From: Doug Flick Date: Mon, 18 Dec 2023 10:37:40 -0800 Subject: [PATCH] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537 REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538 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 = (UINT8)(Offset + *(Option + Offset + 1) + 2); > + OptDataLen = *(IP6_OFFSET_OF_OPT_LEN (Option + Offset)); > + Offset = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); > case Ip6OptionSkip: > - Offset = (UINT8)(Offset + *(Option + Offset + 1)); > OptDataLen = *(IP6_OFFSET_OF_OPT_LEN (Option + Offset)); > Offset = 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 = (UINT8)((*Option + 1) * 8 - 2); > + OptionLen = 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/Ip6Option.c | 76 +++++++++++++++++++++++++----- NetworkPkg/Ip6Dxe/Ip6Option.h | 89 +++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 11 deletions(-) diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c index 8718d5d8756ac..144f8d34deadf 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 == NULL) { + ASSERT (Option != NULL); + return FALSE; + } + + if ((OptionLen <= 0) || (OptionLen > IP6_MAX_EXT_DATA_LENGTH)) { + ASSERT (OptionLen > 0 && OptionLen <= IP6_MAX_EXT_DATA_LENGTH); + return FALSE; + } + + if (Packet == NULL) { + ASSERT (Packet != NULL); + return FALSE; + } + + if (IpSb == NULL) { + ASSERT (IpSb != NULL); + return FALSE; + } Offset = 0; @@ -54,7 +76,8 @@ Ip6IsOptionValid ( // // It is a PadN option // - Offset = (UINT8)(Offset + *(Option + Offset + 1) + 2); + OptDataLen = *(IP6_OFFSET_OF_OPT_LEN (Option + Offset)); + Offset = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); break; case Ip6OptionRouterAlert: // @@ -69,7 +92,8 @@ Ip6IsOptionValid ( // switch (OptionType & Ip6OptionMask) { case Ip6OptionSkip: - Offset = (UINT8)(Offset + *(Option + Offset + 1)); + OptDataLen = *(IP6_OFFSET_OF_OPT_LEN (Option + Offset)); + Offset = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); break; case Ip6OptionDiscard: return FALSE; @@ -308,7 +332,7 @@ Ip6IsExtsValid ( UINT32 Pointer; UINT32 Offset; UINT8 *Option; - UINT8 OptionLen; + UINT16 OptionLen; BOOLEAN Flag; UINT8 CountD; UINT8 CountA; @@ -385,6 +409,36 @@ Ip6IsExtsValid ( // Fall through // case IP6_DESTINATION: + // + // See https://www.rfc-editor.org/rfc/rfc2460#section-4.2 page 23 + // + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + // | Next Header | Hdr Ext Len | | + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + + // | | + // . . + // . Options . + // . . + // | | + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + // + // + // Next Header 8-bit selector. Identifies the type of header + // immediately following the Destination Options + // header. Uses the same values as the IPv4 + // Protocol field [RFC-1700 et seq.]. + // + // Hdr Ext Len 8-bit unsigned integer. Length of the + // Destination Options header in 8-octet units, not + // including the first 8 octets. + // + // Options Variable-length field, of length such that the + // complete Destination Options header is an + // integer multiple of 8 octets long. Contains one + // or more TLV-encoded options, as described in + // section 4.2. + // + if (*NextHeader == IP6_DESTINATION) { CountD++; } @@ -398,7 +452,7 @@ Ip6IsExtsValid ( Offset++; Option = ExtHdrs + Offset; - OptionLen = (UINT8)((*Option + 1) * 8 - 2); + OptionLen = IP6_HDR_EXT_LEN (*Option) - IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN; Option++; Offset++; @@ -430,7 +484,7 @@ Ip6IsExtsValid ( // // Ignore the routing header and proceed to process the next header. // - Offset = Offset + (RoutingHead->HeaderLen + 1) * 8; + Offset = Offset + IP6_HDR_EXT_LEN (RoutingHead->HeaderLen); if (UnFragmentLen != NULL) { *UnFragmentLen = Offset; @@ -441,7 +495,7 @@ Ip6IsExtsValid ( // to the packet's source address, pointing to the unrecognized routing // type. // - Pointer = Offset + 2 + sizeof (EFI_IP6_HEADER); + Pointer = Offset + IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN + sizeof (EFI_IP6_HEADER); if ((IpSb != NULL) && (Packet != NULL) && !IP6_IS_MULTICAST (&Packet->Ip.Ip6->DestinationAddress)) { @@ -527,7 +581,7 @@ Ip6IsExtsValid ( // // RFC2402, Payload length is specified in 32-bit words, minus "2". // - OptionLen = (UINT8)((*Option + 2) * 4); + OptionLen = ((UINT16)(*Option + 2) * 4); Offset = Offset + OptionLen; break; diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.h b/NetworkPkg/Ip6Dxe/Ip6Option.h index bd8e223c8a676..5d786073ebcbc 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 variable +// number of type-length-value (TLV) encoded "options", of the following +// 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 Option +// Data field of this option, in octets. +// +// Option Data Variable-length field. Option-Type-specific +// data. +// +#define IP6_SIZE_OF_OPT_TYPE (sizeof(UINT8)) +#define IP6_SIZE_OF_OPT_LEN (sizeof(UINT8)) +#define IP6_COMBINED_SIZE_OF_OPT_TAG_AND_LEN (IP6_SIZE_OF_OPT_TYPE + IP6_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) == 1, + "The Length field should be 1 octet (8 bits) past the start of the option" + ); + +#define IP6_NEXT_OPTION_OFFSET(offset, length) (offset + IP6_COMBINED_SIZE_OF_OPT_TAG_AND_LEN + length) +STATIC_ASSERT ( + IP6_NEXT_OPTION_OFFSET (0, 0) == 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 units, +// not including the first 8 octets. + +// +// These defines apply to the following: +// 1. Hop by Hop +// 2. Routing +// 3. Destination +// +#define IP6_SIZE_OF_EXT_NEXT_HDR (sizeof(UINT8)) +#define IP6_SIZE_OF_HDR_EXT_LEN (sizeof(UINT8)) + +#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 == 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 first 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 is the maxmimum length permissible by a extension header +// 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_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN) +STATIC_ASSERT ( + IP6_MAX_EXT_DATA_LENGTH == 2046, + "Maximum data length is ((MAX_UINT8 + 1) * 8) - 2" + ); + typedef struct _IP6_FRAGMENT_HEADER { UINT8 NextHeader; UINT8 Reserved;