Fix issues when mobi_buffer_getpointer returns null. With corrupt dat…

…a this could lead to out-of-bounds read
bfabiszewski · Apr 23, 2022 · cded29d · cded29d
1 parent 395dbde
commit cded29d
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 7 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,4 @@
+2022-04-23: Fix issues when mobi_buffer_getpointer returns null. With corrupt data this could lead to out-of-bounds read
 2022-04-13: Add packaging status [skip ci]
 2022-04-10: Make random generation return proper error codes
 2022-04-10: Rewrite randombytes for libmobi

diff --git a/src/encryption.c b/src/encryption.c
@@ -302,11 +302,14 @@ static size_t mobi_vouchers_get(MOBIVoucher **drm, const MOBIData *m) {
     debug_print("%s\n", "Parsing DRM data from record 0");
     for (size_t i = 0; i < count; i++) {
         drm[i] = calloc(1, sizeof(MOBIVoucher));
-		if (drm[i] == NULL) {
-			debug_print("%s\n", "Memory allocation failed");
-			mobi_buffer_free_null(buf);
-			return 0;
-		}
+        if (drm[i] == NULL) {
+            debug_print("%s\n", "Memory allocation failed");
+            mobi_buffer_free_null(buf);
+            for (size_t j = 0; j < i; j++) {
+                free(drm[j]);
+            }
+            return 0;
+        }
         drm[i]->verification = mobi_buffer_get32(buf);
         drm[i]->size = mobi_buffer_get32(buf);
         drm[i]->type = mobi_buffer_get32(buf);
@@ -315,6 +318,14 @@ static size_t mobi_vouchers_get(MOBIVoucher **drm, const MOBIData *m) {
         drm[i]->cookie = mobi_buffer_getpointer(buf, COOKIESIZE);
         debug_print("drm[%zu] verification = %#x, size = %#x, type = %#x, checksum = %#x\n",
                     i, drm[i]->verification, drm[i]->size, drm[i]->type, drm[i]->checksum);
+        if (buf->error != MOBI_SUCCESS) {
+            debug_print("%s\n", "DRM data too short");
+            mobi_buffer_free_null(buf);
+            for (size_t j = 0; j < i + 1; j++) {
+                free(drm[j]);
+            }
+            return 0;
+        }
     }
     mobi_buffer_free_null(buf);
     return count;

diff --git a/src/parse_rawml.c b/src/parse_rawml.c
@@ -851,7 +851,13 @@ MOBI_RET mobi_reconstruct_parts(MOBIRawml *rawml) {
         debug_print("%zu\t%s\t%i\t%i\t%i\n", i, entry->label, fragments_count, skel_position, skel_length);
         mobi_buffer_setpos(buf, skel_position);
 
-        MOBIFragment *first_fragment = mobi_list_add(NULL, 0, mobi_buffer_getpointer(buf, skel_length), skel_length, false);
+        unsigned char *frag_buffer = mobi_buffer_getpointer(buf, skel_length);
+        if (frag_buffer == NULL) {
+            debug_print("%s\n", "Fragment data beyond buffer");
+            mobi_buffer_free_null(buf);
+            return MOBI_DATA_CORRUPT;
+        }
+        MOBIFragment *first_fragment = mobi_list_add(NULL, 0, frag_buffer, skel_length, false);
         MOBIFragment *current_fragment = first_fragment;
         while (fragments_count--) {
             entry = &rawml->frag->entries[j];
@@ -926,7 +932,14 @@ MOBI_RET mobi_reconstruct_parts(MOBIRawml *rawml) {
             }
             skel_length += frag_length;
 
-            current_fragment = mobi_list_insert(current_fragment, insert_position, mobi_buffer_getpointer(buf, frag_length), frag_length, false, insert_position);
+            frag_buffer = mobi_buffer_getpointer(buf, frag_length);
+            if (frag_buffer == NULL) {
+                debug_print("%s\n", "Fragment data beyond buffer");
+                mobi_buffer_free_null(buf);
+                mobi_list_del_all(first_fragment);
+                return MOBI_DATA_CORRUPT;
+            }
+            current_fragment = mobi_list_insert(current_fragment, insert_position, frag_buffer, frag_length, false, insert_position);
             j++;
 
         }