diff options
author | Samuel Huang <huangs@chromium.org> | 2018-12-31 19:59:22 +0000 |
---|---|---|
committer | Copybara-Service <copybara-worker@google.com> | 2021-07-25 20:42:02 -0700 |
commit | 431d119dafdcf886dbf161ba66cfe17100159621 (patch) | |
tree | dde64b243be6559c2d5df153d3cc088982ac8bea | |
parent | fb345573ac09d9e282569d01fceadda5bb570bc4 (diff) | |
download | zucchini-431d119dafdcf886dbf161ba66cfe17100159621.tar.gz |
[Zucchini] DisassemblerWin32: Fix rogue |reloc_region_| usage.
ClusterFuzz found a 32-bit PE file that triggers DCHECK(). The cause (in
DisassemblerWin32):
* ParseAndStoreRelocBlocks() reads malformed reloc region and rejects
it, but writes bad result to |reloc_region_| (with kInvalidOffset).
* MakeReadRelocs() passes the invalid |reloc_region_|, which causes
uint32_t overflow (for 32-bit size_t), and triggering DCHECK().
* MakeWriteRelocs() was not reached, but has similar problem.
Main fix:
* Initialize |reloc_region_| to invalid state.
* In ParseAndStoreRelocBlocks():
* On first run: Only write |reloc_region_| on success.
* On later runs: Return validity of |reloc_region_|.
* In MakeReadRelocs() and MakeWriteRelocs(): Already calls
ParseAndStoreRelocBlocks(). Change this to return vacuous
ReferenceReader / ReferenceWriter on failure, i.e., if
|reloc_region_| is invalid.
* Need to add EmptyReferenceWriter for this.
DisassemblerWin32 is also too lax in dealing with malformed input, so
this CL also implments more stringent checks:
* Disallow ImageSectionHeader (+ ImageOptionalHeader) from overlapping
with ImageDOSHeader.
* Apply more stringent checks for |size_of_optional_header| and
|number_of_rva_and_sizes|: Consistency and bounds.
Bug: 917042
Change-Id: I661303d49a57025a877585eb175f369d067652a4
Reviewed-on: https://chromium-review.googlesource.com/c/1390766
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619316}
NOKEYCHECK=True
GitOrigin-RevId: d46be686cb9e764befdd39aa168dde62b5314e37
-rw-r--r-- | disassembler.cc | 4 | ||||
-rw-r--r-- | disassembler.h | 6 | ||||
-rw-r--r-- | disassembler_win32.cc | 60 | ||||
-rw-r--r-- | disassembler_win32.h | 2 | ||||
-rw-r--r-- | type_win_pe.h | 3 |
5 files changed, 51 insertions, 24 deletions
diff --git a/disassembler.cc b/disassembler.cc index b6a2748..897d949 100644 --- a/disassembler.cc +++ b/disassembler.cc @@ -14,6 +14,10 @@ base::Optional<Reference> EmptyReferenceReader::GetNext() { return base::nullopt; } +/******** EmptyReferenceWriter ********/ + +void EmptyReferenceWriter::PutNext(Reference /* reference */) {} + /******** ReferenceGroup ********/ std::unique_ptr<ReferenceReader> ReferenceGroup::GetReader( diff --git a/disassembler.h b/disassembler.h index d60a3f1..52c69a4 100644 --- a/disassembler.h +++ b/disassembler.h @@ -23,6 +23,12 @@ class EmptyReferenceReader : public ReferenceReader { base::Optional<Reference> GetNext() override; }; +// A vacuous EmptyReferenceWriter that does not write. +class EmptyReferenceWriter : public ReferenceWriter { + public: + void PutNext(Reference reference) override; +}; + // Disassembler needs to be declared before ReferenceGroup because the latter // contains member pointers based on the former, and we use a compiler flag, // -fcomplete-member-pointers, which enforces that member pointer base types are diff --git a/disassembler_win32.cc b/disassembler_win32.cc index 9067ad7..01c4fde 100644 --- a/disassembler_win32.cc +++ b/disassembler_win32.cc @@ -33,9 +33,11 @@ bool ReadWin32Header(ConstBufferView image, BufferSource* source) { return false; const auto* dos_header = source->GetPointer<pe::ImageDOSHeader>(); - if (!dos_header || (dos_header->e_lfanew & 7) != 0) + // For |e_lfanew|, reject on misalignment or overlap with DOS header. + if (!dos_header || (dos_header->e_lfanew & 7) != 0 || + dos_header->e_lfanew < 0U + sizeof(pe::ImageDOSHeader)) { return false; - + } // Offset to PE header is in DOS header. *source = std::move(BufferSource(image).Skip(dos_header->e_lfanew)); // Check 'PE\0\0' magic from PE header. @@ -120,7 +122,8 @@ template <class Traits> std::unique_ptr<ReferenceReader> DisassemblerWin32<Traits>::MakeReadRelocs( offset_t lo, offset_t hi) { - ParseAndStoreRelocBlocks(); + if (!ParseAndStoreRelocBlocks()) + return std::make_unique<EmptyReferenceReader>(); RelocRvaReaderWin32 reloc_rva_reader(image_, reloc_region_, reloc_block_offsets_, lo, hi); @@ -155,7 +158,9 @@ std::unique_ptr<ReferenceReader> DisassemblerWin32<Traits>::MakeReadRel32( template <class Traits> std::unique_ptr<ReferenceWriter> DisassemblerWin32<Traits>::MakeWriteRelocs( MutableBufferView image) { - ParseAndStoreRelocBlocks(); + if (!ParseAndStoreRelocBlocks()) + return std::make_unique<EmptyReferenceWriter>(); + return std::make_unique<RelocWriterWin32>(Traits::kRelocType, image, reloc_region_, reloc_block_offsets_, translator_); @@ -187,28 +192,30 @@ bool DisassemblerWin32<Traits>::ParseHeader() { if (!ReadWin32Header<Traits>(image_, &source)) return false; + constexpr size_t kDataDirBase = + offsetof(typename Traits::ImageOptionalHeader, data_directory); auto* coff_header = source.GetPointer<pe::ImageFileHeader>(); - if (!coff_header || - coff_header->size_of_optional_header < - offsetof(typename Traits::ImageOptionalHeader, data_directory)) { + if (!coff_header || coff_header->size_of_optional_header < kDataDirBase) return false; - } + // |number_of_rva_and_sizes < kImageNumberOfDirectoryEntries| is possible. So + // in theory, GetPointer() on ImageOptionalHeader can reach EOF for a tiny PE + // file, causing false rejection. However, this should not occur for practical + // cases; and rejection is okay for corner cases (e.g., from a fuzzer). auto* optional_header = source.GetPointer<typename Traits::ImageOptionalHeader>(); if (!optional_header || optional_header->magic != Traits::kMagic) return false; - const size_t kDataDirBase = - offsetof(typename Traits::ImageOptionalHeader, data_directory); - size_t size_of_optional_header = coff_header->size_of_optional_header; - if (size_of_optional_header < kDataDirBase) - return false; - - const size_t data_dir_bound = - (size_of_optional_header - kDataDirBase) / sizeof(pe::ImageDataDirectory); - if (optional_header->number_of_rva_and_sizes > data_dir_bound) + // Check |optional_header->number_of_rva_and_sizes|. + const size_t data_dir_size = + coff_header->size_of_optional_header - kDataDirBase; + const size_t num_data_dir = data_dir_size / sizeof(pe::ImageDataDirectory); + if (num_data_dir != optional_header->number_of_rva_and_sizes || + num_data_dir * sizeof(pe::ImageDataDirectory) != data_dir_size || + num_data_dir > pe::kImageNumberOfDirectoryEntries) { return false; + } base_relocation_table_ = ReadDataDirectory<Traits>( optional_header, pe::kIndexOfBaseRelocationTable); @@ -293,21 +300,28 @@ bool DisassemblerWin32<Traits>::ParseHeader() { template <class Traits> bool DisassemblerWin32<Traits>::ParseAndStoreRelocBlocks() { if (has_parsed_relocs_) - return true; + return reloc_region_.lo() != kInvalidOffset; + has_parsed_relocs_ = true; DCHECK(reloc_block_offsets_.empty()); offset_t relocs_offset = translator_.RvaToOffset(base_relocation_table_->virtual_address); size_t relocs_size = base_relocation_table_->size; - reloc_region_ = {relocs_offset, relocs_size}; - // Reject bogus relocs. Note that empty relocs are allowed! - if (!image_.covers(reloc_region_)) + const BufferRegion temp_reloc_region = {relocs_offset, relocs_size}; + + // Reject bogus relocs. It's possible to have no reloc, so this is non-fatal! + if (relocs_offset == kInvalidOffset || !image_.covers(temp_reloc_region)) return false; // Precompute offsets of all reloc blocks. - return RelocRvaReaderWin32::FindRelocBlocks(image_, reloc_region_, - &reloc_block_offsets_); + if (!RelocRvaReaderWin32::FindRelocBlocks(image_, temp_reloc_region, + &reloc_block_offsets_)) { + return false; + } + // Reassign |reloc_region_| only on success. + reloc_region_ = temp_reloc_region; + return true; } template <class Traits> diff --git a/disassembler_win32.h b/disassembler_win32.h index 29033ce..6c7ba91 100644 --- a/disassembler_win32.h +++ b/disassembler_win32.h @@ -104,7 +104,7 @@ class DisassemblerWin32 : public Disassembler { AddressTranslator translator_; // Reference storage. - BufferRegion reloc_region_; + BufferRegion reloc_region_ = {kInvalidOffset, 0U}; std::vector<offset_t> reloc_block_offsets_; offset_t reloc_end_ = 0; std::vector<offset_t> abs32_locations_; diff --git a/type_win_pe.h b/type_win_pe.h index d385ca7..56996fe 100644 --- a/type_win_pe.h +++ b/type_win_pe.h @@ -120,6 +120,9 @@ struct ImageOptionalHeader { uint32_t size_of_heap_commit; uint32_t loader_flags; uint32_t number_of_rva_and_sizes; + + // The number of elements is actually |number_of_rva_and_sizes|, so accesses + // to |data_directory| should be checked against the bound. ImageDataDirectory data_directory[kImageNumberOfDirectoryEntries]; // 0x60 /* 0xE0 */ }; |