* [edk2-devel] [PATCH v1 1/3] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
2024-02-09 0:23 [edk2-devel] [PATCH v1 0/3] Corrects additional concern in NetworkPkg Doug Flick via groups.io
@ 2024-02-09 0:24 ` Doug Flick via groups.io
2024-02-09 0:24 ` [edk2-devel] [PATCH v1 2/3] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup Doug Flick via groups.io
2024-02-09 0:24 ` [edk2-devel] [PATCH v1 3/3] NetworkPkg: : Updating SecurityFixes.yaml Doug Flick via groups.io
2 siblings, 0 replies; 6+ messages in thread
From: Doug Flick via groups.io @ 2024-02-09 0:24 UTC (permalink / raw)
To: devel
Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams,
Doug Flick [MSFT]
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;
> }
>
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>
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 (#115281): https://edk2.groups.io/g/devel/message/115281
Mute This Topic: https://groups.io/mt/104250123/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 6+ messages in thread* [edk2-devel] [PATCH v1 2/3] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup
2024-02-09 0:23 [edk2-devel] [PATCH v1 0/3] Corrects additional concern in NetworkPkg Doug Flick via groups.io
2024-02-09 0:24 ` [edk2-devel] [PATCH v1 1/3] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Doug Flick via groups.io
@ 2024-02-09 0:24 ` Doug Flick via groups.io
2024-02-09 0:24 ` [edk2-devel] [PATCH v1 3/3] NetworkPkg: : Updating SecurityFixes.yaml Doug Flick via groups.io
2 siblings, 0 replies; 6+ messages in thread
From: Doug Flick via groups.io @ 2024-02-09 0:24 UTC (permalink / raw)
To: devel
Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams,
Doug Flick [MSFT]
From: Doug Flick <dougflick@microsoft.com>
Removes duplicate check after merge
>
> //
> // Verify the PacketCursor is within the packet
> //
> if ( (*PacketCursor < Packet->Dhcp6.Option)
> || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size -
sizeof (EFI_DHCP6_HEADER))))
> {
> return EFI_INVALID_PARAMETER;
> }
>
Additionally Calculates the Packet->Length before appending
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/Dhcp6Dxe/Dhcp6Utility.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
index 705c665c519d..348602c4e1a8 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
@@ -657,15 +657,6 @@ Dhcp6AppendOption (
return EFI_BUFFER_TOO_SMALL;
}
- //
- // Verify the PacketCursor is within the packet
- //
- if ( (*PacketCursor < Packet->Dhcp6.Option)
- || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
- {
- return EFI_INVALID_PARAMETER;
- }
-
WriteUnaligned16 ((UINT16 *)*PacketCursor, OptType);
*PacketCursor += DHCP6_SIZE_OF_OPT_CODE;
WriteUnaligned16 ((UINT16 *)*PacketCursor, OptLen);
@@ -929,6 +920,11 @@ Dhcp6AppendIaOption (
*PacketCursor += sizeof (T2);
}
+ //
+ // Update the packet length
+ //
+ Packet->Length += BytesNeeded;
+
//
// Fill all the addresses belong to the Ia
//
@@ -945,11 +941,6 @@ Dhcp6AppendIaOption (
//
*Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));
- //
- // Update the packet length
- //
- Packet->Length += BytesNeeded;
-
return EFI_SUCCESS;
}
@@ -957,6 +948,7 @@ Dhcp6AppendIaOption (
Append the appointed Elapsed time option to Buf, and move Buf to the end.
@param[in, out] Packet A pointer to the packet, on success Packet->Length
+ will be updated.
@param[in, out] PacketCursor The pointer in the packet, on success PacketCursor
will be moved to the end of the option.
@param[in] Instance The pointer to the Dhcp6 instance.
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115282): https://edk2.groups.io/g/devel/message/115282
Mute This Topic: https://groups.io/mt/104250124/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 6+ messages in thread