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

[Rocky Bernstein]

Thanks for the investigation, information and patches. UDF support hasn't
had much love for a long while, if ever.

UDF support is something that a number of people have asked for, but no one
was willing to step up to the plate to work on. For myself, it's just not
something I feel like I want to work on even though, yes, I started that
code.

I'm okay with applying these patches as is on the theory that it moves code
forward which is better than keeping it slightly broken. (Sadly, progress
sometimes is only made this way in libcdio. We have crappy code which is
only good enough to lure someone into deciding to investigate some aspect
that they need or are interested in; and then they rewrite it. I'd say for
example CD-Text code was fixed up in this way)

But given that you are not totally comfortable with this, perhaps we should
hold off  and mull on this for a while.

On Mon, Jan 16, 2012 at 9:11 PM, Pete Batard <address@hidden> wrote:

> 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<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
>
>
>