Array out of bounds write

Array out of bounds write

[Santhapur Naveen]

Hello all,

            In the file PxeBcSupport.c of NetworkPkg there is

EFI_STATUS
PxeBcUdp4Write (
{
            ...
            //
            // Arrange one fragment buffer for data, and another fragment buffer for header if has.
            //
            FragCount = (HeaderSize != NULL) ? 2 : 1;
            ...
            TxData->FragmentTable[FragCount - 1].FragmentLength = (UINT32) *BufferSize;
            ...
}

And similarly in

EFI_STATUS
PxeBcUdp6Write (
{
            ...
            //
            // Arrange one fragment buffer for data, and another fragment buffer for header if has.
            //
            FragCount = (HeaderSize != NULL) ? 2 : 1;
            ...
            TxData->FragmentTable[FragCount - 1].FragmentLength = (UINT32) *BufferSize;
            ...
}

If HeaderSize is not NULL, then there is a chance of writing array over bounds since FragmentTable is of single element.

///
  /// Array of fragment descriptors.
  ///
  EFI_UDP6_FRAGMENT_DATA    FragmentTable[1];

Shouldn't we be taking care of this?


Regards,
Naveen

Re: Array out of bounds write

[Wu, Jiaxin]

Hi Naveen,

Please feel free for the below coding, the array should never over bound since the buffer has been allocated before using it.

 	 FragCount = (HeaderSize != NULL) ? 2 : 1;
  	TxLength  = sizeof (EFI_UDP6_TRANSMIT_DATA) + (FragCount - 1) * sizeof (EFI_UDP6_FRAGMENT_DATA);
  	TxData    = (EFI_UDP6_TRANSMIT_DATA *) AllocateZeroPool (TxLength);

Actually, you can treat it as pointer, and the allocated memory would be lost since we have defined the FragmentCount.

Thanks,
Jiaxin

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Santhapur Naveen
> Sent: Tuesday, April 11, 2017 4:06 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Array out of bounds write
> Importance: High
> 
> Hello all,
> 
>             In the file PxeBcSupport.c of NetworkPkg there is
> 
> EFI_STATUS
> PxeBcUdp4Write (
> {
>             ...
>             //
>             // Arrange one fragment buffer for data, and another fragment buffer
> for header if has.
>             //
>             FragCount = (HeaderSize != NULL) ? 2 : 1;
>             ...
>             TxData->FragmentTable[FragCount - 1].FragmentLength = (UINT32)
> *BufferSize;
>             ...
> }
> 
> And similarly in
> 
> EFI_STATUS
> PxeBcUdp6Write (
> {
>             ...
>             //
>             // Arrange one fragment buffer for data, and another fragment buffer
> for header if has.
>             //
>             FragCount = (HeaderSize != NULL) ? 2 : 1;
>             ...
>             TxData->FragmentTable[FragCount - 1].FragmentLength = (UINT32)
> *BufferSize;
>             ...
> }
> 
> If HeaderSize is not NULL, then there is a chance of writing array over bounds
> since FragmentTable is of single element.
> 
> ///
>   /// Array of fragment descriptors.
>   ///
>   EFI_UDP6_FRAGMENT_DATA    FragmentTable[1];
> 
> Shouldn't we be taking care of this?
> 
> 
> Regards,
> Naveen
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel