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

[Rocky Bernstein]

On Mon, Jan 23, 2012 at 12:43 PM, Pete Batard <address@hidden> 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 <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
>
>
>