Sanitize symlink resolution

ChangeLog

@@ -1,3 +1,12 @@
+Wed Apr  1 11:22:22 BST 2009 Daniel P. Berrange <berrange@redhat.com>
+
+	Sanitise symlink resolving
+	* src/libvirt_private.syms: Add virFileResolveLink
+	* src/util.c, src/util.h: Add convenient virFileResolveLink
+	for reading symlink destination safely
+	* src/storage_backend_disk.c, src/security_selinux.c: Switch
+	over to calling virFileResolveLink
+
 Wed Apr  1 11:18:22 BST 2009 Daniel P. Berrange <berrange@redhat.com>
 
 	Misc memory handling fixes

src/libvirt_private.syms

@@ -306,6 +306,7 @@ virStrToLong_ll;
 virStrToLong_ull;
 virStrToLong_ui;
 virFileLinkPointsTo;
+virFileResolveLink;
 saferead;
 safewrite;
 safezero;

src/security_selinux.c

@@ -293,28 +293,24 @@ SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
     struct stat buf;
     security_context_t fcon = NULL;
     int rc = -1;
+    int err;
     char *newpath = NULL;
     const char *path = disk->src;
 
     if (disk->readonly || disk->shared)
         return 0;
 
-    if (lstat(path, &buf) != 0)
+    if ((err = virFileResolveLink(path, &newpath)) < 0) {
-        return -1;
+        virReportSystemError(conn, err,
-
+                             _("cannot resolve symlink %s"), path);
-    if (S_ISLNK(buf.st_mode)) {
+        goto err;
-        if (VIR_ALLOC_N(newpath, buf.st_size + 1) < 0)
-            return -1;
-
-        if (readlink(path, newpath, buf.st_size) < 0)
-            goto err;
-        path = newpath;
-        if (stat(path, &buf) != 0)
-            goto err;
     }
 
-    if (matchpathcon(path, buf.st_mode, &fcon) == 0)  {
+    if (stat(newpath, &buf) != 0)
-        rc = SELinuxSetFilecon(conn, path, fcon);
+        goto err;
+
+    if (matchpathcon(newpath, buf.st_mode, &fcon) == 0)  {
+        rc = SELinuxSetFilecon(conn, newpath, fcon);
     }
 err:
     VIR_FREE(fcon);

src/storage_backend_disk.c

@@ -362,20 +362,16 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
                                unsigned int flags ATTRIBUTE_UNUSED)
 {
     char *part_num = NULL;
-    int n;
+    int err;
-    char devpath[PATH_MAX];
+    char *devpath = NULL;
     char *devname, *srcname;
+    int rc = -1;
 
-    if ((n = readlink(vol->target.path, devpath, sizeof(devpath))) < 0 &&
+    if ((err = virFileResolveLink(vol->target.path, &devpath)) < 0) {
-        errno != EINVAL) {
+        virReportSystemError(conn, err,
-        virReportSystemError(conn, errno,
                              _("Couldn't read volume target path '%s'"),
                              vol->target.path);
-        return -1;
+        goto cleanup;
-    } else if (n <= 0) {
-        strncpy(devpath, vol->target.path, PATH_MAX);
-    } else {
-        devpath[n] = '\0';
     }
 
     devname = basename(devpath);
@@ -386,7 +382,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
         virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
                               _("Volume path '%s' did not start with parent "
                                 "pool source device name."), devname);
-        return -1;
+        goto cleanup;
     }
 
     part_num = devname + strlen(srcname);
@@ -395,7 +391,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
         virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
                               _("cannot parse partition number from target "
                                 "'%s'"), devname);
-        return -1;
+        goto cleanup;
     }
 
     /* eg parted /dev/sda rm 2 */
@@ -409,9 +405,12 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
     };
 
     if (virRun(conn, prog, NULL) < 0)
-        return -1;
+        goto cleanup;
 
-    return 0;
+    rc = 0;
+cleanup:
+    VIR_FREE(devpath);
+    return rc;
 }
 
 

src/util.c

@@ -937,6 +937,53 @@ int virFileLinkPointsTo(const char *checkLink,
             && SAME_INODE (src_sb, dest_sb));
 }
 
+
+
+/*
+ * Attempt to resolve a symbolic link, returning the
+ * real path
+ *
+ * Return 0 if path was not a symbolic, or the link was
+ * resolved. Return -1 upon error
+ */
+int virFileResolveLink(const char *linkpath,
+                       char **resultpath)
+{
+    struct stat st;
+    char *buf;
+    int n;
+
+    *resultpath = NULL;
+
+    if (lstat(linkpath, &st) < 0)
+        return errno;
+
+    if (!S_ISLNK(st.st_mode)) {
+        if (!(*resultpath = strdup(linkpath)))
+            return -ENOMEM;
+        return 0;
+    }
+
+    /* Posix says that 'st_size' field from
+     * result of an lstat() call is filled with
+     * number of bytes in the destination
+     * filename.
+     */
+    if (VIR_ALLOC_N(buf, st.st_size + 1) < 0)
+        return -ENOMEM;
+
+    if ((n = readlink(linkpath, buf, st.st_size)) < 0) {
+        VIR_FREE(buf);
+        return -errno;
+    }
+
+    buf[n] = '\0';
+
+    *resultpath = buf;
+    return 0;
+}
+
+
 int virFileExists(const char *path)
 {
     struct stat st;

src/util.h

@@ -87,6 +87,9 @@ int virFileStripSuffix(char *str,
 int virFileLinkPointsTo(const char *checkLink,
                         const char *checkDest);
 
+int virFileResolveLink(const char *linkpath,
+                       char **resultpath);
+
 int virFileExists(const char *path);
 
 int virFileMakePath(const char *path);