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

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

[Markus Armbruster]

First of all, my apologies for being so late with this.  I realized
part way through the current development cycle that I couldn't do both
the error work and my half of the block probing work we discussed back
in November, so I punted the latter to the next cycle, missing the one
little feature I quite obviously could do.

Why is this "RFC for-2.3"?  The patch is simple, and quite obviously
does nothing unless you run with --no-format-probing.  If libvirt
wants it in 2.3, then I think we should consider it even at this late
stage.  If libvirt doesn't want it, or won't try to make use of it for
another few months, I'm happy to drop the patch now and revisit the
larger topic in the next cycle.

I readily admit --no-format-probing is fugly.  Better ideas are
welcome.

Markus Armbruster (1):
  block: New command line option --no-format-probing

 block.c               |  9 ++++++++-
 include/block/block.h |  2 +-
 qemu-options.hx       | 12 ++++++++++++
 vl.c                  |  6 +++++-
 4 files changed, 26 insertions(+), 3 deletions(-)

-- 
1.9.3

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

[Markus Armbruster]

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

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

[Max Reitz]

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. :-)

Max

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

[Markus Armbruster]

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.

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

[Max Reitz]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On 03/20/2015 07:49 AM, Max Reitz wrote:

>>>> Disabling probing entirely is a (hamfisted) way to ensure you always
>>>> specify the format.
>>>>

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

I'm totally in favor of the idea; it has no drawback to users that don't
add it to the command line (so no chance of regression to existing
command lines), and allows users that care to ensure that they are being
secure.  And I can argue that this is a bug fix rather than a feature
and therefore appropriate even in freeze (even though the bug has been a
long-standing security hole, rather than a recent regression).

If (a working version of) this makes it in 2.3, libvirt WILL use it in
the next release.  It will take me less than 5 minutes to write up the
libvirt patch, as long as the new option is advertised via
query-command-line-options (which means that QMP introspection of the
new option is a must for v2 :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/20/2015 07:49 AM, Max Reitz wrote:
>
>>>>> Disabling probing entirely is a (hamfisted) way to ensure you always
>>>>> specify the format.
>>>>>
>
>>>
>>> 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.
>
> I'm totally in favor of the idea; it has no drawback to users that don't
> add it to the command line (so no chance of regression to existing
> command lines), and allows users that care to ensure that they are being
> secure.  And I can argue that this is a bug fix rather than a feature
> and therefore appropriate even in freeze (even though the bug has been a
> long-standing security hole, rather than a recent regression).
>
> If (a working version of) this makes it in 2.3, libvirt WILL use it in
> the next release.  It will take me less than 5 minutes to write up the
> libvirt patch, as long as the new option is advertised via
> query-command-line-options (which means that QMP introspection of the
> new option is a must for v2 :)

query-command-line-options covers only options using QemuOpts.  Fixing
that defect isn't in the cards for 2.3, which means I'll have to
implement it with QemuOpts.

Ways to do that:

* Stick it into an existing QemuOpts option.  Is there one that fits?
  --machine doesn't really fit.

* Create a new QemuOpts option for miscellaneous settings.  Would --misc
  format-probing=off be too ugly?  Got a better name than --misc?

  Existing miscellaneous non-QemeOpts options could then (in 2.4!)
  become sugar for something in this option group, thus become available
  with -readconfig.

Thoughts?

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

On 03/20/2015 08:19 AM, Markus Armbruster wrote:

>> If (a working version of) this makes it in 2.3, libvirt WILL use it in
>> the next release.  It will take me less than 5 minutes to write up the
>> libvirt patch, as long as the new option is advertised via
>> query-command-line-options (which means that QMP introspection of the
>> new option is a must for v2 :)

The real libvirt requirement is that the new option is accessible via
_some_ QMP command (doesn't have to be query-command-line-options, but
that is the most obvious).

> 
> query-command-line-options covers only options using QemuOpts.  Fixing
> that defect isn't in the cards for 2.3, which means I'll have to
> implement it with QemuOpts.

Makes sense, and is certainly easier than figuring out an alternative
QMP probing mechanism.

> 
> Ways to do that:
> 
> * Stick it into an existing QemuOpts option.  Is there one that fits?
>   --machine doesn't really fit.

Mostly agree; --machine is more for what the guest sees, while this has
no impact on the guest ABI.  On the other hand, things  like -machine
accel=[tcg|kvm] aren't necessarily guest visible either, so -machine has
tended to be a catch-all for other things.  But that doesn't mean it
should continue to be one.

> 
> * Create a new QemuOpts option for miscellaneous settings.  Would --misc
>   format-probing=off be too ugly?  Got a better name than --misc?

The only clients using the new option will be machine generated, so I
don't think ugly appearance is a show-stopper.  And -misc really does
sound like it is the most amenable to adding other random flags in the
future, so I can live with that name.

Unless anyone else has a better idea, '-misc format-probing=off' has my
vote and works for libvirt; it is only a two-line change to my libvirt
RFC patch:

diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
index b452a75..f7a46c1 100644
--- i/src/qemu/qemu_capabilities.c
+++ w/src/qemu/qemu_capabilities.c
@@ -2517,7 +2517,7 @@ static struct virQEMUCapsCommandLineProps
virQEMUCapsCommandLine[] = {
     { "msg", "timestamp", QEMU_CAPS_MSG_TIMESTAMP },
     { "numa", NULL, QEMU_CAPS_NUMA },
     { "drive", "throttling.bps-total-max", QEMU_CAPS_DRIVE_IOTUNE_MAX },
-    { "no-format-probing", NULL, QEMU_CAPS_NO_FORMAT_PROBING },
+    { "misc", "format-probing", QEMU_CAPS_NO_FORMAT_PROBING },
 };

 static int
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 1085639..08f6560 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -8674,7 +8674,7 @@ qemuBuildCommandLine(virConnectPtr conn,
     }
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_FORMAT_PROBING) &&
         !cfg->allowDiskFormatProbing)
-        virCommandAddArg(cmd, "-no-format-probing");
+        virCommandAddArgList(cmd, "-misc", "format-probing=off", NULL);

     /* Serial graphics adapter */
     if (def->os.bios.useserial == VIR_TRISTATE_BOOL_YES) {


> 
>   Existing miscellaneous non-QemeOpts options could then (in 2.4!)
>   become sugar for something in this option group, thus become available
>   with -readconfig.

Nice way to plan for future cleanups.  I also agree that while this one
option about format-probing is reasonable to get into 2.3, that any
other sugar cleanups are better deferred until after the freeze.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Paolo Bonzini]

On 20/03/2015 15:19, Markus Armbruster wrote:
> > If (a working version of) this makes it in 2.3, libvirt WILL use it in
> > the next release.  It will take me less than 5 minutes to write up the
> > libvirt patch, as long as the new option is advertised via
> > query-command-line-options (which means that QMP introspection of the
> > new option is a must for v2 :)

Why can't libvirt just add ,format=raw instead of leaving out the format
key altogether?

Paolo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On 03/23/2015 11:23 AM, Paolo Bonzini wrote:
> 
> 
> On 20/03/2015 15:19, Markus Armbruster wrote:
>>> If (a working version of) this makes it in 2.3, libvirt WILL use it in
>>> the next release.  It will take me less than 5 minutes to write up the
>>> libvirt patch, as long as the new option is advertised via
>>> query-command-line-options (which means that QMP introspection of the
>>> new option is a must for v2 :)
> 
> Why can't libvirt just add ,format=raw instead of leaving out the format
> key altogether?

Libvirt DOES add format=raw.  This patch is an extra insurance policy to
guarantee that libvirt does not have any code paths that omit the
explicit format (as we have had a couple of CVEs in libvirt over the
years where that was the case).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 23/03/2015 18:48, Eric Blake wrote:
>> Why can't libvirt just add ,format=raw instead of leaving out the
>> format key altogether?
> 
> Libvirt DOES add format=raw.  This patch is an extra insurance
> policy to guarantee that libvirt does not have any code paths that
> omit the explicit format (as we have had a couple of CVEs in
> libvirt over the years where that was the case).

And where's the extra insurance policy to guarantee that QEMU does not
have any code paths that ignore the new command line option?

This is really borderline security theater.  Bugs happen, we fix them.
 Even better, Kevin now has implemented a strong mitigation for CVEs
like this, that won't allow guests to transmute a probed raw image
into another format.  There certainly hasn't been enough discussion
for this to get into 2.3.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJVEFJ+AAoJEL/70l94x66D/OEH/1j58fDg1W8XBjtaGQ12YsL6
HLKYaU2ObaxY3m5sX+mlMr1ftn/5kQnwVC7xx88xDCq/UG+GuSBRrT+SbxZtkdl4
SM9d0fATaK3yC0o0q3SWXeURAvi0bVOEoGqdpvwgrgGTcGkZPzsh9TwQySkupa8J
mQns/HTF3b7JWJvoVCseTOP99Hq+6+2DmWFbzyfisah/f2nlgNhPULSj0KZQmWxP
dMHPn9PG3NXV3E/xelTXWsMDuJKnnMu3w5MbULbNYDkwJe2f5bBOl6/AV4zqHZ5U
49Ewb1Mdcw+6r3aro2kCQ3wEYKnEpLb/Mb6Lj/i6OUXbA+0TlBWX906BBze+6SI=
=BWO8
-----END PGP SIGNATURE-----

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 23/03/2015 18:48, Eric Blake wrote:
>>> Why can't libvirt just add ,format=raw instead of leaving out the
>>> format key altogether?
>> 
>> Libvirt DOES add format=raw.  This patch is an extra insurance
>> policy to guarantee that libvirt does not have any code paths that
>> omit the explicit format (as we have had a couple of CVEs in
>> libvirt over the years where that was the case).
>
> And where's the extra insurance policy to guarantee that QEMU does not
> have any code paths that ignore the new command line option?

Right here (in the non-RFC patch, not this one):

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

The option sets bdrv_image_probing_disabled in a straightforward manner,
and bdrv_image_probing_disabled guards the probing code in an equally
straightforward manner.

> This is really borderline security theater.

I don't think so.  It's optional insurance agains libvirt bugs, and
libvirt wants to buy it.

>                                              Bugs happen, we fix them.

Yes.  Doesn't mean we shouldn't proactively mitigate bugs.

>  Even better, Kevin now has implemented a strong mitigation for CVEs
> like this, that won't allow guests to transmute a probed raw image
> into another format.

Yes.  As we discussed at some length back then, Kevin's mitigation is
not airtight and not without risk, but we decided it's worth having all
the same, in particular since it provides a good amount of protection
for users unwilling or unable to always specify formats.

How to get p0wned anyway:

1. Use your raw image with format=raw.  Malicious guest writes qcow2
header.

2. Use the image again without a format.  Probe returns qcow2, no
warning is printed.

Plausible attack?  Maybe not.  Worth stopping cold anyway?  I think so,
as long as it's easy.

My new option is a different kind of mitigation.  It's for users who
want more airtight protection, prefer writes to sector 0 just work,
funny or not, and are willing to always specify the format.  At least
one such user exists: libvirt.

Without this patch:

* Alternate use of raw images with and without format is still insecure;
  Kevin's mitigation can't protect you then.

* If you somehow miss specifying a format, and probing detects raw, you
  get a warning on stderr.  If your guest writes something funny to
  sector 0, the write fails.

With this patch:

* If you somehow miss specifying a format, open fails.  This is what
  libvirt wants.

>                       There certainly hasn't been enough discussion
> for this to get into 2.3.

Isn't that what were doing now?  :)

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

[Paolo Bonzini]

On 23/03/2015 21:19, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 23/03/2015 18:48, Eric Blake wrote:
>>>> Why can't libvirt just add ,format=raw instead of leaving out the
>>>> format key altogether?
>>>
>>> Libvirt DOES add format=raw.  This patch is an extra insurance
>>> policy to guarantee that libvirt does not have any code paths that
>>> omit the explicit format (as we have had a couple of CVEs in
>>> libvirt over the years where that was the case).
>>
>> And where's the extra insurance policy to guarantee that QEMU does not
>> have any code paths that ignore the new command line option?
> 
> Right here (in the non-RFC patch, not this one):
> 
> @@ -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 "
> 
> The option sets bdrv_image_probing_disabled in a straightforward manner,
> and bdrv_image_probing_disabled guards the probing code in an equally
> straightforward manner.

But what about migration from newer to older QEMU?  Libvirt even
supports QEMU versions where the only way to specify disks is "-hda
XYZ", so it is _impossible_ to honor the format=raw specifier.

Also, libvirt can start qemu-nbd and doesn't force format=raw in that
case.  So the protection is far from complete.  This reinforces my
opinion that the false sense of safety provided by this patch is worse
than the "insurance" against future CVEs (also, have there been any
actual libvirt CVEs about this after 2010?  near misses don't count IMHO).

> How to get p0wned anyway:
> 
> 1. Use your raw image with format=raw.  Malicious guest writes qcow2
> header.
> 
> 2. Use the image again without a format.  Probe returns qcow2, no
> warning is printed.
> 
> Plausible attack?  Maybe not.  Worth stopping cold anyway?  I think so,
> as long as it's easy.
> 
> My new option is a different kind of mitigation.  It's for users who
> want more airtight protection, prefer writes to sector 0 just work,
> funny or not, and are willing to always specify the format.  At least
> one such user exists: libvirt.
> 
> Without this patch:
> 
> * Alternate use of raw images with and without format is still insecure;
>   Kevin's mitigation can't protect you then.

That's PEBKAC.

Paolo

> * If you somehow miss specifying a format, and probing detects raw, you
>   get a warning on stderr.  If your guest writes something funny to
>   sector 0, the write fails.
> 
> With this patch:
> 
> * If you somehow miss specifying a format, open fails.  This is what
>   libvirt wants.
> 
>>                       There certainly hasn't been enough discussion
>> for this to get into 2.3.
> 
> Isn't that what were doing now?  :)
> 
> 

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On 03/24/2015 02:37 AM, Paolo Bonzini wrote:

>> The option sets bdrv_image_probing_disabled in a straightforward manner,
>> and bdrv_image_probing_disabled guards the probing code in an equally
>> straightforward manner.
> 
> But what about migration from newer to older QEMU?  Libvirt even
> supports QEMU versions where the only way to specify disks is "-hda
> XYZ", so it is _impossible_ to honor the format=raw specifier.

No one migrates from new qemu with this option back to a qemu version
that old.  Libvirt continues to drive old qemu, but driving old qemu is
different than migrating to old qemu.  And this feature is
introspectible, so libvirt knows when to use it and when to avoid it.

Furthermore, libvirt already has a knob in /etc/libvirt/qemu.conf to
enable probing - if this command line option ever gets in the way, a
one-line change to that conf file will tell libvirt to quit using it.

> 
> Also, libvirt can start qemu-nbd and doesn't force format=raw in that
> case.  So the protection is far from complete.  This reinforces my

Sounds like we have a bug to fix in libvirt.

> opinion that the false sense of safety provided by this patch is worse
> than the "insurance" against future CVEs (also, have there been any
> actual libvirt CVEs about this after 2010?  near misses don't count IMHO).

CVE-2011-2178 (http://security.libvirt.org/2011/0003.html).

And more recently, I argued that
http://security.libvirt.org/2014/0006.html should have been a CVE; it
was no near miss (in the wild for several months), and the only reason I
did not win my case for making it a CVE was because of the qemu.conf
default setting.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 23/03/2015 21:19, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 23/03/2015 18:48, Eric Blake wrote:
>>>>> Why can't libvirt just add ,format=raw instead of leaving out the
>>>>> format key altogether?
>>>>
>>>> Libvirt DOES add format=raw.  This patch is an extra insurance
>>>> policy to guarantee that libvirt does not have any code paths that
>>>> omit the explicit format (as we have had a couple of CVEs in
>>>> libvirt over the years where that was the case).
>>>
>>> And where's the extra insurance policy to guarantee that QEMU does not
>>> have any code paths that ignore the new command line option?
>> 
>> Right here (in the non-RFC patch, not this one):
>> 
>> @@ -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 "
>> 
>> The option sets bdrv_image_probing_disabled in a straightforward manner,
>> and bdrv_image_probing_disabled guards the probing code in an equally
>> straightforward manner.
>
> But what about migration from newer to older QEMU?  Libvirt even
> supports QEMU versions where the only way to specify disks is "-hda
> XYZ", so it is _impossible_ to honor the format=raw specifier.

If you migrate to a QEMU that doesn't understand the new option, libvirt
simply won't set it.  You lose the protection against libvirt bugs it
provides.  Guest won't notice.

If you somehow manage to find a QEMU old enough to make libvirt use
format-incapable interfaces, you'll be using insecure format probing on
the destination.  My patch makes no difference.  Good luck migrating to
such an old QEMU.

> Also, libvirt can start qemu-nbd and doesn't force format=raw in that
> case.  So the protection is far from complete.  This reinforces my
> opinion that the false sense of safety provided by this patch is worse
> than the "insurance" against future CVEs

The question whether the feature should be added to utilities is valid.
Me neglecting to add it in this patch can be a reason to reject this
patch, but hardly a reason for throwing out the idea wholesale.

>                                          (also, have there been any
> actual libvirt CVEs about this after 2010?  near misses don't count IMHO).

See Eric's testimony.

As to "near misses don't count": for me, what counts is actual users
telling me about their difficulties using QEMU securely.  Secure usage
shouldn't be hard.

>> How to get p0wned anyway:
>> 
>> 1. Use your raw image with format=raw.  Malicious guest writes qcow2
>> header.
>> 
>> 2. Use the image again without a format.  Probe returns qcow2, no
>> warning is printed.
>> 
>> Plausible attack?  Maybe not.  Worth stopping cold anyway?  I think so,
>> as long as it's easy.
>> 
>> My new option is a different kind of mitigation.  It's for users who
>> want more airtight protection, prefer writes to sector 0 just work,
>> funny or not, and are willing to always specify the format.  At least
>> one such user exists: libvirt.
>> 
>> Without this patch:
>> 
>> * Alternate use of raw images with and without format is still insecure;
>>   Kevin's mitigation can't protect you then.
>
> That's PEBKAC.

Secure usage shouldn't be hard.  When it is, then PEBKAC is blaming the
victim.

Today, secure usage requires you to always specify the format, on the
command line, in monitor commands, in every image referencing another
image (e.g. COW referencing backing file), and so forth.  There's images
all the way down.

Users tell us this is hard to get right and keep right, especially as
they race to keep up with our rapidly evolving block subsystem.

As long as they get it right, my proposed option doesn't make a
difference.  What it does is giving them a choice of failure modes for
when they fail:

1. The current failure mode, which maximizes backward compatibility and
   ease of use, except secure usage is inconveniently hard.  This is the
   default.

2. An alternate mode that fails early, hard and safe on insecure use.
   This is what libvirt wants to use.

> Paolo
>
>> * If you somehow miss specifying a format, and probing detects raw, you
>>   get a warning on stderr.  If your guest writes something funny to
>>   sector 0, the write fails.
>> 
>> With this patch:
>> 
>> * If you somehow miss specifying a format, open fails.  This is what
>>   libvirt wants.
>> 
>>>                       There certainly hasn't been enough discussion
>>> for this to get into 2.3.
>> 
>> Isn't that what were doing now?  :)

I chatted with Kevin, and we agree it's too late for 2.3 now, given the
ongoing discussion, but we'd like to pursue this further in 2.4.

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

[Paolo Bonzini]

On 24/03/2015 17:49, Markus Armbruster wrote:
>> But what about migration from newer to older QEMU?  Libvirt even
>> supports QEMU versions where the only way to specify disks is "-hda
>> XYZ", so it is _impossible_ to honor the format=raw specifier.
> 
> If you migrate to a QEMU that doesn't understand the new option, libvirt
> simply won't set it.  You lose the protection against libvirt bugs it
> provides.  Guest won't notice.
> 
> If you somehow manage to find a QEMU old enough to make libvirt use
> format-incapable interfaces, you'll be using insecure format probing on
> the destination.  My patch makes no difference.  Good luck migrating to
> such an old QEMU.

(Didn't mean live migration---sorry, could have simply said "switch").

Based on my reading of the code, libvirt will actually ignore the
allow_disk_format_probing setting, and not do anything about the format
when driving such an old QEMU.  By contrast, if you specify a format and
libvirt invokes an old qemu-nbd without --format, libvirt fails hard.
That's already CVE worthy, isn't it?

So I think an option like this is premature.  libvirt should _first of
all_ ensure that it completely abides by the allow_disk_format_probing
setting, including refusing to drive old QEMUs when format probing is
disabled.  Once libvirt is consistent within itself, we can talk about
what help QEMU can provide.

Perfect is not the enemy of good here.  Good is the enemy of secure.

> As to "near misses don't count": for me, what counts is actual users
> telling me about their difficulties using QEMU securely.  Secure usage
> shouldn't be hard.

The right answer for them is probably "use libvirt" or "use Boxes"
depending on the actual usecase.  Invoking QEMU manually is almost never
the right answer for random untrusted images download from the Internet.
 Also, I suspect any advice to QEMU users about adding
--no-format-probing would be quickly forgotten.

That said, if _humans_ have interest in secure use of QEMU, that's a
much better and more interesting use case than libvirt's, because
libvirt is itself providing a secure management layer.

We have other security options, for example seccomp and FIPS mode.
Format probing definitely falls in this category.  Let's add first of
all a -security grouping, where "-security [all=]on" enables all of them
but it's also possible to control the suboptions individually.  Then we
can add format probing to this category.  The same options can be added
to the utilities.

Let's iron out the kinks and do it for 2.4.  It's a very useful feature
indeed.

But it's something we do for _users_, not for libvirt.  If libvirt wants
to use those features as a parachute, better for them.  But I still
maintain that for libvirt this is basically security theater, and the
priorities are others.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/03/2015 17:49, Markus Armbruster wrote:
>>> But what about migration from newer to older QEMU?  Libvirt even
>>> supports QEMU versions where the only way to specify disks is "-hda
>>> XYZ", so it is _impossible_ to honor the format=raw specifier.
>> 
>> If you migrate to a QEMU that doesn't understand the new option, libvirt
>> simply won't set it.  You lose the protection against libvirt bugs it
>> provides.  Guest won't notice.
>> 
>> If you somehow manage to find a QEMU old enough to make libvirt use
>> format-incapable interfaces, you'll be using insecure format probing on
>> the destination.  My patch makes no difference.  Good luck migrating to
>> such an old QEMU.
>
> (Didn't mean live migration---sorry, could have simply said "switch").
>
> Based on my reading of the code, libvirt will actually ignore the
> allow_disk_format_probing setting, and not do anything about the format
> when driving such an old QEMU.  By contrast, if you specify a format and
> libvirt invokes an old qemu-nbd without --format, libvirt fails hard.
> That's already CVE worthy, isn't it?
>
> So I think an option like this is premature.  libvirt should _first of
> all_ ensure that it completely abides by the allow_disk_format_probing
> setting, including refusing to drive old QEMUs when format probing is
> disabled.  Once libvirt is consistent within itself, we can talk about
> what help QEMU can provide.

If we had a "no format probing" option in qemu-nbd (not in my patch, but
that's a flaw in the patch, not the idea), then libvirt developers
would've put it to use right away, and then their insecure usage
would've failed hard, making it immediately obvious.

My point is: we don't have to perfect libvirt before we can provide it a
safe failure mode.  The safe failure mode will actually assist with
perfecting libvirt, by making the failures immediately fatal.

> Perfect is not the enemy of good here.  Good is the enemy of secure.
>
>> As to "near misses don't count": for me, what counts is actual users
>> telling me about their difficulties using QEMU securely.  Secure usage
>> shouldn't be hard.
>
> The right answer for them is probably "use libvirt" or "use Boxes"
> depending on the actual usecase.  Invoking QEMU manually is almost never
> the right answer for random untrusted images download from the Internet.

I agree secure usage is currently is too hard for casual command line
use.

Unfortunately, it has proven too hard even for sophisticated
programmatic users like libvirt.  I refuse to blame libvirt for that.
Secure usage really shouldn't be that hard.

>  Also, I suspect any advice to QEMU users about adding
> --no-format-probing would be quickly forgotten.

When we discussed security issues with probing last year, my first line
of defense was "the default should be secure".  Overwhelming opposition
from the backward compatibility camp forced me to retreat from that line
to my second line of defense "secure shouldn't be hard".  That line
needs to be held at all costs :)

"Always specify --no-format-probing" (or however else we label the knob)
is workable advice for programmatic users like libvirt.  I agree it's
hardly helpful advice for human users.  For them, the advice needs to be
something like "add the following stanza to ~/.qemu.conf, then forget
about it".

Same thing, different user interface.

~/.qemu.conf isn't currently used, but it could be.

> That said, if _humans_ have interest in secure use of QEMU, that's a
> much better and more interesting use case than libvirt's, because
> libvirt is itself providing a secure management layer.

Debating which use case is "better" and more interesting doesn't seem to
be wortwhile.  I think both are interesting enough.

> We have other security options, for example seccomp and FIPS mode.
> Format probing definitely falls in this category.  Let's add first of
> all a -security grouping, where "-security [all=]on" enables all of them
> but it's also possible to control the suboptions individually.  Then we
> can add format probing to this category.  The same options can be added
> to the utilities.

Putting it in a security option group is certainly fine with me.  I
picked "misc" only because I couldn't find a better fit, and I could see
other readconfig-incapable options where I couldn't find a better fit,
either.

> Let's iron out the kinks and do it for 2.4.  It's a very useful feature
> indeed.
>
> But it's something we do for _users_, not for libvirt.

For whom you want it to get done isn't as important to me as getting it
done :)

>                                                         But I still
> maintain that for libvirt this is basically security theater, and the
> priorities are others.

To be honest, I find your use of "security theater" offensive.

I'd readily accept the moniker if the feature would provide libvirt
developers an excuse not to fix their bugs, or reduce the incentives to
fix their bugs.

But the feature in fact does the opposite: it makes such bugs blatantly
obvious and release-blocker-painful.

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

[Paolo Bonzini]

On 25/03/2015 09:10, Markus Armbruster wrote:
>> Based on my reading of the code, libvirt will actually ignore the
>> allow_disk_format_probing setting, and not do anything about the format
>> when driving such an old QEMU.  By contrast, if you specify a format and
>> libvirt invokes an old qemu-nbd without --format, libvirt fails hard.
>> That's already CVE worthy, isn't it?
>>
>> So I think an option like this is premature.  libvirt should _first of
>> all_ ensure that it completely abides by the allow_disk_format_probing
>> setting, including refusing to drive old QEMUs when format probing is
>> disabled.  Once libvirt is consistent within itself, we can talk about
>> what help QEMU can provide.
> 
> If we had a "no format probing" option in qemu-nbd (not in my patch, but
> that's a flaw in the patch, not the idea), then libvirt developers
> would've put it to use right away, and then their insecure usage
> would've failed hard, making it immediately obvious.

It wouldn't have fixed the bug in _old QEMU without -drive_ (and thus
without the option!), where allow_disk_format_probing does nothing.
Which is a vulnerability!  I noticed now that Eric is not CCed, adding him.

qemu-nbd invocations always require an explicit disk format in libvirt,
and fail hard on old qemu-nbd.

In the end we have:

- one bug for old QEMUs, so libvirt would not use the option

- one case where libvirt is stricter and requires a format, so the
option does not add anything

> I agree secure usage is currently is too hard for casual command line
> use.  Unfortunately, it has proven too hard even for sophisticated
> programmatic users like libvirt.

I disagree.  Libvirt has been doing well.  Hard---yes; but not too hard.

> When we discussed security issues with probing last year, my first line
> of defense was "the default should be secure".  Overwhelming opposition
> from the backward compatibility camp forced me to retreat from that line
> to my second line of defense "secure shouldn't be hard".  That line
> needs to be held at all costs :)

I agree.  Making the default secure is difficult because the old-style
options (-hda, -cdrom, etc.) are still good for casual use.  When I'm
boot-testing 20 different QEMUs to check that a series is bisectable,
it's generally okay to use IDE disks and cache=writeback, and it also
shouldn't matter if my image is raw or qcow2.

And making "secure" as easy as "--security on" is a worthwhile goal indeed.

That said, I'm not sure it's attainable.  For example, most downloadable
images are provided in qcow2 format.  The question then is: if users
can't be bothered to use -drive format=raw all the time, why would they
be bothered to use -drive format=qcow2 instead?

Users want security _and_ DWIM.  Anvil, meet hammer.

>>                                                         But I still
>> maintain that for libvirt this is basically security theater, and the
>> priorities are others.
> 
> To be honest, I find your use of "security theater" offensive.
> 
> I'd readily accept the moniker if the feature would provide libvirt
> developers an excuse not to fix their bugs, or reduce the incentives to
> fix their bugs.

I say it again: libvirt should first be consistent within itself.
Either it should drop support for old QEMUs, or it should be audited to
ensure that old QEMUs are invoked securely.

Once old QEMUs are deemed okay, we can add smarties to new QEMUs.  If
you don't do it in this order, the smarties indeed reduce the incentive
to fix bugs.

Also, I'm calling it security theater because on the surface the option
seems to provide a mitigation strategy, but in the end doesn't provide
much.  Because...

> But the feature in fact does the opposite: it makes such bugs blatantly
> obvious and release-blocker-painful.

... they are only obvious inasmuch as there is a testcase for "no
explicitly specified format".  Otherwise, you have a hidden failure for
new QEMU and a vulnerability for old QEMU, so you don't gain anything.

Problem is, the same person write code and testcases.  If you are smart
enough to add testcases that catch possible vulnerability, you probably
are already catching it in the code.  (And the testcase is a way of
praising yourself for your forethought. :)).

Paolo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On 03/20/2015 07:05 AM, Markus Armbruster wrote:
> First of all, my apologies for being so late with this.  I realized
> part way through the current development cycle that I couldn't do both
> the error work and my half of the block probing work we discussed back
> in November, so I punted the latter to the next cycle, missing the one
> little feature I quite obviously could do.
> 
> Why is this "RFC for-2.3"?  The patch is simple, and quite obviously
> does nothing unless you run with --no-format-probing.  If libvirt
> wants it in 2.3, then I think we should consider it even at this late
> stage.  If libvirt doesn't want it, or won't try to make use of it for
> another few months, I'm happy to drop the patch now and revisit the
> larger topic in the next cycle.
> 
> I readily admit --no-format-probing is fugly.  Better ideas are
> welcome.

So, is it spelled '--no-format-probing', or is it spelled
'-no-format-probing'?  It is in the same category as other flag
parameters like '-enable-fips' or '-no-user-config'.

> 
> Markus Armbruster (1):
>   block: New command line option --no-format-probing
> 
>  block.c               |  9 ++++++++-
>  include/block/block.h |  2 +-
>  qemu-options.hx       | 12 ++++++++++++
>  vl.c                  |  6 +++++-
>  4 files changed, 26 insertions(+), 3 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/20/2015 07:05 AM, Markus Armbruster wrote:
>> First of all, my apologies for being so late with this.  I realized
>> part way through the current development cycle that I couldn't do both
>> the error work and my half of the block probing work we discussed back
>> in November, so I punted the latter to the next cycle, missing the one
>> little feature I quite obviously could do.
>> 
>> Why is this "RFC for-2.3"?  The patch is simple, and quite obviously
>> does nothing unless you run with --no-format-probing.  If libvirt
>> wants it in 2.3, then I think we should consider it even at this late
>> stage.  If libvirt doesn't want it, or won't try to make use of it for
>> another few months, I'm happy to drop the patch now and revisit the
>> larger topic in the next cycle.
>> 
>> I readily admit --no-format-probing is fugly.  Better ideas are
>> welcome.
>
> So, is it spelled '--no-format-probing', or is it spelled
> '-no-format-probing'?  It is in the same category as other flag
> parameters like '-enable-fips' or '-no-user-config'.

Any QEMU option can be spelled both as -NAME and as --NAME.

Rolling your very own command line parser is foolish, but the only way
to stay design-flaw-compatible.

[Qemu-devel] [RFC PATCH] qemu: enforce no format probing when possible

[Eric Blake]

qemu 2.3 added an option, with this documentation:

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

Use it when libvirt is configured to avoid probing, to ensure we
catch libvirt failures on avoiding probing before such bugs can
escalate into another CVE.

* src/qemu/qemu_capabilities.h (QEMU_CAPS_NO_FORMAT_PROBING): New
capability bit.
* src/qemu/qemu_capabilities.c (virQEMUCapsCommandLine): Set it.
* src/qemu/qemu_command.c (qemuBuildCommandLine): Use it.

Signed-off-by: Eric Blake <eblake@redhat.com>
CC: qemu-devel@nongnu.org
---

RFC because I need to enhance the libvirt testsuite to prove we set
the option, and because the qemu side has not been committed yet
(and may therefore change the final spelling for the new option).

 src/qemu/qemu_capabilities.c | 7 +++++--
 src/qemu/qemu_capabilities.h | 3 ++-
 src/qemu/qemu_command.c      | 3 +++
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ccf22f0..b452a75 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1,7 +1,7 @@
 /*
  * qemu_capabilities.c: QEMU capabilities generation
  *
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -279,6 +279,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               "qxl.vgamem_mb",
               "qxl-vga.vgamem_mb",
               "pc-dimm",
+
+              "no-format-probing", /* 185 */
     );


@@ -2514,7 +2516,8 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
     { "spice", "disable-agent-file-xfer", QEMU_CAPS_SPICE_FILE_XFER_DISABLE },
     { "msg", "timestamp", QEMU_CAPS_MSG_TIMESTAMP },
     { "numa", NULL, QEMU_CAPS_NUMA },
-    { "drive", "throttling.bps-total-max", QEMU_CAPS_DRIVE_IOTUNE_MAX},
+    { "drive", "throttling.bps-total-max", QEMU_CAPS_DRIVE_IOTUNE_MAX },
+    { "no-format-probing", NULL, QEMU_CAPS_NO_FORMAT_PROBING },
 };

 static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index c7b1ac7..2cdcd81 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -1,7 +1,7 @@
 /*
  * qemu_capabilities.h: QEMU capabilities generation
  *
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -224,6 +224,7 @@ typedef enum {
     QEMU_CAPS_QXL_VGAMEM         = 182, /* -device qxl.vgamem_mb */
     QEMU_CAPS_QXL_VGA_VGAMEM     = 183, /* -device qxl-vga.vgamem_mb */
     QEMU_CAPS_DEVICE_PC_DIMM     = 184, /* pc-dimm device */
+    QEMU_CAPS_NO_FORMAT_PROBING  = 185, /* -no-format-probing */

     QEMU_CAPS_LAST,                   /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 63d43d4..1085639 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8672,6 +8672,9 @@ qemuBuildCommandLine(virConnectPtr conn,
             virCommandAddArg(cmd, "-nodefconfig");
         virCommandAddArg(cmd, "-nodefaults");
     }
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_FORMAT_PROBING) &&
+        !cfg->allowDiskFormatProbing)
+        virCommandAddArg(cmd, "-no-format-probing");

     /* Serial graphics adapter */
     if (def->os.bios.useserial == VIR_TRISTATE_BOOL_YES) {
-- 
2.1.0