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

[Markus Armbruster]

Max Reitz <address@hidden> 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 <address@hidden>
>> ---
>>   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
>> address@hidden -no-format-probing
>> address@hidden -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.