Re: [edk2-devel] [PATCH 1/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On Fri, Feb 09, 2024 at 19:04:56 -0800, Douglas Flick [MSFT] wrote:
> From: Doug Flick <dougflick@microsoft.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4673
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4534
> 
> This was not part of the Quarkslab bugs however the same pattern
> as CVE-2023-45229 exists in Dhcp6UpdateIaInfo.
> 
> This patch replaces the code in question with the safe function
> created to patch CVE-2023-45229
> 
> >
> >   if (EFI_ERROR (
> >        Dhcp6SeekInnerOptionSafe (
> >          Instance->Config->IaDescriptor.Type,
> >          Option,
> >          OptionLen,
> >          &IaInnerOpt,
> >          &IaInnerLen
> >          )
> >        ))
> >  {
> >    return EFI_DEVICE_ERROR;
> >  }
> >

Could you possibly introduce a declaration for
Dhcp6SeekInnerOptionSafe () instead of moving it? (Or break the move
out as a separate commit.)
That would significantly reduce the likelihood of mistakes during review.

/
    Leif

> Additionally corrects incorrect usage of macro to read the status
> 
> > - StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN
>  (Option)));
> > + StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)
> DHCP6_OFFSET_OF_STATUS_CODE (Option));
> 
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
> ---
>  NetworkPkg/Dhcp6Dxe/Dhcp6Io.c | 232 ++++++++++++++++++++--------------
>  1 file changed, 134 insertions(+), 98 deletions(-)
> 
> diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
> index 3b8feb4a2032..6000e885afaf 100644
> --- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
> +++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
> @@ -510,6 +510,97 @@ Dhcp6CallbackUser (
>    return Status;
>  }
>  
> +/**
> +  Seeks the Inner Options from a DHCP6 Option
> +
> +  @param[in]  IaType          The type of the IA option.
> +  @param[in]  Option          The pointer to the DHCP6 Option.
> +  @param[in]  OptionLen       The length of the DHCP6 Option.
> +  @param[out] IaInnerOpt      The pointer to the IA inner option.
> +  @param[out] IaInnerLen      The length of the IA inner option.
> +
> +  @retval EFI_SUCCESS         Seek the inner option successfully.
> +  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,
> +                              the pointers are not modified
> +**/
> +EFI_STATUS
> +Dhcp6SeekInnerOptionSafe (
> +  IN  UINT16  IaType,
> +  IN  UINT8   *Option,
> +  IN  UINT32  OptionLen,
> +  OUT UINT8   **IaInnerOpt,
> +  OUT UINT16  *IaInnerLen
> +  )
> +{
> +  UINT16  IaInnerLenTmp;
> +  UINT8   *IaInnerOptTmp;
> +
> +  if (Option == NULL) {
> +    ASSERT (Option != NULL);
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  if (IaInnerOpt == NULL) {
> +    ASSERT (IaInnerOpt != NULL);
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  if (IaInnerLen == NULL) {
> +    ASSERT (IaInnerLen != NULL);
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  if (IaType == Dhcp6OptIana) {
> +    //
> +    // Verify we have a fully formed IA_NA
> +    //
> +    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    //
> +    // Get the IA Inner Option and Length
> +    //
> +    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
> +
> +    //
> +    // Verify the IaInnerLen is valid.
> +    //
> +    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));
> +    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;
> +  } else if (IaType == Dhcp6OptIata) {
> +    //
> +    // Verify the OptionLen is valid.
> +    //
> +    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
> +
> +    //
> +    // Verify the IaInnerLen is valid.
> +    //
> +    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
> +    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;
> +  } else {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  *IaInnerOpt = IaInnerOptTmp;
> +  *IaInnerLen = IaInnerLenTmp;
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Update Ia according to the new reply message.
>  
> @@ -528,13 +619,23 @@ Dhcp6UpdateIaInfo (
>  {
>    EFI_STATUS  Status;
>    UINT8       *Option;
> +  UINT32      OptionLen;
>    UINT8       *IaInnerOpt;
>    UINT16      IaInnerLen;
>    UINT16      StsCode;
>    UINT32      T1;
>    UINT32      T2;
>  
> +  T1 = 0;
> +  T2 = 0;
> +
>    ASSERT (Instance->Config != NULL);
> +
> +  // OptionLen is the length of the Options excluding the DHCP header.
> +  // Length of the EFI_DHCP6_PACKET from the first byte of the Header field to the last
> +  // byte of the Option[] field.
> +  OptionLen = Packet->Length - sizeof (Packet->Dhcp6.Header);
> +
>    //
>    // If the reply was received in response to a solicit with rapid commit option,
>    // request, renew or rebind message, the client updates the information it has
> @@ -549,13 +650,29 @@ Dhcp6UpdateIaInfo (
>    //
>    Option = Dhcp6SeekIaOption (
>               Packet->Dhcp6.Option,
> -             Packet->Length - sizeof (EFI_DHCP6_HEADER),
> +             OptionLen,
>               &Instance->Config->IaDescriptor
>               );
>    if (Option == NULL) {
>      return EFI_DEVICE_ERROR;
>    }
>  
> +  //
> +  // Calculate the distance from Packet->Dhcp6.Option to the IA option.
> +  //
> +  // Packet->Size and Packet->Length are both UINT32 type, and Packet->Size is
> +  // the size of the whole packet, including the DHCP header, and Packet->Length
> +  // is the length of the DHCP message body, excluding the DHCP header.
> +  //
> +  // (*Option - Packet->Dhcp6.Option) is the number of bytes from the start of
> +  // DHCP6 option area to the start of the IA option.
> +  //
> +  // Dhcp6SeekInnerOptionSafe() is searching starting from the start of the
> +  // IA option to the end of the DHCP6 option area, thus subtract the space
> +  // up until this option
> +  //
> +  OptionLen = OptionLen - (UINT32)(Option - Packet->Dhcp6.Option);
> +
>    //
>    // The format of the IA_NA option is:
>    //
> @@ -591,32 +708,32 @@ Dhcp6UpdateIaInfo (
>    //
>  
>    //
> -  // sizeof (option-code + option-len + IaId)           = 8
> -  // sizeof (option-code + option-len + IaId + T1)      = 12
> -  // sizeof (option-code + option-len + IaId + T1 + T2) = 16
> -  //
> -  // The inner options still start with 2 bytes option-code and 2 bytes option-len.
> +  // Seek the inner option
>    //
> +  if (EFI_ERROR (
> +        Dhcp6SeekInnerOptionSafe (
> +          Instance->Config->IaDescriptor.Type,
> +          Option,
> +          OptionLen,
> +          &IaInnerOpt,
> +          &IaInnerLen
> +          )
> +        ))
> +  {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
>    if (Instance->Config->IaDescriptor.Type == Dhcp6OptIana) {
>      T1 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T1 (Option))));
>      T2 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T2 (Option))));
>      //
>      // Refer to RFC3155 Chapter 22.4. If a client receives an IA_NA with T1 greater than T2,
>      // and both T1 and T2 are greater than 0, the client discards the IA_NA option and processes
> -    // the remainder of the message as though the server had not  included the invalid IA_NA option.
> +    // the remainder of the message as though the server had not included the invalid IA_NA option.
>      //
>      if ((T1 > T2) && (T2 > 0)) {
>        return EFI_DEVICE_ERROR;
>      }
> -
> -    IaInnerOpt = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
> -    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_COMBINED_IAID_T1_T2);
> -  } else {
> -    T1 = 0;
> -    T2 = 0;
> -
> -    IaInnerOpt = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
> -    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_IAID);
>    }
>  
>    //
> @@ -642,7 +759,7 @@ Dhcp6UpdateIaInfo (
>    Option  = Dhcp6SeekOption (IaInnerOpt, IaInnerLen, Dhcp6OptStatusCode);
>  
>    if (Option != NULL) {
> -    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
> +    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_STATUS_CODE (Option))));
>      if (StsCode != Dhcp6StsSuccess) {
>        return EFI_DEVICE_ERROR;
>      }
> @@ -662,87 +779,6 @@ Dhcp6UpdateIaInfo (
>    return Status;
>  }
>  
> -/**
> -  Seeks the Inner Options from a DHCP6 Option
> -
> -  @param[in]  IaType          The type of the IA option.
> -  @param[in]  Option          The pointer to the DHCP6 Option.
> -  @param[in]  OptionLen       The length of the DHCP6 Option.
> -  @param[out] IaInnerOpt      The pointer to the IA inner option.
> -  @param[out] IaInnerLen      The length of the IA inner option.
> -
> -  @retval EFI_SUCCESS         Seek the inner option successfully.
> -  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,
> -                              the pointers are not modified
> -**/
> -EFI_STATUS
> -Dhcp6SeekInnerOptionSafe (
> -  IN  UINT16  IaType,
> -  IN  UINT8   *Option,
> -  IN  UINT32  OptionLen,
> -  OUT UINT8   **IaInnerOpt,
> -  OUT UINT16  *IaInnerLen
> -  )
> -{
> -  UINT16  IaInnerLenTmp;
> -  UINT8   *IaInnerOptTmp;
> -
> -  if (Option == NULL) {
> -    ASSERT (Option != NULL);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (IaInnerOpt == NULL) {
> -    ASSERT (IaInnerOpt != NULL);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (IaInnerLen == NULL) {
> -    ASSERT (IaInnerLen != NULL);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (IaType == Dhcp6OptIana) {
> -    // Verify we have a fully formed IA_NA
> -    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    //
> -    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
> -
> -    // Verify the IaInnerLen is valid.
> -    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));
> -    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;
> -  } else if (IaType == Dhcp6OptIata) {
> -    // Verify the OptionLen is valid.
> -    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
> -
> -    // Verify the IaInnerLen is valid.
> -    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
> -    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;
> -  } else {
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  *IaInnerOpt = IaInnerOptTmp;
> -  *IaInnerLen = IaInnerLenTmp;
> -
> -  return EFI_SUCCESS;
> -}
> -
>  /**
>    Seek StatusCode Option in package. A Status Code option may appear in the
>    options field of a DHCP message and/or in the options field of another option.
> -- 
> 2.43.0
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115370): https://edk2.groups.io/g/devel/message/115370
Mute This Topic: https://groups.io/mt/104272126/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-