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

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

On 07/15/2010 03:13 AM, Kevin Wolf wrote:
> Am 14.07.2010 19:54, 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>
>> ---
>> v1 ->  v2
>>   - be more paranoid about empty iovecs
>> ---
>>   block.c     |    4 ++
>>   block/raw.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block_int.h |    1 +
>>   3 files changed, 134 insertions(+), 0 deletions(-)
>>
>>      
>    
>>   static BlockDriverAIOCB *raw_aio_writev(BlockDriverState *bs,
>>       int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>>       BlockDriverCompletionFunc *cb, void *opaque)
>>   {
>> +    const uint8_t *first_buf;
>> +    int first_buf_index = 0, i;
>> +
>> +    /* This is probably being paranoid, but handle cases of zero size
>> +       vectors. */
>> +    for (i = 0; i<  qiov->niov; i++) {
>> +        if (qiov->iov[i].iov_len) {
>> +            first_buf_index = i;
>> +            break;
>> +        }
>> +    }
>> +
>> +    first_buf = qiov->iov[first_buf_index].iov_base;
>>      
> It's still not paranoid enough for the case where the magic is spread
> over multiple buffers. We should probably have a qemu_iovec_to_buffer()
> with limited size so that you can just get 4 bytes into a temporary buffer.
>    

I'm quite confident that iovec buffers are never less than 512 in size.  
While it could be more paranoid, I don't think the added complexity helps.

Regards,

Anthony Liguori

> Kevin
>