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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leif Lindholm
> Sent: Monday, October 29, 2018 7:10 PM
> To: Wu, Hao A
> Cc: Ni, Ruiyu; Paulo Alcantara; edk2-devel@lists.01.org; Zeng, Star
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Additional checks for
> ResolveSymlink()
> 
> Hi Hao Wu,
> 
> I have no comment about the code changes, but would it be possible to
> split this up into three separate patches? They are three separate
> changes, but as it stands, it is not _obvious_ that they do not have
> interdependencies. Splitting them up would make that immediately
> obvious. You already have the three commit messages :)

Thanks Leif,

Yes, I will send a new series to address it.

Best Regards,
Hao Wu

> 
> Best Regards,
> 
> Leif
> 
> On Fri, Oct 26, 2018 at 03:54:57PM +0800, 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.
> >
> > 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:
> > --
> > 2.12.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel