Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()

["Zeng, Star" <star.zeng@intel.com>]

Hao,

On 2018/10/26 15:54, Hao Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
> 
> The commit will add 3 types of checks for function ResolveSymlink():
> 
> A. Check for the value of 'Component Type' field within a Path Component
> 
> According to the ECMA-167 standard (3rd Edition - June 1997), Section
> 14.16.1.1, valid values are 1 to 5. All other values will be treated as a
> corrupted volume.
> 
> B. Check for the content pointed by 'File'
> 
> Since content within 'File' is the output data for ResolveSymlink().
> Checks is added to ensure the content in 'File' is valid. Otherwise,
> possible null pointer dereference issue will occur during the subsequent
> usage of the data returned by ResolveSymlink().
> 
> C. Check for possible memory double free/use after free case
> 
> For codes:
> 
>      if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
>                      sizeof (UDF_FILE_INFO)) != 0) {
>        CleanupFileInformation (&PreviousFile);
>      }
> 
>      CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));
> 
> If the contents in 'PreviousFile' and 'File' are the same, call to
> "CleanupFileInformation (&PreviousFile);" will free the buffers in 'File'
> as well. This will lead to potential memory double free/use after free
> issues.

If 'PreviousFile' and 'File' are the same, the coping operation below 
also has no need to be done, right?

CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));

Thanks,
Star

> 
> Cc: Paulo Alcantara <paulo@paulo.ac>
> Cc: Paulo Alcantara <palcantara@suse.de>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>   .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 30 ++++++++++++++++++++--
>   1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index b9ebddfe62..a89e5ba9ff 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -2145,6 +2145,8 @@ ResolveSymlink (
>     UINT8               CompressionId;
>     UDF_FILE_INFO       PreviousFile;
>   
> +  ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO));
> +
>     //
>     // Symlink files on UDF volumes do not contain so much data other than
>     // Path Components which resolves to real filenames, so it's OK to read in
> @@ -2257,6 +2259,13 @@ ResolveSymlink (
>         }
>         FileName[Index] = L'\0';
>         break;
> +    default:
> +      //
> +      // Accoring to the ECMA-167 standard (3rd Edition - June 1997), Section
> +      // 14.16.1.1, all other values are reserved.
> +      //
> +      Status = EFI_VOLUME_CORRUPTED;
> +      goto Error_Find_File;
>       }
>   
>       //
> @@ -2281,8 +2290,18 @@ ResolveSymlink (
>         break;
>       }
>   
> -    if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
> -                    sizeof (UDF_FILE_INFO)) != 0) {
> +    //
> +    // Check the content in the file info pointed by File.
> +    //
> +    if ((File->FileEntry == NULL) || (File->FileIdentifierDesc == NULL)) {
> +      Status = EFI_VOLUME_CORRUPTED;
> +      goto Error_Find_File;
> +    }
> +
> +    if ((CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
> +                    sizeof (UDF_FILE_INFO)) != 0) &&
> +        (CompareMem ((VOID *)&PreviousFile, (VOID *)File,
> +                    sizeof (UDF_FILE_INFO)) != 0)) {
>         CleanupFileInformation (&PreviousFile);
>       }
>   
> @@ -2294,6 +2313,13 @@ ResolveSymlink (
>     //
>     FreePool (ReadFileInfo.FileData);
>   
> +  //
> +  // Check the content in the resolved file info.
> +  //
> +  if ((File->FileEntry == NULL) || (File->FileIdentifierDesc == NULL)) {
> +    return EFI_VOLUME_CORRUPTED;
> +  }
> +
>     return EFI_SUCCESS;
>   
>   Error_Find_File:
>