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

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink()
@ 2018-10-30  1:26 Hao Wu
  2018-10-30  1:26 ` [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component Hao Wu
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Hao Wu @ 2018-10-30  1:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Leif Lindholm, Ruiyu Ni

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component
  2018-10-30  1:26 [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Hao Wu
@ 2018-10-30  1:26 ` Hao Wu
  2018-10-30  1:40   ` Paulo Alcantara
  2018-10-30  1:26 ` [PATCH v3 2/3] MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink() Hao Wu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Hao Wu @ 2018-10-30  1:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Leif Lindholm, Ruiyu Ni

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component
  2018-10-30  1:26 ` [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component Hao Wu
@ 2018-10-30  1:40   ` Paulo Alcantara
  2018-10-30 10:27     ` Wu, Hao A
  0 siblings, 1 reply; 7+ messages in thread
From: Paulo Alcantara @ 2018-10-30  1:40 UTC (permalink / raw)
  To: edk2-devel, Hao Wu; +Cc: Ruiyu Ni

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component
  2018-10-30  1:40   ` Paulo Alcantara
@ 2018-10-30 10:27     ` Wu, Hao A
  0 siblings, 0 replies; 7+ messages in thread
From: Wu, Hao A @ 2018-10-30 10:27 UTC (permalink / raw)
  To: Paulo Alcantara, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 2/3] MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink()
  2018-10-30  1:26 [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Hao Wu
  2018-10-30  1:26 ` [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component Hao Wu
@ 2018-10-30  1:26 ` Hao Wu
  2018-10-30  1:26 ` [PATCH v3 3/3] MdeModulePkg/UdfDxe: Memory free/use after free " Hao Wu
  2018-10-30  9:27 ` [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Leif Lindholm
  3 siblings, 0 replies; 7+ messages in thread
From: Hao Wu @ 2018-10-30  1:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Leif Lindholm, Ruiyu Ni

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 3/3] MdeModulePkg/UdfDxe: Memory free/use after free in ResolveSymlink()
  2018-10-30  1:26 [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Hao Wu
  2018-10-30  1:26 ` [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component Hao Wu
  2018-10-30  1:26 ` [PATCH v3 2/3] MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink() Hao Wu
@ 2018-10-30  1:26 ` Hao Wu
  2018-10-30  9:27 ` [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Leif Lindholm
  3 siblings, 0 replies; 7+ messages in thread
From: Hao Wu @ 2018-10-30  1:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Leif Lindholm, Ruiyu Ni

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink()
  2018-10-30  1:26 [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Hao Wu
                   ` (2 preceding siblings ...)
  2018-10-30  1:26 ` [PATCH v3 3/3] MdeModulePkg/UdfDxe: Memory free/use after free " Hao Wu
@ 2018-10-30  9:27 ` Leif Lindholm
  3 siblings, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2018-10-30  9:27 UTC (permalink / raw)
  To: Hao Wu; +Cc: edk2-devel, Ruiyu Ni

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
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-10-30 10:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-30  1:26 [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Hao Wu
2018-10-30  1:26 ` [PATCH v3 1/3] MdeModulePkg/UdfDxe: Check 'Component Type' within a Path Component Hao Wu
2018-10-30  1:40   ` Paulo Alcantara
2018-10-30 10:27     ` Wu, Hao A
2018-10-30  1:26 ` [PATCH v3 2/3] MdeModulePkg/UdfDxe: Content check for 'File' in ResolveSymlink() Hao Wu
2018-10-30  1:26 ` [PATCH v3 3/3] MdeModulePkg/UdfDxe: Memory free/use after free " Hao Wu
2018-10-30  9:27 ` [PATCH v3 0/3] UdfDxe: Additional checks for ResolveSymlink() Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox