Re: [PATCH] ShellPkg/hexedit: Fix a read-after-free bug

["Carsey, Jaben" <jaben.carsey@intel.com>]

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, February 07, 2018 8:45 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: [PATCH] ShellPkg/hexedit: Fix a read-after-free bug
> Importance: High
> 
> HDiskImageSetDiskNameOffsetSize() and HFileImageSetFileName()
> may be called using the current disk name or file name.
> When this happens, today's implementation firstly frees the memory
> and then accesses the just-freed memory.
> The patch fixes this issue by doing nothing when the disk or file
> name is the current one.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> ---
>  .../UefiShellDebug1CommandsLib/HexEdit/DiskImage.c | 22 +++++++++-----
> -------
>  .../UefiShellDebug1CommandsLib/HexEdit/FileImage.c | 23 +++++++++------
> -------
>  2 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> index 846b102975..8deb643f07 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Functions to deal with Disk buffer.
> 
> -  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved. <BR>
> +  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. <BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at
> @@ -120,27 +120,23 @@ HDiskImageSetDiskNameOffsetSize (
>    IN UINTN    Size
>    )
>  {
> -  UINTN Len;
> -  UINTN Index;
> +  if (Str == HDiskImage.Name) {
> +    //
> +    // This function might be called using HDiskImage.FileName as Str.
> +    // Directly return without updating HDiskImage.FileName.
> +    //
> +    return EFI_SUCCESS;
> +  }
> 
>    //
>    // free the old file name
>    //
>    SHELL_FREE_NON_NULL (HDiskImage.Name);
> -
> -  Len             = StrLen (Str);
> -
> -  HDiskImage.Name = AllocateZeroPool (2 * (Len + 1));
> +  HDiskImage.Name = AllocateCopyPool (StrSize (Str), Str);
>    if (HDiskImage.Name == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
> -  for (Index = 0; Index < Len; Index++) {
> -    HDiskImage.Name[Index] = Str[Index];
> -  }
> -
> -  HDiskImage.Name[Len]  = L'\0';
> -
>    HDiskImage.Offset     = Offset;
>    HDiskImage.Size       = Size;
> 
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c
> index 2517a57f59..d9fd72cdd2 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Functions to deal with file buffer.
> 
> -  Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved. <BR>
> +  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. <BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at
> @@ -110,27 +110,22 @@ HFileImageSetFileName (
>    IN CONST CHAR16 *Str
>    )
>  {
> -  UINTN Size;
> -  UINTN Index;
> -
> +  if (Str == HFileImage.FileName) {
> +    //
> +    // This function might be called using HFileImage.FileName as Str.
> +    // Directly return without updating HFileImage.FileName.
> +    //
> +    return EFI_SUCCESS;
> +  }
>    //
>    // free the old file name
>    //
>    SHELL_FREE_NON_NULL (HFileImage.FileName);
> -
> -  Size                = StrLen (Str);
> -
> -  HFileImage.FileName = AllocateZeroPool (2 * (Size + 1));
> +  HFileImage.FileName = AllocateCopyPool (StrSize (Str), Str);
>    if (HFileImage.FileName == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
> -  for (Index = 0; Index < Size; Index++) {
> -    HFileImage.FileName[Index] = Str[Index];
> -  }
> -
> -  HFileImage.FileName[Size] = L'\0';
> -
>    return EFI_SUCCESS;
>  }
> 
> --
> 2.16.1.windows.1