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 D2ECDAC1A45 for ; Mon, 5 Feb 2024 16:44:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=2xjehcqUCKwpAPBvyYtEgN9f0vHn8toSx/zWe1+M9qA=; c=relaxed/simple; d=groups.io; h=Subject:To:From:User-Agent:MIME-Version:Date:References:In-Reply-To:Message-ID:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1707151458; v=1; b=XL2ae+SdK+AaXJL4VeeJPW8r3ExlKzKOuJXA76ez4/xKqMGYKsZgHhuhCCz3KyaRbBnK69hc WMBM2HROSzS0klFJTqnfgkf3xB20bwu9ZvNKs5G/P0cvBMHTUAKjVj5JWRPTHTI1nrZvPnopmNW duZVMuiQPFFetDrj6/lPhOFI= X-Received: by 127.0.0.2 with SMTP id sphFYY7687511xj50mdI59pI; Mon, 05 Feb 2024 08:44:18 -0800 Subject: Re: [edk2-devel] [PATCH v2 01/15] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Patch To: Doug Flick ,devel@edk2.groups.io From: "bryan-bt.tan via groups.io" X-Originating-Location: London, England, GB (90.255.43.212) X-Originating-Platform: Mac Firefox 122 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Mon, 05 Feb 2024 05:41:00 -0800 References: In-Reply-To: Message-ID: <18011.1707140460470094636@groups.io> 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,bryan-bt.tan@broadcom.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: HPaTBYTDMLBAagHT3GmmLQ1Yx7686176AA= Content-Type: multipart/alternative; boundary="n2xyjcIZnYMpcBXY8YvL" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=XL2ae+Sd; dmarc=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 --n2xyjcIZnYMpcBXY8YvL Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Thu, Jan 25, 2024 at 11:06 PM, Doug Flick wrote: >=20 > > @@ -607,35 +616,95 @@ Dhcp6AppendOption ( > // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=3D > =3D0D > //=3D0D > =3D0D > - ASSERT (OptLen !=3D3D 0);=3D0D > + //=3D0D > + // Verify the arguments are valid=3D0D > + //=3D0D > + if (Packet =3D3D=3D3D NULL) {=3D0D > + return EFI_INVALID_PARAMETER;=3D0D > + }=3D0D > =3D0D > - WriteUnaligned16 ((UINT16 *)Buf, OptType);=3D0D > - Buf +=3D3D 2;=3D0D > - WriteUnaligned16 ((UINT16 *)Buf, OptLen);=3D0D > - Buf +=3D3D 2;=3D0D > - CopyMem (Buf, Data, NTOHS (OptLen));=3D0D > - Buf +=3D3D NTOHS (OptLen);=3D0D > + if ((PacketCursor =3D3D=3D3D NULL) || (*PacketCursor =3D3D=3D3D NULL)) = {=3D0D > + return EFI_INVALID_PARAMETER;=3D0D > + }=3D0D > =3D0D > - return Buf;=3D0D > + if (Data =3D3D=3D3D NULL) {=3D0D > + return EFI_INVALID_PARAMETER;=3D0D > + }=3D0D > +=3D0D > + if (OptLen =3D3D=3D3D 0) {=3D0D > + return EFI_INVALID_PARAMETER;=3D0D > + }=3D0D > +=3D0D > + //=3D0D > + // Verify the PacketCursor is within the packet=3D0D > + //=3D0D > + if ( (*PacketCursor < Packet->Dhcp6.Option)=3D0D > + || (*PacketCursor >=3D3D Packet->Dhcp6.Option + (Packet->Size - sizeof = =3D > (EFI_DHCP6_HEADER))))=3D0D > + {=3D0D > + return EFI_INVALID_PARAMETER;=3D0D > + }=3D0D > +=3D0D > + //=3D0D > + // Calculate the bytes needed for the option=3D0D > + //=3D0D > + BytesNeeded =3D3D DHCP6_SIZE_OF_COMBINED_CODE_AND_LEN + NTOHS (OptLen);= =3D0D >=20 > +=3D0D > + //=3D0D > + // Space remaining in the packet=3D0D > + //=3D0D > + Length =3D3D Packet->Size - Packet->Length;=3D0D > + if (Length < BytesNeeded) {=3D0D > + return EFI_BUFFER_TOO_SMALL;=3D0D > + }=3D0D > +=3D0D > + //=3D0D > + // Verify the PacketCursor is within the packet=3D0D > + //=3D0D > + if ( (*PacketCursor < Packet->Dhcp6.Option)=3D0D > + || (*PacketCursor >=3D3D Packet->Dhcp6.Option + (Packet->Size - sizeof = =3D > (EFI_DHCP6_HEADER))))=3D0D > + {=3D0D > + return EFI_INVALID_PARAMETER;=3D0D > + }=3D0D This check is unnecessary; the same check is done above. >=20 > > @@ -766,35 +934,51 @@ Dhcp6AppendIaOption ( > //=3D0D > for (Index =3D3D 0; Index < Ia->IaAddressCount; Index++) {=3D0D > AddrOpt =3D3D (UINT8 *)Ia->IaAddress + Index * sizeof (EFI_DHCP6_IA_ADDR= =3D > ESS);=3D0D > - Buf =3D3D Dhcp6AppendIaAddrOption (Buf, (EFI_DHCP6_IA_ADDRESS *)Addr=3D > Opt, MessageType);=3D0D > + Status =3D3D Dhcp6AppendIaAddrOption (Packet, PacketCursor, (EFI_DHCP6_= =3D > IA_ADDRESS *)AddrOpt, MessageType);=3D0D > + if (EFI_ERROR (Status)) {=3D0D > + return Status;=3D0D > + }=3D0D > }=3D0D > =3D0D > //=3D0D > // Fill the value of Ia option length=3D0D > //=3D0D > - *Len =3D3D HTONS ((UINT16)(Buf - (UINT8 *)Len - 2));=3D0D > + *Len =3D3D HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));=3D0D > =3D0D > - return Buf;=3D0D > + //=3D0D > + // Update the packet length=3D0D > + //=3D0D > + Packet->Length +=3D3D BytesNeeded;=3D0D Shouldn't we update Packet->Length before calling Dhcp6AppendIaAddrOption as it needs to know how much space is left in the Packet? >=20 > +=3D0D > + return EFI_SUCCESS;=3D0D > }=3D0D > =3D0D > /**=3D0D > Append the appointed Elapsed time option to Buf, and move Buf to the end= =3D > .=3D0D > =3D0D > - @param[in, out] Buf The pointer to the position to append.=3D0D > + @param[in, out] Packet A pointer to the packet, on success Packet=3D > ->Length=3D0D Nit: Missing "will be updated." like other function header comments. >=20 > + @param[in, out] PacketCursor The pointer in the packet, on success Pack= =3D >=20 > etCursor=3D0D > + will be moved to the end of the option.=3D0D > @param[in] Instance The pointer to the Dhcp6 instance.=3D0D > @param[out] Elapsed The pointer to the elapsed time value in=3D0D > - the generated packet.=3D0D > + the generated packet.=3D0D > =3D0D > - @return Buf The position to append the next Ia option.=3D > =3D0D > + @retval EFI_INVALID_PARAMETER An argument provided to the function was= =3D > invalid=3D0D > + @retval EFI_BUFFER_TOO_SMALL The buffer is too small to append the op= =3D > tion.=3D0D > + @retval EFI_SUCCESS The option is appended successfully.=3D0D > =3D0D > **/=3D0D > -UINT8 *=3D0D > +EFI_STATUS=3D0D > Dhcp6AppendETOption (=3D0D > - IN OUT UINT8 *Buf,=3D0D > - IN DHCP6_INSTANCE *Instance,=3D0D > - OUT UINT16 **Elapsed=3D0D > + IN OUT EFI_DHCP6_PACKET *Packet,=3D0D > + IN OUT UINT8 **PacketCursor,=3D0D > + IN DHCP6_INSTANCE *Instance,=3D0D > + OUT UINT16 **Elapsed=3D0D > )=3D0D > {=3D0D > + UINT32 BytesNeeded;=3D0D > + UINT32 Length;=3D0D > +=3D0D > //=3D0D > // The format of elapsed time option:=3D0D > //=3D0D Apologies for any poor formatting, this is my first time posting on this mailing list and I'm using the web interface. -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --n2xyjcIZnYMpcBXY8YvL Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Thu, Jan 25, 2024 at 11:06 PM, Doug Flick wrote:
<snip>
@@ -607,35 +616,95 @@ Dhcp6AppendOption (
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=3D=3D0D
//=3D0D
=3D0D
- ASSERT (OptLen !=3D3D 0);=3D0D
+ //=3D0D
+ // Verify the arguments are valid=3D0D
+ //=3D0D+ if (Packet =3D3D=3D3D NULL) {=3D0D
+ return EFI_INVALID_PARAMETER= ;=3D0D
+ }=3D0D
=3D0D
- WriteUnaligned16 ((UINT16 *)Buf, Opt= Type);=3D0D
- Buf +=3D3D 2;=3D0D
- WriteUnaligned16 ((UINT16 *)Bu= f, OptLen);=3D0D
- Buf +=3D3D 2;=3D0D
- CopyMem (Buf, Data, NTOHS= (OptLen));=3D0D
- Buf +=3D3D NTOHS (OptLen);=3D0D
+ if ((PacketC= ursor =3D3D=3D3D NULL) || (*PacketCursor =3D3D=3D3D NULL)) {=3D0D
+ re= turn EFI_INVALID_PARAMETER;=3D0D
+ }=3D0D
=3D0D
- return Buf= ;=3D0D
+ if (Data =3D3D=3D3D NULL) {=3D0D
+ return EFI_INVALID_PA= RAMETER;=3D0D
+ }=3D0D
+=3D0D
+ if (OptLen =3D3D=3D3D 0) {= =3D0D
+ return EFI_INVALID_PARAMETER;=3D0D
+ }=3D0D
+=3D0D+ //=3D0D
+ // Verify the PacketCursor is within the packet=3D0D+ //=3D0D
+ if ( (*PacketCursor < Packet->Dhcp6.Option)=3D0D=
+ || (*PacketCursor >=3D3D Packet->Dhcp6.Option + (Packet->S= ize - sizeof =3D
(EFI_DHCP6_HEADER))))=3D0D
+ {=3D0D
+ retur= n EFI_INVALID_PARAMETER;=3D0D
+ }=3D0D
+=3D0D
+ //=3D0D
+ // Calculate the bytes needed for the option=3D0D
+ //=3D0D
+ = BytesNeeded =3D3D DHCP6_SIZE_OF_COMBINED_CODE_AND_LEN + NTOHS (OptLen);=3D0= D
+=3D0D
+ //=3D0D
+ // Space remaining in the packet=3D0D+ //=3D0D
+ Length =3D3D Packet->Size - Packet->Length;=3D0D=
+ if (Length < BytesNeeded) {=3D0D
+ return EFI_BUFFER_TOO_SM= ALL;=3D0D
+ }=3D0D
+=3D0D
+ //=3D0D
+ // Verify the Pac= ketCursor is within the packet=3D0D
+ //=3D0D
+ if ( (*PacketCurs= or < Packet->Dhcp6.Option)=3D0D
+ || (*PacketCursor >=3D3D Pa= cket->Dhcp6.Option + (Packet->Size - sizeof =3D
(EFI_DHCP6_HEADE= R))))=3D0D
+ {=3D0D
+ return EFI_INVALID_PARAMETER;=3D0D
+ }= =3D0D
This check is unnecessary; the same check is done above.
<snip>
@@ -766,35 +934,51 @@ Dhcp6AppendIaOption (//=3D0D
for (Index =3D3D 0; Index < Ia->IaAddressCount; Inde= x++) {=3D0D
AddrOpt =3D3D (UINT8 *)Ia->IaAddress + Index * sizeof (= EFI_DHCP6_IA_ADDR=3D
ESS);=3D0D
- Buf =3D3D Dhcp6AppendIaAddrOpti= on (Buf, (EFI_DHCP6_IA_ADDRESS *)Addr=3D
Opt, MessageType);=3D0D
= + Status =3D3D Dhcp6AppendIaAddrOption (Packet, PacketCursor, (EFI_DHCP6_= =3D
IA_ADDRESS *)AddrOpt, MessageType);=3D0D
+ if (EFI_ERROR (Sta= tus)) {=3D0D
+ return Status;=3D0D
+ }=3D0D
}=3D0D
=3D0= D
//=3D0D
// Fill the value of Ia option length=3D0D
//=3D0D=
- *Len =3D3D HTONS ((UINT16)(Buf - (UINT8 *)Len - 2));=3D0D
+ *L= en =3D3D HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));=3D0D
=3D0= D
- return Buf;=3D0D
+ //=3D0D
+ // Update the packet length= =3D0D
+ //=3D0D
+ Packet->Length +=3D3D BytesNeeded;=3D0D

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

+=3D0D
+ return EFI_SUCCESS;=3D0D
}=3D0D
=3D0D/**=3D0D
Append the appointed Elapsed time option to Buf, and move= Buf to the end=3D
.=3D0D
=3D0D
- @param[in, out] Buf The po= inter to the position to append.=3D0D
+ @param[in, out] Packet A point= er to the packet, on success Packet=3D
->Length=3D0D
Nit: Missing "will be updated." like other function header comments.
+ @param[in, out] PacketCursor The pointer in the packet, on su= ccess Pack=3D
etCursor=3D0D
+ will be moved to the end of the opt= ion.=3D0D
@param[in] Instance The pointer to the Dhcp6 instance.=3D0D<= br />@param[out] Elapsed The pointer to the elapsed time value in=3D0D
- the generated packet.=3D0D
+ the generated packet.=3D0D
=3D0D<= br />- @return Buf The position to append the next Ia option.=3D
=3D0D=
+ @retval EFI_INVALID_PARAMETER An argument provided to the function = was=3D
invalid=3D0D
+ @retval EFI_BUFFER_TOO_SMALL The buffer is = too small to append the op=3D
tion.=3D0D
+ @retval EFI_SUCCESS Th= e option is appended successfully.=3D0D
=3D0D
**/=3D0D
-UINT= 8 *=3D0D
+EFI_STATUS=3D0D
Dhcp6AppendETOption (=3D0D
- IN OU= T UINT8 *Buf,=3D0D
- IN DHCP6_INSTANCE *Instance,=3D0D
- OUT UINT= 16 **Elapsed=3D0D
+ IN OUT EFI_DHCP6_PACKET *Packet,=3D0D
+ IN OU= T UINT8 **PacketCursor,=3D0D
+ IN DHCP6_INSTANCE *Instance,=3D0D
= + OUT UINT16 **Elapsed=3D0D
)=3D0D
{=3D0D
+ UINT32 BytesNeed= ed;=3D0D
+ UINT32 Length;=3D0D
+=3D0D
//=3D0D
// The fo= rmat of elapsed time option:=3D0D
//=3D0D
Apologies for any poor formatting, this is my first time posting on thismailing list and I'm using the web interface.
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#115120) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--n2xyjcIZnYMpcBXY8YvL--