Re: [Qemu-devel] [PATCH 06/10] block/dmg: process XML plists

[Peter Wu <peter@lekensteyn.nl>]

On Friday 02 January 2015 19:04:32 John Snow wrote:
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > The format is simple enough to avoid using a full-blown XML parser.
> > The offsets are based on the description at
> > http://newosxbook.com/DMG.html
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> >   block/dmg.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 69 insertions(+)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 19e4fe2..c03ea01 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -26,6 +26,7 @@
> >   #include "qemu/bswap.h"
> >   #include "qemu/module.h"
> >   #include <zlib.h>
> > +#include <glib.h>
> >
> >   enum {
> >       /* Limit chunk sizes to prevent unreasonable amounts of memory being used
> > @@ -333,12 +334,66 @@ fail:
> >       return ret;
> >   }
> >
> > +static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
> > +                              uint64_t info_begin, uint64_t info_length)
> > +{
> > +    BDRVDMGState *s = bs->opaque;
> > +    int ret;
> > +    uint8_t *buffer = NULL;
> > +    char *data_begin, *data_end;
> > +
> > +    /* Have at least some length to avoid NULL for g_malloc. Attempt to set a
> > +     * safe upper cap on the data length. A test sample had a XML length of
> > +     * about 1 MiB. */
> > +    if (info_length == 0 || info_length > 16 * 1024 * 1024) {
> > +        ret = -EINVAL;
> > +        goto fail;
> > +    }
> > +
> > +    buffer = g_malloc(info_length + 1);
> > +    buffer[info_length] = '\0';
> > +    ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
> > +    if (ret != info_length) {
> > +        ret = -EINVAL;
> > +        goto fail;
> > +    }
> > +
> > +    /* look for <data>...</data>. The data is 284 (0x11c) bytes after base64
> > +     * decode. The actual data element has 431 (0x1af) bytes which includes tabs
> > +     * and line feeds. */
> > +    data_end = (char *)buffer;
> > +    while ((data_begin = strstr(data_end, "<data>")) != NULL) {
> > +        gsize out_len = 0;
> > +
> > +        data_begin += 6;
> > +        data_end = strstr(data_begin, "</data>");
> > +        /* malformed XML? */
> > +        if (data_end == NULL) {
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +        *data_end++ = '\0';
> > +        g_base64_decode_inplace(data_begin, &out_len);
> > +        ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
> > +                                  (uint32_t)out_len);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +    }
> > +    ret = 0;
> > +
> > +fail:
> > +    g_free(buffer);
> > +    return ret;
> > +}
> > +
> 
> This starts to make me a little nervous, because we're ignoring so much 
> of the XML document structure here and just effectively performing a 
> regular search for "<data>(.*)</data>".
> 
> Can we guarantee that the ONLY time the data element is used in this 
> document is when it is being used in the exact context we are expecting 
> here, where it contains the b64 mish data we expect it to?
> 
> i.e. it is always in a path like this as detailed by 
> http://newosxbook.com/DMG.html :
> 
> plist/dict/key[text()='resource-fork']/following-sibling::dict/key[text()='blkx']/following-sibling::array/dict/key[text()='data']/following-sibling::data
> 
> I notice that this document says other sections MAY be present, do any 
> of them ever need to be parsed? Has anyone written about them before?
> 
> Do we know if any use data sections?
> 
> I suppose at the very least, sections of interest are always going to 
> include the "mish" magic, so that should probably keep us from doing 
> anything too stupid ...

I did not find DMG files with <data> elements at other locations. If it
would occur, at worst we would fail to parse a DMG file. I think that
introducing a XML parser here would introduce a risk for a minor benefit
(being prepared for future cases).

Since this is a property list, in theory people could include all kinds
of data for different keys (which would then be matched by the current
implementation). But how likely is this for a disk image?

FWIW, I looked into the dmg2img program and that also looks for the
strings "<data>" and "</data>". Nobody has raised a bug for that program
so far.

Do you think that it is worth to use a XML parser on potentially
insecure data? I suggest to keep it as it, and reconsider a different
approach in case a problem is encountered.

Kind regards,
Peter

> >   static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> >                       Error **errp)
> >   {
> >       BDRVDMGState *s = bs->opaque;
> >       DmgHeaderState ds;
> >       uint64_t rsrc_fork_offset, rsrc_fork_length;
> > +    uint64_t plist_xml_offset, plist_xml_length;
> >       int64_t offset;
> >       int ret;
> >
> > @@ -366,12 +421,26 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> >       if (ret < 0) {
> >           goto fail;
> >       }
> > +    /* offset of property list (XMLOffset) */
> > +    ret = read_uint64(bs, offset + 0xd8, &plist_xml_offset);
> > +    if (ret < 0) {
> > +        goto fail;
> > +    }
> > +    ret = read_uint64(bs, offset + 0xe0, &plist_xml_length);
> > +    if (ret < 0) {
> > +        goto fail;
> > +    }
> >       if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
> >           ret = dmg_read_resource_fork(bs, &ds,
> >                                        rsrc_fork_offset, rsrc_fork_length);
> >           if (ret < 0) {
> >               goto fail;
> >           }
> > +    } else if (plist_xml_offset != 0 && plist_xml_length != 0) {
> > +        ret = dmg_read_plist_xml(bs, &ds, plist_xml_offset, plist_xml_length);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> >       } else {
> >           ret = -EINVAL;
> >           goto fail;
> >