[libvirt] [PATCH] storage: add check for invalid volume name
If volume name is a path, storageVolCreateXML appends that name to the specified pool path, that will taint other pools. Adding the volume check is better for sanity. Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/storage/storage_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2cb8347..c3b807f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -49,6 +49,7 @@ #include "configmake.h" #include "virstring.h" #include "viraccessapicheck.h" +#include "dirname.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1864,6 +1865,14 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } + /* Make sure the volume name is not a path */ + if (last_component(newvol->name) != newvol->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage volume name '%s' is a path"), + newvol->name); + goto cleanup; + } + /* Is there ever a valid case for this? */ if (newvol->target.capacity < origvol->target.capacity) newvol->target.capacity = origvol->target.capacity; -- 1.8.3.1
On 04/10/2014 10:02 PM, Jincheng Miao wrote:
If volume name is a path, storageVolCreateXML appends that name to the specified pool path, that will taint other pools. Adding the volume check is better for sanity.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/storage/storage_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+)
+ /* Make sure the volume name is not a path */ + if (last_component(newvol->name) != newvol->name) {
Why not just: if (strchr(newvol->name, '/')) Also, shouldn't we forbid things like "." and ".." as the newvol name?
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage volume name '%s' is a path"),
I don't like the word "path" in a user-facing error message for anything other than a list of directory names separated by colon; better would be a message such as "requested storage volume name '%s' cannot contain /" or "requested storage volume name '%s' not permitted". -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/11/2014 12:28 PM, Eric Blake wrote:
On 04/10/2014 10:02 PM, Jincheng Miao wrote:
If volume name is a path, storageVolCreateXML appends that name to the specified pool path, that will taint other pools. Adding the volume check is better for sanity.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/storage/storage_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+)
+ /* Make sure the volume name is not a path */ + if (last_component(newvol->name) != newvol->name) { Why not just:
if (strchr(newvol->name, '/'))
sure.
Also, shouldn't we forbid things like "." and ".." as the newvol name?
I think we should just forbid '/' in the newvol name. Why that is illegal? Consider this situation: There is a directory pool named 'taint-pool' for a *normal user*. If privileged user creates a volume clone, with path-based name, pointer to this 'taint-pool'. Then this normal user could see this volume clone after pool-refresh. If normal user vol-delete this volume clone, then this clone belongs to privileged user gone. I think that is insane. If you agree with it, I will send V2 patch with modification.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage volume name '%s' is a path"), I don't like the word "path" in a user-facing error message for anything other than a list of directory names separated by colon; better would be a message such as "requested storage volume name '%s' cannot contain /" or "requested storage volume name '%s' not permitted".
Yes, "requested storage volume name '%s' cannot contain /" is more precise.
participants (2)
-
Eric Blake -
Jincheng Miao