[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

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.