[PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink()

[PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink()

[Hao Wu]

V3 changes:

According to Leif's recommendation, split the original patch into 3
seperate ones.

Since there is no code changes compared with the V2 of the patch, I just
preserved the 'Reviewed-by' tags by Paulo and Star.

V2 history:

Refine type C check (refer to V1 history below) to eliminate the
unnecessary CopyMem() call.

V1 history:

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: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>

Hao Wu (3):
  MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component
  MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink()
  MdeModulePkg/UdfDxe: Memory free/use after free in ResolveSymlink()

 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 38 ++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

-- 
2.12.0.windows.1

[PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279

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.

This commit will add such check within function ResolveSymlink().

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index b9ebddfe62..c15741a032 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2257,6 +2257,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;
     }
 
     //
-- 
2.12.0.windows.1

[PATCH v3 2/3] MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink()

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279

The content within 'File' is the output data for ResolveSymlink(). This
commit will add checks 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().

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index c15741a032..2227f10d07 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
@@ -2288,6 +2290,14 @@ ResolveSymlink (
       break;
     }
 
+    //
+    // 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) {
       CleanupFileInformation (&PreviousFile);
@@ -2301,6 +2311,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:
-- 
2.12.0.windows.1

[PATCH v3 3/3] MdeModulePkg/UdfDxe: Memory free/use after free in ResolveSymlink()

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279

For function ResolveSymlink(), the below 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.

This commit will add additional check to address the above issue.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 2227f10d07..971f562ae2 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2144,6 +2144,8 @@ ResolveSymlink (
   UINTN               Index;
   UINT8               CompressionId;
   UDF_FILE_INFO       PreviousFile;
+  BOOLEAN             NotParent;
+  BOOLEAN             NotFile;
 
   ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO));
 
@@ -2298,12 +2300,18 @@ ResolveSymlink (
       goto Error_Find_File;
     }
 
-    if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
-                    sizeof (UDF_FILE_INFO)) != 0) {
+    NotParent = (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
+                 sizeof (UDF_FILE_INFO)) != 0);
+    NotFile   = (CompareMem ((VOID *)&PreviousFile, (VOID *)File,
+                 sizeof (UDF_FILE_INFO)) != 0);
+
+    if (NotParent && NotFile) {
       CleanupFileInformation (&PreviousFile);
     }
 
-    CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));
+    if (NotFile) {
+      CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));
+    }
   }
 
   //
-- 
2.12.0.windows.1

Re: [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component

[Paulo Alcantara]

Hi Hao Wu,

On October 29, 2018 10:26:15 PM GMT-03:00, Hao Wu <hao.a.wu@intel.com> wrote:
>REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
>
>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.
>
>This commit will add such check within function ResolveSymlink().
>
>Cc: Leif Lindholm <leif.lindholm@linaro.org>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>Reviewed-by: Paulo Alcantara <palcantara@suse.de>
>Reviewed-by: Star Zeng <star.zeng@intel.com>
>---
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>index b9ebddfe62..c15741a032 100644
>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>@@ -2257,6 +2257,13 @@ ResolveSymlink (
>       }
>       FileName[Index] = L'\0';
>       break;
>+    default:
>+      //
>+      // Accoring to the ECMA-167 standard (3rd Edition - June 1997),
>Section

Minor typo: s/Accoring/According/

Paulo

>+      // 14.16.1.1, all other values are reserved.
>+      //
>+      Status = EFI_VOLUME_CORRUPTED;
>+      goto Error_Find_File;
>     }
> 
>     //

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink()

[Leif Lindholm]

Many thanks for the rework. For the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

On Tue, Oct 30, 2018 at 09:26:14AM +0800, Hao Wu wrote:
> V3 changes:
> 
> According to Leif's recommendation, split the original patch into 3
> seperate ones.
> 
> Since there is no code changes compared with the V2 of the patch, I just
> preserved the 'Reviewed-by' tags by Paulo and Star.
> 
> V2 history:
> 
> Refine type C check (refer to V1 history below) to eliminate the
> unnecessary CopyMem() call.
> 
> V1 history:
> 
> 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: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> Hao Wu (3):
>   MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component
>   MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink()
>   MdeModulePkg/UdfDxe: Memory free/use after free in ResolveSymlink()
> 
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 38 ++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> -- 
> 2.12.0.windows.1
> 

Re: [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component

[Wu, Hao A]

> -----Original Message-----
> From: Paulo Alcantara [mailto:paulo@paulo.ac]
> Sent: Tuesday, October 30, 2018 9:41 AM
> To: edk2-devel@lists.01.org; Wu, Hao A
> Cc: Ni, Ruiyu
> Subject: Re: [edk2] [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check
> 'Component Type' within a Path Component
> 
> Hi Hao Wu,
> 
> On October 29, 2018 10:26:15 PM GMT-03:00, Hao Wu
> <hao.a.wu@intel.com> wrote:
> >REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
> >
> >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.
> >
> >This commit will add such check within function ResolveSymlink().
> >
> >Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> >Reviewed-by: Paulo Alcantara <palcantara@suse.de>
> >Reviewed-by: Star Zeng <star.zeng@intel.com>
> >---
> > MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 7
> +++++++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> >b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> >index b9ebddfe62..c15741a032 100644
> >--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> >+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> >@@ -2257,6 +2257,13 @@ ResolveSymlink (
> >       }
> >       FileName[Index] = L'\0';
> >       break;
> >+    default:
> >+      //
> >+      // Accoring to the ECMA-167 standard (3rd Edition - June 1997),
> >Section
> 
> Minor typo: s/Accoring/According/

Thanks for the catch, Paulo.

I will address this before pushing the codes.

Best Regards,
Hao Wu

> 
> Paulo
> 
> >+      // 14.16.1.1, all other values are reserved.
> >+      //
> >+      Status = EFI_VOLUME_CORRUPTED;
> >+      goto Error_Find_File;
> >     }
> >
> >     //
> 
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.