Global: fixes issues found by AI

src/common/android/dex.c

@@ -55,10 +55,37 @@ static uint32_t dexU32(const uint8_t* p) {
     return (uint32_t) p[0] | ((uint32_t) p[1] << 8) | ((uint32_t) p[2] << 16) | ((uint32_t) p[3] << 24);
 }
 
-// LEB128, at most five bytes for the 32-bit values a dex stores this way.
-static uint32_t dexUleb128(const uint8_t** p) {
+// The dex header points at its tables with offsets that a corrupt or hostile file is free to set
+// anywhere, including past the end of the mapping. Testing `end - p` alone does not catch that: once
+// `p` is past `end` the subtraction is negative and casting it to size_t turns it into a huge value,
+// so the comparison passes and the read goes off the end. Both directions have to be one test.
+static bool dexInRange(const uint8_t* dex, const uint8_t* end, const uint8_t* p, size_t size) {
+    return p >= dex && p <= end && (size_t) (end - p) >= size;
+}
+
+// Same, for a table of `count` elements of `elementSize` bytes. `count` is 64-bit because the
+// callers add one to an index that came out of the file, and doing that in 32 bits can wrap to zero
+// and make the check pass for an index that is not in the table. Dividing rather than multiplying is
+// what keeps a hostile `count` from wrapping the size_t on a 32-bit build -- armeabi-v7a is one, and
+// there `count * 4` overflows for any count above 0x3FFFFFFF and the check passes again.
+static bool dexTableInRange(const uint8_t* dex, const uint8_t* end, const uint8_t* p, uint64_t count, size_t elementSize) {
+    if (p < dex || p > end) {
+        return false;
+    }
+    return (size_t) (end - p) / elementSize >= count;
+}
+
+// LEB128, at most five bytes for the 32-bit values a dex stores this way. `end` bounds every read:
+// the lengths and indexes decoded here come out of the file, so a truncated one must not walk off
+// the end. A read that runs into `end` yields what was decoded so far, and the callers treat the
+// resulting value as untrusted -- it is either compared against another count or used as an index
+// that is range checked in turn.
+static uint32_t dexUleb128(const uint8_t** p, const uint8_t* end) {
     uint32_t value = 0;
     for (int shift = 0; shift <= 28; shift += 7) {
+        if (*p >= end) {
+            break;
+        }
         const uint8_t byte = *(*p)++;
         value |= (uint32_t) (byte & 0x7f) << shift;
         if ((byte & 0x80) == 0) {
@@ -71,12 +98,18 @@ static uint32_t dexUleb128(const uint8_t** p) {
 // encoded_value: one header byte holding ((size - 1) << 5) | type, then that many payload bytes. A
 // `static final int` constant is written as VALUE_INT, whose payload is a sign-extended
 // little-endian integer. Reading through 64 bits keeps every shift in range even if the size field
-// is nonsense; the caller's length check is what rejects such a dex.
-static int32_t dexEncodedInt(const uint8_t** p) {
+// is nonsense; `end` is what keeps a truncated payload from running off the end of the mapping.
+static int32_t dexEncodedInt(const uint8_t** p, const uint8_t* end) {
+    if (*p >= end) {
+        return 0;
+    }
     const uint8_t header = *(*p)++;
     const uint32_t size = (uint32_t) (header >> 5) + 1;
     uint64_t value = 0;
     for (uint32_t i = 0; i < size && i < 8; ++i) {
+        if (*p >= end) {
+            break;
+        }
         value |= (uint64_t) (*(*p)++) << (i * 8);
     }
     const uint32_t bits = size * 8;
@@ -91,14 +124,17 @@ static int32_t dexEncodedInt(const uint8_t** p) {
 // bytes can be used as they are.
 static const char* dexString(const uint8_t* dex, const uint8_t* end, uint32_t index) {
     const uint8_t* ids = dex + dexU32(dex + FF_DEX_OFF_STRING_IDS);
-    if (ids < dex || (size_t) (end - ids) < (size_t) index * 4 + 4) {
+    // `string_ids_size` is not read: the check is that the entry `index` names lies inside the
+    // mapping, which is what the read below needs, and the table is the only thing that can say it
+    // without trusting a second field out of the same file.
+    if (!dexTableInRange(dex, end, ids, (uint64_t) index + 1, 4)) {
         return nullptr;
     }
     const uint8_t* p = dex + dexU32(ids + (size_t) index * 4);
     if (p < dex || p >= end) {
         return nullptr;
     }
-    (void) dexUleb128(&p);
+    (void) dexUleb128(&p, end);
     if (p >= end) {
         return nullptr;
     }
@@ -123,7 +159,7 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla
     // The class's type index, so that the class_def_item and the field ids can be filtered by class.
     const uint8_t* types = dex + dexU32(dex + FF_DEX_OFF_TYPE_IDS);
     const uint32_t typeCount = dexU32(dex + FF_DEX_OFF_TYPE_IDS_SIZE);
-    if (types < dex || (size_t) (end - types) < (size_t) typeCount * 4) {
+    if (!dexTableInRange(dex, end, types, typeCount, 4)) {
         return "The dex type table is out of range";
     }
     uint32_t typeIndex = UINT32_MAX;
@@ -140,7 +176,7 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla
 
     const uint8_t* classes = dex + dexU32(dex + FF_DEX_OFF_CLASS_DEFS);
     const uint32_t classCount = dexU32(dex + FF_DEX_OFF_CLASS_DEFS_SIZE);
-    if (classes < dex || (size_t) (end - classes) < (size_t) classCount * FF_DEX_CLASS_DEF_SIZE) {
+    if (!dexTableInRange(dex, end, classes, classCount, FF_DEX_CLASS_DEF_SIZE)) {
         return "The dex class table is out of range";
     }
 
@@ -152,15 +188,16 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla
 
         const uint8_t* fields = dex + dexU32(classDef + FF_DEX_OFF_CLASS_DEF_DATA);
         const uint8_t* values = dex + dexU32(classDef + FF_DEX_OFF_CLASS_DEF_STATIC_VALUES);
-        if (fields < dex || fields >= end || values < dex || values >= end) {
+        // One byte each, so that the first uleb128 below has something to read.
+        if (!dexInRange(dex, end, fields, 1) || !dexInRange(dex, end, values, 1)) {
             return "The dex class data is out of range";
         }
 
-        const uint32_t staticFields = dexUleb128(&fields);
-        (void) dexUleb128(&fields); // instance_fields_size
-        (void) dexUleb128(&fields); // direct_methods_size
-        (void) dexUleb128(&fields); // virtual_methods_size
-        const uint32_t staticValues = dexUleb128(&values);
+        const uint32_t staticFields = dexUleb128(&fields, end);
+        (void) dexUleb128(&fields, end); // instance_fields_size
+        (void) dexUleb128(&fields, end); // direct_methods_size
+        (void) dexUleb128(&fields, end); // virtual_methods_size
+        const uint32_t staticValues = dexUleb128(&values, end);
         if (staticFields != staticValues) {
             return "The dex static fields and values do not pair up";
         }
@@ -168,14 +205,20 @@ static const char* dexStaticInt(const uint8_t* dex, size_t size, const char* cla
         const uint8_t* fieldIds = dex + dexU32(dex + FF_DEX_OFF_FIELD_IDS);
         uint32_t fieldIndex = 0;
         for (uint32_t j = 0; j < staticFields; ++j) {
-            if (fields >= end) {
+            // The two arrays are walked in step, so running out of either one is a truncation.
+            if (fields >= end || values >= end) {
                 return "The dex static field list is truncated";
             }
-            fieldIndex += dexUleb128(&fields); // field_idx_diff
-            (void) dexUleb128(&fields);        // access_flags
-            const int32_t value = dexEncodedInt(&values);
 
-            if ((size_t) (end - fieldIds) < ((size_t) fieldIndex + 1) * FF_DEX_FIELD_ID_SIZE) {
+            const uint32_t fieldIndexDiff = dexUleb128(&fields, end); // field_idx_diff
+            if (fieldIndexDiff > UINT32_MAX - fieldIndex) {
+                return "The dex field index is out of range";
+            }
+            fieldIndex += fieldIndexDiff;
+            (void) dexUleb128(&fields, end); // access_flags
+            const int32_t value = dexEncodedInt(&values, end);
+
+            if (!dexTableInRange(dex, end, fieldIds, (uint64_t) fieldIndex + 1, FF_DEX_FIELD_ID_SIZE)) {
                 return "The dex field table is out of range";
             }
             const char* name = dexString(dex, end, dexU32(fieldIds + (size_t) fieldIndex * FF_DEX_FIELD_ID_SIZE + FF_DEX_OFF_FIELD_ID_NAME));
@@ -335,8 +378,6 @@ const char* ffDexStaticInt(const char* jarPath, const char* classDescriptor, con
         if (data == nullptr) {
             return "No dex in the jar";
         }
-    } else if (method == FF_ZIP_METHOD_STORED) {
-        dataSize = uncompressedSize;
     } else if (method == FF_ZIP_METHOD_DEFLATED) {
         #ifdef FF_HAVE_ZLIB
         const char* error = inflateDex(data, dataSize, uncompressedSize, &mapping.inflated);
@@ -348,9 +389,14 @@ const char* ffDexStaticInt(const char* jarPath, const char* classDescriptor, con
         #else
         return "The jar deflates classes.dex and fastfetch was built without zlib";
         #endif
-    } else {
+    } else if (method != FF_ZIP_METHOD_STORED) {
         return "classes.dex uses an unsupported compression method";
     }
+    // A STORED entry needs no further work: `dataSize` already holds the `compressedSize` that
+    // `findDexEntry` checked against the mapping, and for a STORED entry the payload is the file
+    // itself. The header's `uncompressedSize` is deliberately not used for it -- nothing bounds that
+    // value, so a corrupt one reaches past the end of the mapping, which is exactly what the dex
+    // header would then be validated against.
 
     mapping.data = data;
     mapping.size = dataSize;