Re: [Qemu-devel] [PATCH RFC for-2.3 1/1] block: New command line option --no-format-probing

[Max Reitz <mreitz@redhat.com>]

On 2015-03-20 at 09:48, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> On 2015-03-20 at 09:05, Markus Armbruster wrote:
>>> Probing is convenient, but probing untrusted raw images is insecure
>>> (CVE-2008-2004).  To avoid it, users should always specify raw format
>>> explicitly.  This isn't trivial, and even sophisticated users have
>>> gotten it wrong (libvirt CVE-2010-2237, CVE-2010-2238, CVE-2010-2239,
>>> plus more recent variations of the theme that didn't get CVEs because
>>> they were caught before they could hurt users).
>>>
>>> Disabling probing entirely is a (hamfisted) way to ensure you always
>>> specify the format.
>>>
>>> Unfortunately, the new option is not available with -readconfig.
>>> There's no obvious option group to take it.  I think we could use a
>>> "miscellaneous" option group.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    block.c               |  9 ++++++++-
>>>    include/block/block.h |  2 +-
>>>    qemu-options.hx       | 12 ++++++++++++
>>>    vl.c                  |  6 +++++-
>>>    4 files changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 0fe97de..5865309 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -103,6 +103,7 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
>>>                                 int nr_sectors);
>>>    /* If non-zero, use only whitelisted block drivers */
>>>    static int use_bdrv_whitelist;
>>> +static bool bdrv_image_probing_disabled;
>>>      #ifdef _WIN32
>>>    static int is_windows_drive_prefix(const char *filename)
>>> @@ -751,6 +752,11 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
>>>            return ret;
>>>        }
>>>    +    if (bdrv_image_probing_disabled) {
>>> +        error_setg(errp, "Format not specified and image probing disabled");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>        ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>>>        if (ret < 0) {
>>>            error_setg_errno(errp, -ret, "Could not read image for determining its "
>>> @@ -4909,9 +4915,10 @@ void bdrv_init(void)
>>>        module_call_init(MODULE_INIT_BLOCK);
>>>    }
>>>    -void bdrv_init_with_whitelist(void)
>>> +void bdrv_init_with_whitelist(bool no_format_probing)
>>>    {
>>>        use_bdrv_whitelist = 1;
>>> +    bdrv_image_probing_disabled = no_format_probing;
>>>        bdrv_init();
>>>    }
>>>    diff --git a/include/block/block.h b/include/block/block.h
>>> index 4c57d63..b5a8b23 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -177,7 +177,7 @@ void bdrv_io_limits_enable(BlockDriverState *bs);
>>>    void bdrv_io_limits_disable(BlockDriverState *bs);
>>>      void bdrv_init(void);
>>> -void bdrv_init_with_whitelist(void);
>>> +void bdrv_init_with_whitelist(bool no_format_probing);
>>>    BlockDriver *bdrv_find_protocol(const char *filename,
>>>                                    bool allow_protocol_prefix,
>>>                                    Error **errp);
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 319d971..8aa4d7b 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -963,6 +963,18 @@ STEXI
>>>    Disable SDL window close capability.
>>>    ETEXI
>>>    +DEF("no-format-probing", 0, QEMU_OPTION_no_format_probing,
>>> +    "-no-format-probing\n"
>>> +    "                disable block image format probing\n", QEMU_ARCH_ALL)
>>> +STEXI
>>> +@item -no-format-probing
>>> +@findex -no-format-probing
>>> +Disable block image format probing.  Probing is convenient, but
>>> +probing untrusted raw images is insecure.  To avoid it, always specify
>>> +raw format explicitly.  Disabling probing entirely is a (hamfisted)
>>> +way to ensure you do.
>>> +ETEXI
>>> +
>>>    DEF("sdl", 0, QEMU_OPTION_sdl,
>>>        "-sdl            enable SDL\n", QEMU_ARCH_ALL)
>>>    STEXI
>>> diff --git a/vl.c b/vl.c
>>> index 75ec292..94d5e15 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -2754,6 +2754,7 @@ int main(int argc, char **argv, char **envp)
>>>    #endif
>>>        bool defconfig = true;
>>>        bool userconfig = true;
>>> +    bool no_format_probing = false;
>>>        const char *log_mask = NULL;
>>>        const char *log_file = NULL;
>>>        GMemVTable mem_trace = {
>>> @@ -2823,7 +2824,7 @@ int main(int argc, char **argv, char **envp)
>>>          nb_nics = 0;
>>>    -    bdrv_init_with_whitelist();
>>> +    bdrv_init_with_whitelist(no_format_probing);
>>>          autostart = 1;
>>>    @@ -3381,6 +3382,9 @@ int main(int argc, char **argv, char **envp)
>>>                case QEMU_OPTION_no_quit:
>>>                    no_quit = 1;
>>>                    break;
>>> +            case QEMU_OPTION_no_format_probing:
>>> +                no_format_probing = true;
>>> +                break;
>>>                case QEMU_OPTION_sdl:
>>>    #ifdef CONFIG_SDL
>>>                    display_type = DT_SDL;
>> You're setting no_format_probing after you're using it, so it doesn't
>> work very well. :-)
> I really should not test stuff on a hurried Friday afternoon...
>
> I'd appreciate opinions on whether this is wanted for 2.3.  If it is,
> I'll post a version that actually works.

I don't have any objections because it won't break anything. But I guess 
it'll be mostly up to whether Eric thinks that we'll need it right now.

Max