On Mon, Jan 23, 2012 at 12:43 PM, Pete Batard <[email protected]> wrote:
> OK, here is my actual proposed fix for #22865. > Overall looks okay. The only thing I'd like to suggest is to be mindful about Doxygen comments which start /** and /**< See http://www.gnu.org/software/libcdio/doxygen/ecma__167_8h.html And as I look at it now, boy it looks ugly and needs attention. > Haven't applied it to my branch as I want to see if it looks sound to you, > or if you prefer something different. > Again that's find. Just apply without mixing other commits in so that I can work towards testing just this piece of it. (Or saying this in the converse: mixing multiple concerns into a commit will make it harder to disentangle and test each concern.) > Also note that I'm adding an assert for the size of udf_file_entry_t in > udf_open. > That's fine. It's okay to use asserts and for things like this. > > 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 <[email protected]> > 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 > > >
