From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id E1BB5941CED for ; Mon, 12 Feb 2024 18:56:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=0+H2K/gCSBcjo7Gv64FkHHql6ky1Ukh6cTND+gILJgM=; c=relaxed/simple; d=groups.io; h=Date:From:To:CC:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1707764181; v=1; b=PsvYUn1NBopl/+B3teYUQE76/CZleGNAZVldzh7v284CLBbDfjnQBNUIERGn/bRkdoB5RjCg sxAWRIzdzaaTfA/ZNGzRLnx7flKnuzIDoZV5aFJiyzmhnpoEBMiFZ+vmcgE+Vuc5LpcX2eCAGtp 31qBQ1acOwuiiRakBwgj4IiQ= X-Received: by 127.0.0.2 with SMTP id 09bWYY7687511xoDBRl2rP3Q; Mon, 12 Feb 2024 10:56:21 -0800 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web11.15021.1707764180941021879 for ; Mon, 12 Feb 2024 10:56:21 -0800 X-Received: from pps.filterd (m0279869.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.24/8.17.1.24) with ESMTP id 41CHR8hW032250; Mon, 12 Feb 2024 18:56:14 GMT X-Received: from nasanppmta04.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3w62q2v655-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Feb 2024 18:56:14 +0000 (GMT) X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA04.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 41CIuD08008124 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Feb 2024 18:56:13 GMT X-Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Mon, 12 Feb 2024 10:56:11 -0800 Date: Mon, 12 Feb 2024 18:56:08 +0000 From: "Leif Lindholm" To: "Douglas Flick [MSFT]" CC: , Doug Flick , Saloni Kasbekar , Zachary Clark-williams , Andrew Fish , Michael D Kinney Subject: Re: [edk2-devel] [PATCH 1/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Message-ID: References: <18e9d08ad2c15daa4fee37ba5283856e758d6e94.1707534069.git.doug.edk2@gmail.com> MIME-Version: 1.0 In-Reply-To: <18e9d08ad2c15daa4fee37ba5283856e758d6e94.1707534069.git.doug.edk2@gmail.com> X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-ORIG-GUID: 6LHUUIZYYAneXhJVJLncbagCZaFmTKks X-Proofpoint-GUID: 6LHUUIZYYAneXhJVJLncbagCZaFmTKks Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: PbMH059DUEjq2W517A4F21Iix7686176AA= Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=PsvYUn1N; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Fri, Feb 09, 2024 at 19:04:56 -0800, Douglas Flick [MSFT] wrote: > From: Doug Flick > > 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 > Cc: Zachary Clark-williams > > Cc: Andrew Fish > Cc: Leif Lindholm > Cc: Michael D Kinney > > Signed-off-by: Doug Flick [MSFT] > --- > 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] -=-=-=-=-=-=-=-=-=-=-=-