Re: [Libcdio-devel] proposed fix for bug #22865

[Pete Batard]

OK, here is my actual proposed fix for #22865.

Haven't applied it to my branch as I want to see if it looks sound to 
you, or if you prefer something different.

Also note that I'm adding an assert for the size of udf_file_entry_t in 
udf_open.

Once this patch is applied, and if you test against the Windows 8 
preview, you will still end up with "Error reading UDF file 
/sources/install.wim" on Linux 32 bit (haven't test x64), but this time 
it should be due to a 32 bit offset issue. I'll discuss that in an 
upcoming mail.

Regards,

/Pete

>From 0d5e54af5ce136929ef714f5988ba689e8d42656 Mon Sep 17 00:00:00 2001
From: Pete Batard <address@hidden>
Date: Mon, 23 Jan 2012 12:34:11 +0000
Subject: [PATCH] Fix buffer overflow when reusing an UDF file entry

* use an union to ensure a file entry is always the maximum
  allocatable size as per specs
* this is bug #22865
---
 include/cdio/ecma_167.h |   10 ++++++++--
 lib/udf/udf_file.c      |    5 ++---
 lib/udf/udf_fs.c        |   36 +++++++++++++++---------------------
 3 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/include/cdio/ecma_167.h b/include/cdio/ecma_167.h
index 78da7ae..9c7c1bb 100644
--- a/include/cdio/ecma_167.h
+++ b/include/cdio/ecma_167.h
@@ -727,8 +727,14 @@ struct udf_file_entry_s
   udf_Uint64_t   unique_ID;
   udf_Uint32_t   i_extended_attr;
   udf_Uint32_t   i_alloc_descs;
-  udf_Uint8_t    ext_attr[0];
-  udf_Uint8_t    alloc_descs[0];
+  /* The following union allows file entry reuse without worrying
+     about overflows, by ensuring the struct is always the
+     maximum possible size allowed by the specs: one UDF block. */
+  union {
+    udf_Uint8_t          ext_attr[0];
+    udf_Uint8_t          alloc_descs[0];
+    udf_Uint8_t          pad_to_one_block[2048-176];
+  } u;
 } GNUC_PACKED;
 
 typedef struct udf_file_entry_s udf_file_entry_t;
diff --git a/lib/udf/udf_file.c b/lib/udf/udf_file.c
index ee766a8..ca4d920 100644
--- a/lib/udf/udf_file.c
+++ b/lib/udf/udf_file.c
@@ -34,7 +34,7 @@
 #define CEILING(x, y) ((x+(y-1))/y)
 
 #define        GETICB(offset)  \
-       &p_udf_fe->alloc_descs[offset]
+       &p_udf_fe->u.alloc_descs[offset]
 
 const char *
 udf_get_filename(const udf_dirent_t *p_udf_dirent)
@@ -44,8 +44,7 @@ udf_get_filename(const udf_dirent_t *p_udf_dirent)
   return p_udf_dirent->psz_name;
 }
 
-/* Get UDF File Entry. However we do NOT get the variable-length extended
- attributes. */
+/* Copy an UDF File Entry into a Directory Entry structure. */
 bool
 udf_get_file_entry(const udf_dirent_t *p_udf_dirent, 
                   /*out*/ udf_file_entry_t *p_udf_fe)
diff --git a/lib/udf/udf_fs.c b/lib/udf/udf_fs.c
index dcd6e53..eb1da77 100644
--- a/lib/udf/udf_fs.c
+++ b/lib/udf/udf_fs.c
@@ -68,6 +68,7 @@ const char VSD_STD_ID_TEA01[] = {'T', 'E', 'A', '0', '1'};
 #include <cdio/bytesex.h>
 #include "udf_private.h"
 #include "udf_fs.h"
+#include "cdio_assert.h"
 
 /*
  * The UDF specs are pretty clear on how each data structure is made
@@ -154,7 +155,7 @@ udf_get_lba(const udf_file_entry_t *p_udf_fe,
     {
       /* The allocation descriptor field is filled with short_ad's. */
       udf_short_ad_t *p_ad = (udf_short_ad_t *) 
-       (p_udf_fe->ext_attr + p_udf_fe->i_extended_attr);
+       (p_udf_fe->u.ext_attr + p_udf_fe->i_extended_attr);
       
       *start = uint32_from_le(p_ad->pos);
       *end = *start + 
@@ -166,7 +167,7 @@ udf_get_lba(const udf_file_entry_t *p_udf_fe,
     {
       /* The allocation descriptor field is filled with long_ad's */
       udf_long_ad_t *p_ad = (udf_long_ad_t *) 
-       (p_udf_fe->ext_attr + p_udf_fe->i_extended_attr);
+       (p_udf_fe->u.ext_attr + p_udf_fe->i_extended_attr);
       
       *start = uint32_from_le(p_ad->loc.lba); /* ignore partition number */
       *end = *start + 
@@ -177,7 +178,7 @@ udf_get_lba(const udf_file_entry_t *p_udf_fe,
   case ICBTAG_FLAG_AD_EXTENDED:
     {
       udf_ext_ad_t *p_ad = (udf_ext_ad_t *)
-       (p_udf_fe->ext_attr + p_udf_fe->i_extended_attr);
+       (p_udf_fe->u.ext_attr + p_udf_fe->i_extended_attr);
       
       *start = uint32_from_le(p_ad->ext_loc.lba); /* ignore partition number */
       *end = *start + 
@@ -287,11 +288,8 @@ static udf_dirent_t *
 udf_new_dirent(udf_file_entry_t *p_udf_fe, udf_t *p_udf,
               const char *psz_name, bool b_dir, bool b_parent) 
 {
-  const unsigned int i_alloc_size = p_udf_fe->i_alloc_descs
-    + p_udf_fe->i_extended_attr;
-  
   udf_dirent_t *p_udf_dirent = (udf_dirent_t *) 
-    calloc(1, sizeof(udf_dirent_t) + i_alloc_size);
+    calloc(1, sizeof(udf_dirent_t));
   if (!p_udf_dirent) return NULL;
   
   p_udf_dirent->psz_name     = strdup(psz_name);
@@ -302,7 +300,7 @@ udf_new_dirent(udf_file_entry_t *p_udf_fe, udf_t *p_udf,
   p_udf_dirent->dir_left     = uint64_from_le(p_udf_fe->info_len); 
 
   memcpy(&(p_udf_dirent->fe), p_udf_fe, 
-        sizeof(udf_file_entry_t) + i_alloc_size);
+        sizeof(udf_file_entry_t));
   udf_get_lba( p_udf_fe, &(p_udf_dirent->i_loc), 
               &(p_udf_dirent->i_loc_end) );
   return p_udf_dirent;
@@ -349,6 +347,9 @@ udf_open (const char *psz_path)
 
   if (!p_udf) return NULL;
 
+  /* Sanity check */
+  cdio_assert(sizeof(udf_file_entry_t) == UDF_BLOCKSIZE);
+
   p_udf->cdio = cdio_open(psz_path, DRIVER_UNKNOWN);
   if (!p_udf->cdio) {
     /* Not a CD-ROM drive or CD Image. Maybe it's a UDF file not
@@ -585,19 +586,18 @@ udf_opendir(const udf_dirent_t *p_udf_dirent)
 {
   if (p_udf_dirent->b_dir && !p_udf_dirent->b_parent && p_udf_dirent->fid) {
     udf_t *p_udf = p_udf_dirent->p_udf;
-    uint8_t data[UDF_BLOCKSIZE];
-    udf_file_entry_t *p_udf_fe = (udf_file_entry_t *) &data;
+    udf_file_entry_t udf_fe;
     
     driver_return_code_t i_ret = 
-      udf_read_sectors(p_udf, p_udf_fe, p_udf->i_part_start 
+      udf_read_sectors(p_udf, &udf_fe, p_udf->i_part_start 
                       + p_udf_dirent->fid->icb.loc.lba, 1);
 
     if (DRIVER_OP_SUCCESS == i_ret 
-       && !udf_checktag(&p_udf_fe->tag, TAGID_FILE_ENTRY)) {
+       && !udf_checktag(&udf_fe.tag, TAGID_FILE_ENTRY)) {
       
-      if (ICBTAG_FILE_TYPE_DIRECTORY == p_udf_fe->icb_tag.file_type) {
+      if (ICBTAG_FILE_TYPE_DIRECTORY == udf_fe.icb_tag.file_type) {
        udf_dirent_t *p_udf_dirent_new = 
-         udf_new_dirent(p_udf_fe, p_udf, p_udf_dirent->psz_name, true, true);
+         udf_new_dirent(&udf_fe, p_udf, p_udf_dirent->psz_name, true, true);
        return p_udf_dirent_new;
       }
     }
@@ -661,16 +661,10 @@ 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;
 
-       if (DRIVER_OP_SUCCESS != udf_read_sectors(p_udf, p_udf_fe, 
p_udf->i_part_start 
+       if (DRIVER_OP_SUCCESS != udf_read_sectors(p_udf, &p_udf_dirent->fe, 
p_udf->i_part_start 
                         + p_udf_dirent->fid->icb.loc.lba, 1))
                return NULL;
-      
-       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 );
 
        if (strlen(p_udf_dirent->psz_name) < i_len) 
          p_udf_dirent->psz_name = (char *)
-- 
1.7.8.msysgit.0