* [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
@ 2018-10-27 12:27 Hao Wu
2018-10-27 13:19 ` Zeng, Star
0 siblings, 1 reply; 2+ messages in thread
From: Hao Wu @ 2018-10-27 12:27 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Ruiyu Ni, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
V2 change:
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: 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>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 38 ++++++++++++++++++--
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index b9ebddfe62..971f562ae2 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2144,6 +2144,10 @@ ResolveSymlink (
UINTN Index;
UINT8 CompressionId;
UDF_FILE_INFO PreviousFile;
+ BOOLEAN NotParent;
+ BOOLEAN NotFile;
+
+ ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO));
//
// Symlink files on UDF volumes do not contain so much data other than
@@ -2257,6 +2261,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,12 +2292,26 @@ 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;
+ }
+
+ 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));
+ }
}
//
@@ -2294,6 +2319,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] 2+ messages in thread
* Re: [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
2018-10-27 12:27 [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() Hao Wu
@ 2018-10-27 13:19 ` Zeng, Star
0 siblings, 0 replies; 2+ messages in thread
From: Zeng, Star @ 2018-10-27 13:19 UTC (permalink / raw)
To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Wu, Hao A
Sent: Saturday, October 27, 2018 8:28 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
V2 change:
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: 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>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 38 ++++++++++++++++++--
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index b9ebddfe62..971f562ae2 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2144,6 +2144,10 @@ ResolveSymlink (
UINTN Index;
UINT8 CompressionId;
UDF_FILE_INFO PreviousFile;
+ BOOLEAN NotParent;
+ BOOLEAN NotFile;
+
+ ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO));
//
// Symlink files on UDF volumes do not contain so much data other than @@ -2257,6 +2261,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,12 +2292,26 @@ 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;
+ }
+
+ 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));
+ }
}
//
@@ -2294,6 +2319,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] 2+ messages in thread
end of thread, other threads:[~2018-10-27 13:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-27 12:27 [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() Hao Wu
2018-10-27 13:19 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox