Re: [Qemu-devel] Re: [PATCH] Make default invocation of block drivers safer

[Anthony Liguori <anthony@codemonkey.ws>]

On 07/14/2010 11:42 AM, Kevin Wolf wrote:
> Am 14.07.2010 18:12, schrieb Anthony Liguori:
>    
>> CVE-2008-2004 described a vulnerability in QEMU whereas a malicious user could
>> trick the block probing code into accessing arbitrary files in a guest.  To
>> mitigate this, we added an explicit format parameter to -drive which disabling
>> block probing.
>>
>> Fast forward to today, and the vast majority of users do not use this parameter.
>> libvirt does not use this by default nor does virt-manager.
>>
>> Most users want block probing so we should try to make it safer.
>>
>> This patch adds some logic to the raw device which attempts to detect a write
>> operation to the beginning of a raw device.  If the first 4 bytes happen to
>> match an image file that has a backing file that we support, it scrubs the
>> signature to all zeros.  If a user specifies an explicit format parameter, this
>> behavior is disabled.
>>
>> I contend that while a legitimate guest could write such a signature to the
>> header, we would behave incorrectly anyway upon the next invocation of QEMU.
>> This simply changes the incorrect behavior to not involve a security
>> vulnerability.
>>
>> I've tested this pretty extensively both in the positive and negative case.  I'm
>> not 100% confident in the block layer's ability to deal with zero sized writes
>> particularly with respect to the aio functions so some additional eyes would be
>> appreciated.
>>
>> Even in the case of a single sector write, we have to make sure to invoked the
>> completion from a bottom half so just removing the zero sized write is not an
>> option.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>      
> I guess something like this makes sense, and the approach looks okay in
> general. With the check that we have really probed the format, we still
> allow legitimate use cases (whatever they might be).
>
> However, I wonder why you even bother with adjusting buffers and
> requests and stuff instead of just returning a straight -EIO. Doing so
> would have the additional advantage that the expectation of the guest OS
> matches what is really on the disk (garbage) instead of silently
> corrupting things.
>    

I started with that approach.  My concern is that it would trigger the 
stop-on-error behavior and the result would be far too difficult for a 
management tool/person to deal with.

Scrubbing seemed like a easier-to-use solution.

>>   static BlockDriverAIOCB *raw_aio_writev(BlockDriverState *bs,
>>       int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>>       BlockDriverCompletionFunc *cb, void *opaque)
>>   {
>> +    if (check_write_unsafe(bs, sector_num, qiov->iov[0].iov_base, nb_sectors)) {
>>      
> Have you checked that the bad value is always in iov[0]? Could a guest
> construct a zero-length iov[0] and do the bad access in iov[1]? Or use
> two two-byte buffers to write the magic number?
>
> I'm not saying that any of these work, I honestly don't know, but did
> you consider them?
>    

No, and it's certainly worth being a bit more paranoid.

Regards,

Anthony Liguori

> Kevin
>
>