Re: [edk2-devel] [PATCH 07/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Wed, Jan 24, 2024 at 5:20 AM Doug Flick via groups.io
<dougflick=microsoft.com@groups.io> wrote:
>
> From: Doug Flick <dougflick@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.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
>
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-