[Libcdio-devel] buffer overflow/memory corruption in udf_readdir()

[Pete Batard]

OK, this one has been a bit more of a headache to identify, and I am not 
sure how to properly fix the issue, as I am not familiar with the UDF 
file format.

Basically, I have been experiencing some memory corruption issues when 
using libcdio with UDF ISO images, especially against the UDF images 
that Microsoft makes available through the MSDN, such as the Windows 
installation ISOs, etc.

These issues occurred with all of MinGW32, MinGW64 and Visual Studio 
2010, and I suspect they will manifest themselves on other platforms.

Basically, it looks like the:

   memcpy(&(p_udf_dirent->fe), p_udf_fe,
            sizeof(udf_file_entry_t) + p_udf_fe->i_alloc_descs
            + p_udf_fe->i_extended_attr )

in udf_readdir() can overflow the udf_dirent_t that was originally 
allocated, as we may use larger values for i_alloc_descs and 
i_extended_attr during memcpy than the ones used for alloc.

With some instrumentation added to udf_fs.c and when using the udf1 
sample, this manifests itself as follows:
--------------------------------------------------------------------
--DEBUG: udf_readdir: reading LBA 314
--DEBUG: udf_opendir: using LBA 314 for udf_dirent alloc
--DEBUG: ALLOC: p_udf_dirent 004e09a0 (size 0x118: i_alloc_descs=8, 
i_extended_attr = 56)

(more readout of files/directory entries from the current dirent...)

--DEBUG: udf_readdir: reading LBA 2963
++ WARN: MISMATCH! p_udf_dirent = 004e09a0: i_alloc_desc 40 (new LBA) vs 
8 (existing)
--------------------------------------------------------------------

004e09a0 above is the address of the allocated udf_dirent_t, and the 
instrumentation which motinors alloc/free confirms that the same entry 
is being reused. Yet on allocation, it uses the i_alloc_descs and 
i_extended_attr it got from LBA 314, but while the i_extended_attr is 
the same, the i_alloc_descs from LBA 2963 is 32 bytes larger => buffer 
overflow.

This usually results in an application crash.

If you want to test this behaviour, you can download the "Windows 8 
Developer Preview with developer tools English, 64-bit (x64)", which is 
freely available from Microsoft at [1] and run it against the udf1 sample.

With the attached workaround (needs a proper fix) and instrumentation 
patches, you should get something similar to the attached log, with a 
handful of "MISMATCH" lines...

I hope you can investigate this issue and help devise a proper patch.

Regards,

/Pete

[1] http://msdn.microsoft.com/en-us/windows/apps/br229516

>From 2a48d8c804befce5c133322c250fc35200225765 Mon Sep 17 00:00:00 2001
From: Pete Batard <address@hidden>
Date: Mon, 16 Jan 2012 20:27:48 +0000
Subject: [PATCH 1/2] udf workaround for udf_fs memory corruption

* issue is due to blind memcopy that may overflow udf_dirent
  structure if new LBA for file entry has larger i_alloc_desc
  and i_extended_attr than the ones we used during allocation
---
 lib/udf/udf_fs.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib/udf/udf_fs.c b/lib/udf/udf_fs.c
index e5900c7..a0e7a6d 100644
--- a/lib/udf/udf_fs.c
+++ b/lib/udf/udf_fs.c
@@ -54,6 +54,10 @@
 # include <stdlib.h>
 #endif
 
+#ifndef min
+#define min(a,b) (((a) < (b)) ? (a) : (b))
+#endif
+
 /* These definitions are also to make debugging easy. Note that they
    have to come *before* #include <cdio/ecma_167.h> which sets 
    #defines for these.
@@ -657,14 +661,30 @@ udf_readdir(udf_dirent_t *p_udf_dirent)
        const unsigned int i_len = p_udf_dirent->fid->i_file_id;
        uint8_t data[UDF_BLOCKSIZE] = {0};
        udf_file_entry_t *p_udf_fe = (udf_file_entry_t *) &data;
+       udf_Uint32_t i_alloc_descs = p_udf_dirent->fe.i_alloc_descs;
+       udf_Uint32_t i_extended_attr = p_udf_dirent->fe.i_extended_attr;
 
        if (DRIVER_OP_SUCCESS != udf_read_sectors(p_udf, p_udf_fe, 
p_udf->i_part_start 
                         + p_udf_dirent->fid->icb.loc.lba, 1))
                return NULL;
       
+/* There is a bug here, as we may use a file entry with i_alloc_descs or 
i_extended_attr
+   that doesn't match the one we used when allocating the structure. If they 
are bigger
+   memcpy will result in memory overflow and corruption. Use min() as a 
workaround. */
+if ((p_udf_fe->i_alloc_descs != p_udf_dirent->fe.i_alloc_descs)) {
+       cdio_warn("MISMATCH! p_udf_dirent = %p: i_alloc_desc %d (new LBA) vs %d 
(existing)", p_udf_dirent, p_udf_fe->i_alloc_descs, 
p_udf_dirent->fe.i_alloc_descs);
+       i_alloc_descs = min(p_udf_fe->i_alloc_descs, 
p_udf_dirent->fe.i_alloc_descs);
+}
+if ((p_udf_fe->i_extended_attr != p_udf_dirent->fe.i_extended_attr)) {
+       cdio_warn("MISMATCH! p_udf_dirent = %p: i_extended_attr %d (new LBA) vs 
%d (existing)", p_udf_dirent, p_udf_fe->i_extended_attr, 
p_udf_dirent->fe.i_extended_attr);
+       i_extended_attr = min(p_udf_fe->i_extended_attr, 
p_udf_dirent->fe.i_extended_attr);
+}
+
        memcpy(&(p_udf_dirent->fe), p_udf_fe, 
-              sizeof(udf_file_entry_t) + p_udf_fe->i_alloc_descs 
-              + p_udf_fe->i_extended_attr );
+              sizeof(udf_file_entry_t) + i_alloc_descs 
+              + i_extended_attr );
+       p_udf_dirent->fe.i_alloc_descs = i_alloc_descs;
+       p_udf_dirent->fe.i_extended_attr = i_extended_attr;
 
        if (strlen(p_udf_dirent->psz_name) < i_len) 
          p_udf_dirent->psz_name = (char *)
-- 
1.7.8.msysgit.0

>From de4f0a91f8f5cf4e8fa5807e21a36b5b2e06ac50 Mon Sep 17 00:00:00 2001
From: Pete Batard <address@hidden>
Date: Mon, 16 Jan 2012 23:15:45 +0000
Subject: [PATCH 2/2] udf: add instrumentation for udf_fs memory corruption
 issue

---
 example/udf1.c   |    3 +++
 lib/udf/udf_fs.c |    9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/example/udf1.c b/example/udf1.c
index 52f7077..48fa00e 100644
--- a/example/udf1.c
+++ b/example/udf1.c
@@ -28,6 +28,7 @@
 #include <sys/types.h>
 #include <cdio/cdio.h>
 #include <cdio/udf.h>
+#include <cdio/logging.h>
 
 #include <stdio.h>
 
@@ -96,6 +97,8 @@ main(int argc, const char *argv[])
   udf_t *p_udf;
   char const *psz_udf_image;
 
+       cdio_loglevel_default = CDIO_LOG_DEBUG;
+
   if (argc > 1) 
     psz_udf_image = argv[1];
   else 
diff --git a/lib/udf/udf_fs.c b/lib/udf/udf_fs.c
index a0e7a6d..49d95ab 100644
--- a/lib/udf/udf_fs.c
+++ b/lib/udf/udf_fs.c
@@ -293,6 +293,8 @@ udf_new_dirent(udf_file_entry_t *p_udf_fe, udf_t *p_udf,
   
   udf_dirent_t *p_udf_dirent = (udf_dirent_t *) 
     calloc(1, sizeof(udf_dirent_t) + i_alloc_size);
+cdio_debug("ALLOC: p_udf_dirent %p (size 0x%x: i_alloc_descs=%d, 
i_extended_attr = %d)",
+       p_udf_dirent, sizeof(udf_dirent_t) + i_alloc_size, 
p_udf_fe->i_alloc_descs, p_udf_fe->i_extended_attr);
   if (!p_udf_dirent) return NULL;
   
   p_udf_dirent->psz_name     = strdup(psz_name);
@@ -589,7 +591,10 @@ udf_opendir(const udf_dirent_t *p_udf_dirent)
     uint8_t data[UDF_BLOCKSIZE];
     udf_file_entry_t *p_udf_fe = (udf_file_entry_t *) &data;
     
-    driver_return_code_t i_ret = 
+    driver_return_code_t i_ret;
+    
+cdio_debug("udf_opendir: using LBA %d for udf_dirent alloc", 
p_udf->i_part_start + p_udf_dirent->fid->icb.loc.lba);
+    i_ret =
       udf_read_sectors(p_udf, p_udf_fe, p_udf->i_part_start 
                       + p_udf_dirent->fid->icb.loc.lba, 1);
 
@@ -612,6 +617,7 @@ udf_readdir(udf_dirent_t *p_udf_dirent)
   udf_t *p_udf;
   
   if (p_udf_dirent->dir_left <= 0) {
+cdio_debug("FREEING: p_udf_dirent = %p", p_udf_dirent);
     udf_dirent_free(p_udf_dirent);
     return NULL;
   }
@@ -664,6 +670,7 @@ udf_readdir(udf_dirent_t *p_udf_dirent)
        udf_Uint32_t i_alloc_descs = p_udf_dirent->fe.i_alloc_descs;
        udf_Uint32_t i_extended_attr = p_udf_dirent->fe.i_extended_attr;
 
+cdio_debug("udf_readdir: reading LBA %d", p_udf->i_part_start + 
p_udf_dirent->fid->icb.loc.lba);
        if (DRIVER_OP_SUCCESS != udf_read_sectors(p_udf, p_udf_fe, 
p_udf->i_part_start 
                         + p_udf_dirent->fid->icb.loc.lba, 1))
                return NULL;
-- 
1.7.8.msysgit.0