Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.11.2014 um 16:25 hat Stefan Hajnoczi geschrieben:
>> On Tue, Nov 04, 2014 at 11:11:33AM +0100, Kevin Wolf wrote:
>> > Am 03.11.2014 um 16:05 hat Stefan Hajnoczi geschrieben:
>> > > The argument that there might not be a traditional filename doesn't make
>> > > sense to me.  When there is no filename the command-line is already
>> > > sufficiently complex and usage is fancy enough that probing adds no
>> > > convenience, the user can just specify the format.
>> > 
>> > -hda nbd://localhost
>> > -drive file=nbd://localhost,format=raw
>> > 
>> > Almost double the length, and I don't see anything fancy in the first
>> > line.
>> > 
>> > > Anyway, does this sound reasonable:
>> > > 
>> > > In QEMU 3.0, require the format= option for -drive.  Keep probing the
>> > > way it is for non-drive options because they are used for convenience by
>> > > local users.
>> > 
>> > And being hacked while using -hda is better in which way?
>> 
>> Markus is proposing that we look at the filename extension.  In that
>> case QEMU cannot be tricked by the contents of a raw image.
>> 
>> That makes -hda perfectly safe although there are cases where QEMU
>> doesn't know what to do and requires format=.
>
> Wait, by "keep probing the way it is" you mean implementing one of the
> other proposals? So you're only suggesting being stricter on -drive as
> an additional measure?
>
>> I do worry that changing QEMU's probing behavior drastically can lead to
>> consistencies where libvirt does its own probing :(.  Haven't thought
>> through the bug scenarios but that could be a security problem in
>> itself.
>
> Hm... In which cases does libvirt probe the image format? And is it even
> consistent with qemu today?

I had a quick look at the source.  Eric, please correct
misunderstandings.

Enumation type virStorageFileProbeFormat enumerates supported formats.
It has pseudo-formats VIR_STORAGE_FILE_AUTO, VIR_STORAGE_FILE_AUTO_SAFE.

I don't understand VIR_STORAGE_FILE_AUTO_SAFE offhand.

VIR_STORAGE_FILE_AUTO means probing.  Its use appears to be deprecated.
Actual probing happens in virStorageFileProbeFormatFromBuf():

    For all formats:
        if magic and version match, pick this format

    If some magic matched, but not the version: warn

    For all formats:
        if file name extension matches, pick this format

    Pick raw.

The formats' magic, version and extension are defined in fileTypeInfo[].

If I remember correctly, libvirt has its own probing because running an
external program just to probe is too slow.

Another reason for having its own probing might be providing a secure
replacement for QEMU's insecure probing.

> If you can get libvirt to explicitly pass the wrong format=... option
> because it did its own probing, we have a problem no matter what we
> change in qemu.

Yes, but that would be a libvirt problem.  No excuse for us to ignore
our own problems.