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]
-=-=-=-=-=-=-=-=-=-=-=-
