CVE-2022-36938: Improper Validation of Specified Quantity in Input (CWE-1284)
value: DexLoader function get_stringidx_fromdex() in Redex prior to commit 3b44c64 can load an out of bound address when loading the string index table, potentially allowing remote code execution during processing of a 3rd party Android APK file.
CWE description: The product receives input that is expected to specify a quantity (such as size or length), but it does not validate or incorrectly validates that the quantity has the required properties.
CVSS v3 base score: 9.8
Published: 2022-11-11T00:15Z
Repository: https://github.com/facebook/redex
Language: C++
Files changed: libredex/DexLoader.cpp
Commit date: 2022-11-04 16:52:15 -0700
Commit message: Add more ids table size bound checks

Summary: When loading Dex files, for the size of each section tables, we did not do enough checks on whether the encoded size of the table is actually out of bound. Having the additional checks makes Dex file loading a bit safer. :)

Reviewed By: agampe

Differential Revision: D41043723

fbshipit-source-id: 874de6a1081e27130ea7b175cfe5b455a49466a7

Diff: diff --git a/libredex/DexLoader.cpp b/libredex/DexLoader.cpp
index 8511b988..a29776a2 100644
--- a/libredex/DexLoader.cpp
+++ b/libredex/DexLoader.cpp
@@ -29,10 +29,14 @@ DexLoader::DexLoader(const DexLocation* location)
       m_location(location) {}
 
 static void validate_dex_header(const dex_header* dh,
                                 size_t dexsize,
                                 int support_dex_version) {
+  always_assert_log(sizeof(dex_header) <= dexsize,
+                    "Header size (%lu) is larger than file size (%zu)\n",
+                    dexsize,
+                    sizeof(dex_header));
   bool supported = false;
   switch (support_dex_version) {
   case 38:
     supported = supported ||
                 !memcmp(dh->magic, DEX_HEADER_DEXMAGIC_V38, sizeof(dh->magic));
@@ -54,14 +58,45 @@ static void validate_dex_header(const dex_header* dh,
   always_assert_log(
       dh->file_size == dexsize,
       "Reported size in header (%zu) does not match file size (%u)\n",
       dexsize,
       dh->file_size);
-  auto off = (uint64_t)dh->class_defs_off;
-  auto limit = off + dh->class_defs_size * sizeof(dex_class_def);
-  always_assert_log(off < dexsize, "class_defs_off out of range");
-  always_assert_log(limit <= dexsize, "invalid class_defs_size");
+
+  auto str_ids_off = (uint64_t)dh->string_ids_off;
+  auto str_ids_limit =
+      str_ids_off + dh->string_ids_size * sizeof(dex_string_id);
+  always_assert_log(str_ids_off < dexsize, "string_ids_off out of range");
+  always_assert_log(str_ids_limit <= dexsize, "invalid string_ids_size");
+
+  auto type_ids_off = (uint64_t)dh->type_ids_off;
+  auto type_ids_limit = type_ids_off + dh->type_ids_size * sizeof(dex_type_id);
+  always_assert_log(type_ids_off < dexsize, "type_ids_off out of range");
+  always_assert_log(type_ids_limit <= dexsize, "invalid type_ids_size");
+
+  auto proto_ids_off = (uint64_t)dh->proto_ids_off;
+  auto proto_ids_limit =
+      proto_ids_off + dh->proto_ids_size * sizeof(dex_proto_id);
+  always_assert_log(proto_ids_off < dexsize, "proto_ids_off out of range");
+  always_assert_log(proto_ids_limit <= dexsize, "invalid proto_ids_size");
+
+  auto field_ids_off = (uint64_t)dh->field_ids_off;
+  auto field_ids_limit =
+      field_ids_off + dh->field_ids_size * sizeof(dex_field_id);
+  always_assert_log(field_ids_off < dexsize, "field_ids_off out of range");
+  always_assert_log(field_ids_limit <= dexsize, "invalid field_ids_size");
+
+  auto meth_ids_off = (uint64_t)dh->method_ids_off;
+  auto meth_ids_limit =
+      meth_ids_off + dh->method_ids_size * sizeof(dex_method_id);
+  always_assert_log(meth_ids_off < dexsize, "method_ids_off out of range");
+  always_assert_log(meth_ids_limit <= dexsize, "invalid method_ids_size");
+
+  auto cls_defs_off = (uint64_t)dh->class_defs_off;
+  auto cls_defs_limit =
+      cls_defs_off + dh->class_defs_size * sizeof(dex_class_def);
+  always_assert_log(cls_defs_off < dexsize, "class_defs_off out of range");
+  always_assert_log(cls_defs_limit <= dexsize, "invalid class_defs_size");
 }
 
 void DexLoader::gather_input_stats(dex_stats_t* stats, const dex_header* dh) {
   if (!stats) {
     return;

Vulnerable code:   auto off = (uint64_t)dh->class_defs_off;
  auto limit = off + dh->class_defs_size * sizeof(dex_class_def);
  always_assert_log(off < dexsize, "class_defs_off out of range");
  always_assert_log(limit <= dexsize, "invalid class_defs_size");
Fixed code:   always_assert_log(sizeof(dex_header) <= dexsize,
                    "Header size (%lu) is larger than file size (%zu)\n",
                    dexsize,
                    sizeof(dex_header));

  auto str_ids_off = (uint64_t)dh->string_ids_off;
  auto str_ids_limit =
      str_ids_off + dh->string_ids_size * sizeof(dex_string_id);
  always_assert_log(str_ids_off < dexsize, "string_ids_off out of range");
  always_assert_log(str_ids_limit <= dexsize, "invalid string_ids_size");

  auto type_ids_off = (uint64_t)dh->type_ids_off;
  auto type_ids_limit = type_ids_off + dh->type_ids_size * sizeof(dex_type_id);
  always_assert_log(type_ids_off < dexsize, "type_ids_off out of range");
  always_assert_log(type_ids_limit <= dexsize, "invalid type_ids_size");

  auto proto_ids_off = (uint64_t)dh->proto_ids_off;
  auto proto_ids_limit =
      proto_ids_off + dh->proto_ids_size * sizeof(dex_proto_id);
  always_assert_log(proto_ids_off < dexsize, "proto_ids_off out of range");
  always_assert_log(proto_ids_limit <= dexsize, "invalid proto_ids_size");

  auto field_ids_off = (uint64_t)dh->field_ids_off;
  auto field_ids_limit =
      field_ids_off + dh->field_ids_size * sizeof(dex_field_id);
  always_assert_log(field_ids_off < dexsize, "field_ids_off out of range");
  always_assert_log(field_ids_limit <= dexsize, "invalid field_ids_size");

  auto meth_ids_off = (uint64_t)dh->method_ids_off;
  auto meth_ids_limit =
      meth_ids_off + dh->method_ids_size * sizeof(dex_method_id);
  always_assert_log(meth_ids_off < dexsize, "method_ids_off out of range");
  always_assert_log(meth_ids_limit <= dexsize, "invalid method_ids_size");

  auto cls_defs_off = (uint64_t)dh->class_defs_off;
  auto cls_defs_limit =
      cls_defs_off + dh->class_defs_size * sizeof(dex_class_def);
  always_assert_log(cls_defs_off < dexsize, "class_defs_off out of range");
  always_assert_log(cls_defs_limit <= dexsize, "invalid class_defs_size");