On Wed, Jan 24, 2024 at 5:20 AM Doug Flick via groups.io <dougflick=microsoft....@groups.io> wrote: > > From: Doug Flick <dougfl...@microsoft.com> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538 > > 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 <saloni.kasbe...@intel.com> > Cc: Zachary Clark-williams <zachary.clark-willi...@intel.com> > > Signed-off-by: Doug Flick [MSFT] <doug.e...@gmail.com> > --- > 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 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. > +//
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 + > 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 > +// 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 == 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 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_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; > 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 == NULL) { > + ASSERT (Option != NULL); > + return FALSE; > + } > + > + if ((OptionLen <= 0) || (OptionLen > IP6_MAX_EXT_DATA_LENGTH)) { OptionLen is unsigned hence < 0 is not a valid check -- Pedro -=-=-=-=-=-=-=-=-=-=-=- 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-