diff options
author | Samuel Huang <huangs@chromium.org> | 2018-07-11 20:02:23 +0000 |
---|---|---|
committer | Copybara-Service <copybara-worker@google.com> | 2021-07-25 20:31:10 -0700 |
commit | 06a9be1c70878cf651b70192275187bf9a8662a9 (patch) | |
tree | 3524b5d787ae1dc22f1f8c4860be32ef523c7097 /disassembler_dex.cc | |
parent | a09f4e23431ab0718a168d4b37afd9a2923fb1cb (diff) | |
download | zucchini-06a9be1c70878cf651b70192275187bf9a8662a9.tar.gz |
[Zucchini] DEX parsing: Implement stricter size checks for MapItem.
This CL fixes a bug discovered by the Fuzzer, where DEX parsing
triggers CHECK() failure due to an attempt to read a 4-byte value that
straddles EOF.
Tracing (Zucchini-read) using the Fuzzer-provided DEX show that the
faulty read attempt is for ClassDefItem::static_values_off, which is
a field in a fixed-length item.
Our fix is to validate MapItem entries for fixed-length items. Details:
- Add GetItemBaseSize() to return an item size bound. For fixed-length
items, this is exactly the item size. Moreover, no 4-byte alignment
is needed, since the item sizes are already multiples of 4 bytes.
- In DisassemblerDex::ParseHeader(), verify each MapItem fits in the
image. For fixed-length items, this check enables safe read for
items whose index are within bound (and avoid similar bugs). For
variable-length items, this serves as a sanity check to quickly
reject obviously bad inputs. Subsequent parsing will perform a more
refined check. For unhandled items, sizes are assumed to be 1.
- Not checked: Whether MapItems ranges overlap. May do this in the
future (more refactoring will be needed).
Bug: 862566
Change-Id: If8efce122979fa1a36d1d445556d414eb499d273
Reviewed-on: https://chromium-review.googlesource.com/1133713
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574294}
NOKEYCHECK=True
GitOrigin-RevId: 1f63bf220bcb93560358fbe593ebccc4d64f4baa
Diffstat (limited to 'disassembler_dex.cc')
-rw-r--r-- | disassembler_dex.cc | 46 |
1 files changed, 43 insertions, 3 deletions
diff --git a/disassembler_dex.cc b/disassembler_dex.cc index 9d2ee3a..6ec67d1 100644 --- a/disassembler_dex.cc +++ b/disassembler_dex.cc @@ -49,6 +49,41 @@ bool Is32BitAligned(offset_t offset) { return offset % 4 == 0; } +// Returns a lower bound for the size of an item of type |type_item_code|. +// - For fixed-length items (e.g., kTypeFieldIdItem) this is the exact size. +// - For variant-length items (e.g., kTypeCodeItem), returns a value that is +// known to be less than the item length (e.g., header size). +// - For items not handled by this function, returns 1 for sanity check. +size_t GetItemBaseSize(uint16_t type_item_code) { + switch (type_item_code) { + case dex::kTypeStringIdItem: + return sizeof(dex::StringIdItem); + case dex::kTypeTypeIdItem: + return sizeof(dex::TypeIdItem); + case dex::kTypeProtoIdItem: + return sizeof(dex::ProtoIdItem); + case dex::kTypeFieldIdItem: + return sizeof(dex::FieldIdItem); + case dex::kTypeMethodIdItem: + return sizeof(dex::MethodIdItem); + case dex::kTypeClassDefItem: + return sizeof(dex::ClassDefItem); + // No need to handle dex::kTypeMapList. + case dex::kTypeTypeList: + return sizeof(uint32_t); // Variable-length. + case dex::kTypeAnnotationSetRefList: + return sizeof(uint32_t); // Variable-length. + case dex::kTypeAnnotationSetItem: + return sizeof(uint32_t); // Variable-length. + case dex::kTypeCodeItem: + return sizeof(dex::CodeItem); // Variable-length. + case dex::kTypeAnnotationsDirectoryItem: + return sizeof(dex::AnnotationsDirectoryItem); // Variable-length. + default: + return 1U; // Unhandled item. For sanity check assume size >= 1. + } +} + /******** CodeItemParser ********/ // A parser to extract successive code items from a DEX image whose header has @@ -1549,6 +1584,10 @@ bool DisassemblerDex::ParseHeader() { return false; // Read and validate map list, ensuring that required item types are present. + // - GetItemBaseSize() should have an entry for each item. + // - dex::kTypeCodeItem is actually not required; it's possible to have a DEX + // file with classes that have no code. However, this is unlikely to appear + // in application, so for simplicity we require DEX files to have code. std::set<uint16_t> required_item_types = { dex::kTypeStringIdItem, dex::kTypeTypeIdItem, dex::kTypeProtoIdItem, dex::kTypeFieldIdItem, dex::kTypeMethodIdItem, dex::kTypeClassDefItem, @@ -1556,9 +1595,10 @@ bool DisassemblerDex::ParseHeader() { }; for (offset_t i = 0; i < list_size; ++i) { const dex::MapItem* item = &item_list[i]; - // Sanity check to reject unreasonably large |item->size|. - // TODO(huangs): Implement a more stringent check. - if (!image_.covers({item->offset, item->size})) + // Reject unreasonably large |item->size|. + size_t item_size = GetItemBaseSize(item->type); + // Confusing name: |item->size| is actually the number of items. + if (!image_.covers_array(item->offset, item->size, item_size)) return false; if (!map_item_map_.insert(std::make_pair(item->type, item)).second) return false; // A given type must appear at most once. |