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

[John Snow <jsnow@redhat.com>]

On 01/03/2015 06:54 AM, Peter Wu wrote:
> 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

No: I was just asking the questions. If dmg2img gets away with it, the 
worst that will happen is we will fail to parse/load a DMG file because 
we ignore everything without the "mish" magic, so this is OK.

I just wanted to check, since I didn't have a lot of DMG files on-hand 
and I couldn't really find a fuller reference to the types of XML that 
shows up.

Thanks!

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