Lz retdec 54 (#981)

* Added more checks for delay import directory

* Hardened resilience against malformed delayed import directories

* Removed unnecessary file

* Iconhash problem fixed

* Fixed getRealSizeOfRawData()

Co-authored-by: Ladislav Zezula <ladislav.zezula@avast.com>

include/retdec/fileformat/file_format/pe/pe_format_parser.h

@@ -265,8 +265,8 @@ class PeFormatParser
 		section.setType(sectionType);
 		section.setIndex(secIndex);
 		section.setOffset(imageLoader.getRealPointerToRawData(secIndex));
-		section.setSizeInFile(pSectionHeader->SizeOfRawData);
-		section.setSizeInMemory(pSectionHeader->VirtualSize);
+		section.setSizeInFile(imageLoader.getRealSizeOfRawData(secIndex));
+		section.setSizeInMemory((pSectionHeader->VirtualSize != 0) ? pSectionHeader->VirtualSize : pSectionHeader->SizeOfRawData);
 		section.setAddress(pSectionHeader->VirtualAddress ? imageLoader.getImageBase() + pSectionHeader->VirtualAddress : 0);
 		section.setMemory(section.getAddress());
 		section.setPeCoffFlags(pSectionHeader->Characteristics);

include/retdec/fileformat/types/resource_table/resource_icon.h

@@ -56,6 +56,8 @@ class ResourceIcon : public Resource
 		/// @{
 		bool hasLoadedProperties() const;
 		bool hasValidColorCount() const;
+		bool hasValidDimensions() const;
+
 		/// @}
 };
 

include/retdec/pelib/DelayImportDirectory.h

@@ -102,7 +102,7 @@ namespace PeLib
 					return ERROR_INVALID_FILE;
 				init();
 
-				// Keep loading until we encounter an entry filles with zeros
+				// Keep loading until we encounter an entry filled with zeros
 				for(std::uint32_t i = 0;; i += sizeof(PELIB_IMAGE_DELAY_LOAD_DESCRIPTOR))
 				{
 					PELIB_IMAGE_DELAY_IMPORT_DIRECTORY_RECORD rec;
@@ -113,8 +113,14 @@ namespace PeLib
 					if(!imageLoader.readImage(&rec.delayedImport, rva + i, sizeof(PELIB_IMAGE_DELAY_LOAD_DESCRIPTOR)))
 						break;
 
-					// Valid delayed import entry starts either with 0 or 0x01
-					if(rec.delayedImport.Attributes & 0xFFFF0000)
+					// Valid delayed import entry starts either with 0 or 0x01.
+					// We strict require one of the valid values here
+					if(rec.delayedImport.Attributes > PELIB_DELAY_ATTRIBUTE_V2)
+						break;
+
+					// Stop on blatantly invalid entries
+					if(rec.delayedImport.NameRva < sizeof(PELIB_IMAGE_DOS_HEADER) ||
+					   rec.delayedImport.DelayImportNameTableRva < sizeof(PELIB_IMAGE_DOS_HEADER))
 						break;
 
 					// Check for the termination entry

include/retdec/pelib/ImageLoader.h

@@ -172,6 +172,7 @@ class ImageLoader
 	std::uint32_t vaToRva(std::uint64_t VirtualAddress) const;
 	std::uint32_t getFileOffsetFromRva(std::uint32_t rva) const;
 	std::uint32_t getRealPointerToRawData(std::size_t sectionIndex) const;
+	std::uint32_t getRealSizeOfRawData(std::size_t sectionIndex) const;
 	std::uint32_t getImageProtection(std::uint32_t characteristics) const;
 	std::size_t   getSectionIndexByRva(std::uint32_t Rva) const;
 

src/fileformat/file_format/file_format.cpp

@@ -83,7 +83,7 @@ bool isAddressFromRegion(const SecSeg *actualRegion, const SecSeg *newRegion, st
 		return false;
 	}
 
-	unsigned long long newRegionSize;
+	unsigned long long newRegionSize = 0;
 	if(!newRegion->getSizeInMemory(newRegionSize))
 	{
 		newRegionSize = newRegion->getSizeInFile();

src/fileformat/types/resource_table/resource_icon.cpp

@@ -179,5 +179,15 @@ bool ResourceIcon::hasValidColorCount() const
 	return validColorCount;
 }
 
+/**
+* Returns tru if the icon dimensions were already set before
+* @return @c `true` if they were, otherwise `false`
+*/
+bool ResourceIcon::hasValidDimensions() const
+{
+	return (width && height);
+}
+
+
 } // namespace fileformat
 } // namespace retdec

src/fileformat/types/resource_table/resource_icon_group.cpp

@@ -39,27 +39,31 @@ constexpr std::pair<uint16_t, uint16_t> iconPriorities[] =
 bool iconCompare(const retdec::fileformat::ResourceIcon *i1, const retdec::fileformat::ResourceIcon *i2)
 {
 	auto i1Width = i1->getWidth();
+	auto i1Height = i1->getHeight();
 	auto i1BitCount = i1->getBitCount();
 	auto i2Width = i2->getWidth();
 	auto i2Height = i2->getHeight();
 	auto i2BitCount = i2->getBitCount();
 
-	if(i2Width != i2Height)
-	{
+	// Priority 1: icon with the same dimensions over icons 
+	if((i1Width == i1Height) && (i2Width != i2Height))
 		return false;
-	}
+	if((i1Width != i1Height) && (i2Width == i2Height))
+		return true;
 
 	for(const auto &p : iconPriorities)
 	{
+		// Priority 2: Icons where both size and bit count match
 		if(p.first == i1Width && p.second == i1BitCount)
-		{
 			return false;
-		}
-
 		if(p.first == i2Width && p.second == i2BitCount)
-		{
 			return true;
-		}
+
+		// Priority 3: Icons where size matches
+		if(p.first == i1Width)
+			return false;
+		if(p.first == i2Width)
+			return true;
 	}
 
 	return false;
@@ -183,8 +187,8 @@ bool ResourceIconGroup::getEntryWidth(std::size_t eIndex, std::uint16_t &width)
 		return false;
 	}
 
-	width = bytes[0];
-
+	if((width = bytes[0]) == 0)
+		width = 256;	// https://devblogs.microsoft.com/oldnewthing/20101018-00/?p=12513
 	return true;
 }
 
@@ -203,8 +207,8 @@ bool ResourceIconGroup::getEntryHeight(std::size_t eIndex, std::uint16_t &height
 		return false;
 	}
 
-	height = bytes[0];
-
+	if((height = bytes[0]) == 0)
+		height = 256;	// https://devblogs.microsoft.com/oldnewthing/20101018-00/?p=12513
 	return true;
 }
 

src/fileformat/types/resource_table/resource_table.cpp

@@ -781,6 +781,11 @@ void ResourceTable::linkResourceIconGroups()
 					continue;
 				}
 
+				// Multiple icon group may reference an icon. If that happens, do not rewrite
+				// icon dimensions. Doing so messes up with the icon hash, and we only care for the first icon anyway
+				if(icon->hasValidDimensions())
+					continue;
+
 				icon->setWidth(width);
 				icon->setHeight(height);
 				icon->setIconSize(iconSize);

src/pelib/ImageLoader.cpp

@@ -556,6 +556,18 @@ uint32_t PeLib::ImageLoader::getRealPointerToRawData(size_t sectionIndex) const
 	return sections[sectionIndex].PointerToRawData & ~(PELIB_SECTOR_SIZE - 1);
 }
 
+uint32_t PeLib::ImageLoader::getRealSizeOfRawData(std::size_t sectionIndex) const
+{
+	if(sectionIndex >= sections.size())
+		return UINT32_MAX;
+	if(optionalHeader.SectionAlignment < PELIB_PAGE_SIZE)
+		return sections[sectionIndex].SizeOfRawData;
+
+	uint32_t beginOfRawData = sections[sectionIndex].PointerToRawData & ~(PELIB_SECTOR_SIZE - 1);
+	uint32_t endOfRawData = sections[sectionIndex].PointerToRawData + AlignToSize(sections[sectionIndex].SizeOfRawData, optionalHeader.FileAlignment);
+	return endOfRawData - beginOfRawData;
+}
+
 uint32_t PeLib::ImageLoader::getImageProtection(uint32_t sectionCharacteristics) const
 {
 	uint32_t Index = 0;

src/pelib/ResourceDirectory.cpp

@@ -677,15 +677,14 @@ namespace PeLib
 			return ERROR_NONE;
 		}
 
-		// Load all entries to the vector
-		std::vector<PELIB_IMAGE_RESOURCE_DIRECTORY_ENTRY> vResourceChildren(uiNumberOfEntries);
 		resDir->insertNodeOffset(uiOffset);
 
 		if (uiNumberOfEntries > 0)
 		{
 			resDir->addOccupiedAddressRange(uiRva, uiRva + uiNumberOfEntries * PELIB_IMAGE_RESOURCE_DIRECTORY_ENTRY::size() - 1);
 		}
 
+		// Load all entries to the vector
 		for (unsigned int i = 0; i < uiNumberOfEntries; i++)
 		{
 			ResourceChild rc;
@@ -728,7 +727,7 @@ namespace PeLib
 				{
 					// Check whether we have enough space to read at least one character
 					unsigned int uiNameOffset = rc.entry.irde.Name & PELIB_IMAGE_RESOURCE_RVA_MASK;
-					if (uiRva + uiNameOffset + sizeof(std::uint16_t) > sizeOfImage)
+					if (uiRsrcRva + uiNameOffset + sizeof(std::uint16_t) > sizeOfImage)
 					{
 						return ERROR_INVALID_FILE;
 					}