Re: [edk2-devel] [PATCH v2 01/15] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Patch

["bryan-bt.tan via groups.io" <bryan-bt.tan=broadcom.com@groups.io>]

[-- Attachment #1: Type: text/plain, Size: 4914 bytes --]

On Thu, Jan 25, 2024 at 11:06 PM, Doug Flick wrote:

> 
> <snip>
> @@ -607,35 +616,95 @@ Dhcp6AppendOption (
> // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=
> =0D
> //=0D
> =0D
> - ASSERT (OptLen !=3D 0);=0D
> + //=0D
> + // Verify the arguments are valid=0D
> + //=0D
> + if (Packet =3D=3D NULL) {=0D
> + return EFI_INVALID_PARAMETER;=0D
> + }=0D
> =0D
> - WriteUnaligned16 ((UINT16 *)Buf, OptType);=0D
> - Buf +=3D 2;=0D
> - WriteUnaligned16 ((UINT16 *)Buf, OptLen);=0D
> - Buf +=3D 2;=0D
> - CopyMem (Buf, Data, NTOHS (OptLen));=0D
> - Buf +=3D NTOHS (OptLen);=0D
> + if ((PacketCursor =3D=3D NULL) || (*PacketCursor =3D=3D NULL)) {=0D
> + return EFI_INVALID_PARAMETER;=0D
> + }=0D
> =0D
> - return Buf;=0D
> + if (Data =3D=3D NULL) {=0D
> + return EFI_INVALID_PARAMETER;=0D
> + }=0D
> +=0D
> + if (OptLen =3D=3D 0) {=0D
> + return EFI_INVALID_PARAMETER;=0D
> + }=0D
> +=0D
> + //=0D
> + // Verify the PacketCursor is within the packet=0D
> + //=0D
> + if ( (*PacketCursor < Packet->Dhcp6.Option)=0D
> + || (*PacketCursor >=3D Packet->Dhcp6.Option + (Packet->Size - sizeof =
> (EFI_DHCP6_HEADER))))=0D
> + {=0D
> + return EFI_INVALID_PARAMETER;=0D
> + }=0D
> +=0D
> + //=0D
> + // Calculate the bytes needed for the option=0D
> + //=0D
> + BytesNeeded =3D DHCP6_SIZE_OF_COMBINED_CODE_AND_LEN + NTOHS (OptLen);=0D
> 
> +=0D
> + //=0D
> + // Space remaining in the packet=0D
> + //=0D
> + Length =3D Packet->Size - Packet->Length;=0D
> + if (Length < BytesNeeded) {=0D
> + return EFI_BUFFER_TOO_SMALL;=0D
> + }=0D
> +=0D
> + //=0D
> + // Verify the PacketCursor is within the packet=0D
> + //=0D
> + if ( (*PacketCursor < Packet->Dhcp6.Option)=0D
> + || (*PacketCursor >=3D Packet->Dhcp6.Option + (Packet->Size - sizeof =
> (EFI_DHCP6_HEADER))))=0D
> + {=0D
> + return EFI_INVALID_PARAMETER;=0D
> + }=0D

This check is unnecessary; the same check is done above.

> 
> <snip>
> @@ -766,35 +934,51 @@ Dhcp6AppendIaOption (
> //=0D
> for (Index =3D 0; Index < Ia->IaAddressCount; Index++) {=0D
> AddrOpt =3D (UINT8 *)Ia->IaAddress + Index * sizeof (EFI_DHCP6_IA_ADDR=
> ESS);=0D
> - Buf =3D Dhcp6AppendIaAddrOption (Buf, (EFI_DHCP6_IA_ADDRESS *)Addr=
> Opt, MessageType);=0D
> + Status =3D Dhcp6AppendIaAddrOption (Packet, PacketCursor, (EFI_DHCP6_=
> IA_ADDRESS *)AddrOpt, MessageType);=0D
> + if (EFI_ERROR (Status)) {=0D
> + return Status;=0D
> + }=0D
> }=0D
> =0D
> //=0D
> // Fill the value of Ia option length=0D
> //=0D
> - *Len =3D HTONS ((UINT16)(Buf - (UINT8 *)Len - 2));=0D
> + *Len =3D HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));=0D
> =0D
> - return Buf;=0D
> + //=0D
> + // Update the packet length=0D
> + //=0D
> + Packet->Length +=3D BytesNeeded;=0D

Shouldn't we update Packet->Length before calling Dhcp6AppendIaAddrOption
as it needs to know how much space is left in the Packet?

> 
> +=0D
> + return EFI_SUCCESS;=0D
> }=0D
> =0D
> /**=0D
> Append the appointed Elapsed time option to Buf, and move Buf to the end=
> .=0D
> =0D
> - @param[in, out] Buf The pointer to the position to append.=0D
> + @param[in, out] Packet A pointer to the packet, on success Packet=
> ->Length=0D

Nit: Missing "will be updated." like other function header comments.

> 
> + @param[in, out] PacketCursor The pointer in the packet, on success Pack=
> 
> etCursor=0D
> + will be moved to the end of the option.=0D
> @param[in] Instance The pointer to the Dhcp6 instance.=0D
> @param[out] Elapsed The pointer to the elapsed time value in=0D
> - the generated packet.=0D
> + the generated packet.=0D
> =0D
> - @return Buf The position to append the next Ia option.=
> =0D
> + @retval EFI_INVALID_PARAMETER An argument provided to the function was=
> invalid=0D
> + @retval EFI_BUFFER_TOO_SMALL The buffer is too small to append the op=
> tion.=0D
> + @retval EFI_SUCCESS The option is appended successfully.=0D
> =0D
> **/=0D
> -UINT8 *=0D
> +EFI_STATUS=0D
> Dhcp6AppendETOption (=0D
> - IN OUT UINT8 *Buf,=0D
> - IN DHCP6_INSTANCE *Instance,=0D
> - OUT UINT16 **Elapsed=0D
> + IN OUT EFI_DHCP6_PACKET *Packet,=0D
> + IN OUT UINT8 **PacketCursor,=0D
> + IN DHCP6_INSTANCE *Instance,=0D
> + OUT UINT16 **Elapsed=0D
> )=0D
> {=0D
> + UINT32 BytesNeeded;=0D
> + UINT32 Length;=0D
> +=0D
> //=0D
> // The format of elapsed time option:=0D
> //=0D

Apologies for any poor formatting, this is my first time posting on this
mailing list and I'm using the web interface.


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



[-- Attachment #2: Type: text/html, Size: 5746 bytes --]