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

[Daniel P. Berrange]

On Mon, Oct 22, 2012 at 05:05:19PM +0530, Harsh Bora wrote:
> 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 ?

I mean to populate a virURIPtr object and then call virURIFormat
to turn it into a string, thus guaranteeing that escaping is
properly done.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|