From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9D35C2194D3B8 for ; Tue, 16 Oct 2018 00:23:46 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Oct 2018 00:23:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,387,1534834800"; d="scan'208";a="97812128" Received: from shwdeopenpsi014.ccr.corp.intel.com ([10.239.9.9]) by fmsmga004.fm.intel.com with ESMTP; 16 Oct 2018 00:23:45 -0700 From: Hao Wu To: edk2-devel@lists.01.org Cc: Hao Wu , Paulo Alcantara , Ruiyu Ni , Jiewen Yao , Star Zeng Date: Tue, 16 Oct 2018 15:23:32 +0800 Message-Id: <20181016072340.22068-3-hao.a.wu@intel.com> X-Mailer: git-send-email 2.12.0.windows.1 In-Reply-To: <20181016072340.22068-1-hao.a.wu@intel.com> References: <20181016072340.22068-1-hao.a.wu@intel.com> Subject: [PATCH v1 02/10] MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Oct 2018 07:23:46 -0000 REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828 The commit refines the boundary checks for file/path name string to prevent possible buffer overrun. Cc: Paulo Alcantara Cc: Ruiyu Ni Cc: Jiewen Yao Cc: Star Zeng Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Hao Wu --- MdeModulePkg/Universal/Disk/UdfDxe/File.c | 29 +++++++-- MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 64 +++++++++++++++++--- MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 29 ++++++++- 3 files changed, 107 insertions(+), 15 deletions(-) diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c index 6f07bf2066..80db734f86 100644 --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c @@ -248,7 +248,7 @@ UdfOpen ( FileName = TempFileName + 1; } - StrCpyS (NewPrivFileData->FileName, UDF_PATH_LENGTH, FileName); + StrCpyS (NewPrivFileData->FileName, UDF_FILENAME_LENGTH, FileName); Status = GetFileSize ( PrivFsData->BlockIo, @@ -444,7 +444,7 @@ UdfRead ( FreePool ((VOID *)NewFileEntryData); NewFileEntryData = FoundFile.FileEntry; - Status = GetFileNameFromFid (NewFileIdentifierDesc, FileName); + Status = GetFileNameFromFid (NewFileIdentifierDesc, ARRAY_SIZE (FileName), FileName); if (EFI_ERROR (Status)) { FreePool ((VOID *)FoundFile.FileIdentifierDesc); goto Error_Get_FileName; @@ -456,7 +456,7 @@ UdfRead ( FoundFile.FileIdentifierDesc = NewFileIdentifierDesc; FoundFile.FileEntry = NewFileEntryData; - Status = GetFileNameFromFid (FoundFile.FileIdentifierDesc, FileName); + Status = GetFileNameFromFid (FoundFile.FileIdentifierDesc, ARRAY_SIZE (FileName), FileName); if (EFI_ERROR (Status)) { goto Error_Get_FileName; } @@ -718,6 +718,12 @@ UdfSetPosition ( /** Get information about a file. + @attention This is boundary function that may receive untrusted input. + @attention The input is from FileSystem. + + The File Set Descriptor is external input, so this routine will do basic + validation for File Set Descriptor and report status. + @param This Protocol instance pointer. @param InformationType Type of information to return in Buffer. @param BufferSize On input size of buffer, on output amount of data in @@ -794,6 +800,10 @@ UdfGetInfo ( *String = *(UINT8 *)(OstaCompressed + Index) << 8; Index++; } else { + if (Index > ARRAY_SIZE (VolumeLabel)) { + return EFI_VOLUME_CORRUPTED; + } + *String = 0; } @@ -813,7 +823,11 @@ UdfGetInfo ( String++; } - *String = L'\0'; + Index = ((UINTN)String - (UINTN)VolumeLabel) / sizeof (CHAR16); + if (Index > ARRAY_SIZE (VolumeLabel) - 1) { + Index = ARRAY_SIZE (VolumeLabel) - 1; + } + VolumeLabel[Index] = L'\0'; FileSystemInfoLength = StrSize (VolumeLabel) + sizeof (EFI_FILE_SYSTEM_INFO); @@ -823,8 +837,11 @@ UdfGetInfo ( } FileSystemInfo = (EFI_FILE_SYSTEM_INFO *)Buffer; - StrCpyS (FileSystemInfo->VolumeLabel, ARRAY_SIZE (VolumeLabel), - VolumeLabel); + StrCpyS ( + FileSystemInfo->VolumeLabel, + (*BufferSize - OFFSET_OF (EFI_FILE_SYSTEM_INFO, VolumeLabel)) / sizeof (CHAR16), + VolumeLabel + ); Status = GetVolumeSize ( PrivFsData->BlockIo, PrivFsData->DiskIo, diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c index ecc172303e..5fb88c2ff3 100644 --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c @@ -1412,7 +1412,7 @@ InternalFindFile ( break; } } else { - Status = GetFileNameFromFid (FileIdentifierDesc, FoundFileName); + Status = GetFileNameFromFid (FileIdentifierDesc, ARRAY_SIZE (FoundFileName), FoundFileName); if (EFI_ERROR (Status)) { break; } @@ -1705,6 +1705,11 @@ FindFile ( while (*FilePath != L'\0') { FileNamePointer = FileName; while (*FilePath != L'\0' && *FilePath != L'\\') { + if ((((UINTN)FileNamePointer - (UINTN)FileName) / sizeof (CHAR16)) >= + (ARRAY_SIZE (FileName) - 1)) { + return EFI_NOT_FOUND; + } + *FileNamePointer++ = *FilePath++; } @@ -1882,22 +1887,38 @@ ReadDirectoryEntry ( Get a filename (encoded in OSTA-compressed format) from a File Identifier Descriptor on an UDF volume. + @attention This is boundary function that may receive untrusted input. + @attention The input is from FileSystem. + + The File Identifier Descriptor is external input, so this routine will do + basic validation for File Identifier Descriptor and report status. + @param[in] FileIdentifierDesc File Identifier Descriptor pointer. + @param[in] CharMax The maximum number of FileName Unicode char, + including terminating null char. @param[out] FileName Decoded filename. @retval EFI_SUCCESS Filename decoded and read. @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. + @retval EFI_BUFFER_TOO_SMALL The string buffer FileName cannot hold the + decoded filename. **/ EFI_STATUS GetFileNameFromFid ( IN UDF_FILE_IDENTIFIER_DESCRIPTOR *FileIdentifierDesc, + IN UINTN CharMax, OUT CHAR16 *FileName ) { - UINT8 *OstaCompressed; - UINT8 CompressionId; - UINT8 Length; - UINTN Index; + UINT8 *OstaCompressed; + UINT8 CompressionId; + UINT8 Length; + UINTN Index; + CHAR16 *FileNameBak; + + if (CharMax == 0) { + return EFI_BUFFER_TOO_SMALL; + } OstaCompressed = (UINT8 *)( @@ -1910,10 +1931,22 @@ GetFileNameFromFid ( return EFI_VOLUME_CORRUPTED; } + FileNameBak = FileName; + // // Decode filename. // Length = FileIdentifierDesc->LengthOfFileIdentifier; + if (CompressionId == 16) { + if (((UINTN)Length >> 1) > CharMax) { + return EFI_BUFFER_TOO_SMALL; + } + } else { + if ((Length != 0) && ((UINTN)Length - 1 > CharMax)) { + return EFI_BUFFER_TOO_SMALL; + } + } + for (Index = 1; Index < Length; Index++) { if (CompressionId == 16) { *FileName = OstaCompressed[Index++] << 8; @@ -1928,7 +1961,11 @@ GetFileNameFromFid ( FileName++; } - *FileName = L'\0'; + Index = ((UINTN)FileName - (UINTN)FileNameBak) / sizeof (CHAR16); + if (Index > CharMax - 1) { + Index = CharMax - 1; + } + FileNameBak[Index] = L'\0'; return EFI_SUCCESS; } @@ -1936,6 +1973,12 @@ GetFileNameFromFid ( /** Resolve a symlink file on an UDF volume. + @attention This is boundary function that may receive untrusted input. + @attention The input is from FileSystem. + + The Path Component is external input, so this routine will do basic + validation for Path Component and report status. + @param[in] BlockIo BlockIo interface. @param[in] DiskIo DiskIo interface. @param[in] Volume UDF volume information structure. @@ -2054,6 +2097,9 @@ ResolveSymlink ( Index) << 8; Index++; } else { + if (Index > ARRAY_SIZE (FileName)) { + return EFI_UNSUPPORTED; + } *Char = 0; } @@ -2064,7 +2110,11 @@ ResolveSymlink ( Char++; } - *Char = L'\0'; + Index = ((UINTN)Char - (UINTN)FileName) / sizeof (CHAR16); + if (Index > ARRAY_SIZE (FileName) - 1) { + Index = ARRAY_SIZE (FileName) - 1; + } + FileName[Index] = L'\0'; break; } diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h index d441539d16..85bf5e7733 100644 --- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h +++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h @@ -559,9 +559,16 @@ UdfSetPosition ( /** Get information about a file. + @attention This is boundary function that may receive untrusted input. + @attention The input is from FileSystem. + + The File Set Descriptor is external input, so this routine will do basic + validation for File Set Descriptor and report status. + @param This Protocol instance pointer. @param InformationType Type of information to return in Buffer. - @param BufferSize On input size of buffer, on output amount of data in buffer. + @param BufferSize On input size of buffer, on output amount of data in + buffer. @param Buffer The buffer to return data. @retval EFI_SUCCESS Data was returned. @@ -571,7 +578,8 @@ UdfSetPosition ( @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. @retval EFI_WRITE_PROTECTED The device is write protected. @retval EFI_ACCESS_DENIED The file was open for read only. - @retval EFI_BUFFER_TOO_SMALL Buffer was too small; required size returned in BufferSize. + @retval EFI_BUFFER_TOO_SMALL Buffer was too small; required size returned in + BufferSize. **/ EFI_STATUS @@ -769,21 +777,38 @@ ReadDirectoryEntry ( Get a filename (encoded in OSTA-compressed format) from a File Identifier Descriptor on an UDF volume. + @attention This is boundary function that may receive untrusted input. + @attention The input is from FileSystem. + + The File Identifier Descriptor is external input, so this routine will do + basic validation for File Identifier Descriptor and report status. + @param[in] FileIdentifierDesc File Identifier Descriptor pointer. + @param[in] CharMax The maximum number of FileName Unicode char, + including terminating null char. @param[out] FileName Decoded filename. @retval EFI_SUCCESS Filename decoded and read. @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted. + @retval EFI_BUFFER_TOO_SMALL The string buffer FileName cannot hold the + decoded filename. **/ EFI_STATUS GetFileNameFromFid ( IN UDF_FILE_IDENTIFIER_DESCRIPTOR *FileIdentifierDesc, + IN UINTN CharMax, OUT CHAR16 *FileName ); /** Resolve a symlink file on an UDF volume. + @attention This is boundary function that may receive untrusted input. + @attention The input is from FileSystem. + + The Path Component is external input, so this routine will do basic + validation for Path Component and report status. + @param[in] BlockIo BlockIo interface. @param[in] DiskIo DiskIo interface. @param[in] Volume UDF volume information structure. -- 2.12.0.windows.1