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

[Paulo Alcantara <paulo@paulo.ac>]

Hao Wu <hao.a.wu@intel.com> writes:

> 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.
>
> 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:

Reviewed-by: Paulo Alcantara <palcantara@suse.de>

Thanks!
Paulo