[edk2-devel] [edk2-platforms PATCH v2 5/5] Ext4Pkg: Sanity check more EXT4_DIR_ENTRY values.
Pedro Falcato
pedro.falcato at gmail.com
Sat Aug 21 14:47:10 UTC 2021
This should close up some possible exploits using crafted
filesystem images.
Cc: Leif Lindholm <leif at nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney at intel.com>
Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
Signed-off-by: Pedro Falcato <pedro.falcato at gmail.com>
---
Features/Ext4Pkg/Ext4Dxe/Directory.c | 90 ++++++++++++++++------------
1 file changed, 51 insertions(+), 39 deletions(-)
diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 7d1b2dcfe524..102c82f05da0 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -50,6 +50,37 @@ Ext4GetUcs2DirentName (
return Status;
}
+/**
+ Validates a directory entry.
+
+ @param[in] Dirent Pointer to the directory entry.
+
+ @retval TRUE Valid directory entry.
+ FALSE Invalid directory entry.
+**/
+STATIC
+BOOLEAN
+Ext4ValidDirent (
+ IN CONST EXT4_DIR_ENTRY *Dirent
+ )
+{
+ UINTN RequiredSize;
+
+ RequiredSize = Dirent->name_len + EXT4_MIN_DIR_ENTRY_LEN;
+
+ if (Dirent->rec_len < RequiredSize) {
+ DEBUG ((DEBUG_ERROR, "[ext4] dirent size %lu too small (compared to %lu)\n", Dirent->rec_len, RequiredSize));
+ return FALSE;
+ }
+
+ // Dirent sizes need to be 4 byte aligned
+ if (Dirent->rec_len % 4) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
/**
Retrieves a directory entry.
@@ -75,11 +106,11 @@ Ext4RetrieveDirent (
UINT64 DirInoSize;
UINT32 BlockRemainder;
UINTN Length;
- CHAR8 *BufPtr;
EXT4_DIR_ENTRY *Entry;
UINTN RemainingBlock;
CHAR16 DirentUcs2Name[EXT4_NAME_MAX + 1];
UINTN ToCopy;
+ UINTN BlockOffset;
Status = EFI_NOT_FOUND;
Buf = AllocatePool (Partition->BlockSize);
@@ -109,14 +140,19 @@ Ext4RetrieveDirent (
return Status;
}
- for (BufPtr = Buf; BufPtr < Buf + Partition->BlockSize; ) {
- Entry = (EXT4_DIR_ENTRY *)BufPtr;
- if (Entry->rec_len == 0) {
+ for (BlockOffset = 0; BlockOffset < Partition->BlockSize; ) {
+ Entry = (EXT4_DIR_ENTRY *)(Buf + BlockOffset);
+ RemainingBlock = Partition->BlockSize - BlockOffset;
+ // Check if the minimum directory entry fits inside [BlockOffset, EndOfBlock]
+ if (RemainingBlock < EXT4_MIN_DIR_ENTRY_LEN) {
FreePool (Buf);
return EFI_VOLUME_CORRUPTED;
}
- RemainingBlock = Partition->BlockSize - (BufPtr - Buf);
+ if (!Ext4ValidDirent (Entry)) {
+ FreePool (Buf);
+ return EFI_VOLUME_CORRUPTED;
+ }
if (Entry->name_len > RemainingBlock || Entry->rec_len > RemainingBlock) {
// Corrupted filesystem
@@ -131,12 +167,13 @@ Ext4RetrieveDirent (
2) Linux and a number of BSDs also have a filename limit of 255.
*/
if (Entry->name_len > EXT4_NAME_MAX) {
+ BlockOffset += Entry->rec_len;
continue;
}
// Unused entry
if (Entry->inode == 0) {
- BufPtr += Entry->rec_len;
+ BlockOffset += Entry->rec_len;
continue;
}
@@ -150,7 +187,7 @@ Ext4RetrieveDirent (
if (EFI_ERROR (Status)) {
// If we error out, skip this entry
// I'm not sure if this is correct behaviour, but I don't think there's a precedent here.
- BufPtr += Entry->rec_len;
+ BlockOffset += Entry->rec_len;
continue;
}
@@ -163,7 +200,7 @@ Ext4RetrieveDirent (
return EFI_SUCCESS;
}
- BufPtr += Entry->rec_len;
+ BlockOffset += Entry->rec_len;
}
Off += Partition->BlockSize;
@@ -379,37 +416,6 @@ Ext4OpenVolume (
return EFI_SUCCESS;
}
-/**
- Validates a directory entry.
-
- @param[in] Dirent Pointer to the directory entry.
-
- @retval TRUE Valid directory entry.
- FALSE Invalid directory entry.
-**/
-STATIC
-BOOLEAN
-Ext4ValidDirent (
- IN CONST EXT4_DIR_ENTRY *Dirent
- )
-{
- UINTN RequiredSize;
-
- RequiredSize = Dirent->name_len + EXT4_MIN_DIR_ENTRY_LEN;
-
- if (Dirent->rec_len < RequiredSize) {
- DEBUG ((DEBUG_ERROR, "[ext4] dirent size %lu too small (compared to %lu)\n", Dirent->rec_len, RequiredSize));
- return FALSE;
- }
-
- // Dirent sizes need to be 4 byte aligned
- if (Dirent->rec_len % 4) {
- return FALSE;
- }
-
- return TRUE;
-}
-
/**
Reads a directory entry.
@@ -481,6 +487,12 @@ Ext4ReadDir (
goto Out;
}
+ // Check if the entire dir entry length fits in Len
+ if (Len < EXT4_MIN_DIR_ENTRY_LEN + Entry.name_len) {
+ Status = EFI_VOLUME_CORRUPTED;
+ goto Out;
+ }
+
// We don't care about passing . or .. entries to the caller of ReadDir(),
// since they're generally useless entries *and* may break things if too
// many callers assume FAT32.
--
2.33.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79666): https://edk2.groups.io/g/devel/message/79666
Mute This Topic: https://groups.io/mt/85043015/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list