[libvirt] [PATCH v2 1/2] Qemu/Gluster: Add Gluster protocol as supported network disk formats.

[Harsh Bora]

On 10/05/2012 03:13 AM, Daniel P. Berrange wrote:
> On Thu, Oct 04, 2012 at 09:17:08PM +0530, Deepak C Shetty wrote:
>>> +        virBufferAsprintf(opt, "gluster+%s://",
>>> +                          virDomainDiskProtocolTransportTypeToString(disk->hosts->transport));
>>> +
>>> +        /* if transport type is not unix, specify server:port */
>>> +        if (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
>>> +            if (strstr(disk->hosts->name, ":")) {
>> Wouldn't it be better to check for ':' and check for absence of "."
>> (and vice versa in the else) so that if someone specified a.b.c:d
>> or a:b:c:d:e.f we can throw error rite away, instead of qemu
>> erroring out later in time ? Its a very primtive check but
>> can help to catch user input error early enuf.
>>
>>> +                /* if IPv6 addr, use square brackets to enclose it */
>>> +                virBufferAsprintf(opt, "[%s]:%s", disk->hosts->name, disk->hosts->port);
>>> +            } else {
>>> +                virBufferAsprintf(opt, "%s:%s", disk->hosts->name, disk->hosts->port);
>>> +            }
>>> +        }
>>> +
>>> +        /* append source path to gluster disk image */
>>> +        virBufferAsprintf(opt, "/%s", disk->src);
>>> +
>>> +        /* if transport type is unix, server name is path to unix socket, ignore port */
>>> +        if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
>>> +            virBufferAsprintf(opt, "?socket=%s", disk->hosts->name);
>>> +        }
>> This can be clubbed as part of else clause to the above if condn (if
>> (disk->hosts->transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX)), that
>> ways we avoid an un-necessary check of transport here.
>> It also means that disk->src needs to be pulled inside the if & else
>> clauses, which I feel should be ok.
>
> Since this code is building up a URI, it really should be re-written to
> use the virURIPtr object, instead of virBufferPtr. This will ensure that
> things like [] for IPv6 are handled automatically, as well as doing the
> correct escaping of parameter values. This code is quite possibly
> exploitable as it stands if the user provided cleverly crafted values
> for disks->hosts->name which abused URI syntax

Hi Daniel,

I had a look at src/util/viruri.c and found that we have virURIParse() 
which takes a const char* and returns a virURIPtr object, likewise we 
have virURIFormat() which take a virURIPtr and returns a char*.

Are you suggesting to manually populate a virURI struct and pass its 
address to virURIFormat() to build the cmdline from libvirt XML or are 
you suggesting to first build a cmdline (virBuffer) from the XML and 
pass it to virURIParse to validate if the libvirt XML has been provided 
a valid URI ?

Also, IIUC, the usage of virURIPtr is going to be limited inside this 
function only, since the calling code expects the passed virBuffer to be 
populated with the required command line, right?

regards,
Harsh

>
> Daniel
>