Sanitize symlink resolution

diff --git a/ChangeLog b/ChangeLog
index 1afec5929e30b0745ed178185933982a5f4c219b..9f939b64f4d171310ca9a65d00d44cc9dd27cb71 100644 (file)
--- a/ChangeLog
+++ b/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

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a5f9f925a6295927e052783abc1a745a24f0679b..78f093c1965465635840c9dde02ab857fc5de5d9 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -306,6 +306,7 @@ virStrToLong_ll;
 virStrToLong_ull;
 virStrToLong_ui;
 virFileLinkPointsTo;
+virFileResolveLink;
 saferead;
 safewrite;
 safezero;

diff --git a/src/security_selinux.c b/src/security_selinux.c
index 1708d5508244479dcafbeed5376abe7ff0251bac..091fe6ee5a583b2c5f164f6f8a916ed54294b955 100644 (file)
--- a/src/security_selinux.c
+++ b/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)
-        return -1;
-
-    if (S_ISLNK(buf.st_mode)) {
-        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 ((err = virFileResolveLink(path, &newpath)) < 0) {
+        virReportSystemError(conn, err,
+                             _("cannot resolve symlink %s"), path);
+        goto err;
     }
 
-    if (matchpathcon(path, buf.st_mode, &fcon) == 0)  {
-        rc = SELinuxSetFilecon(conn, path, fcon);
+    if (stat(newpath, &buf) != 0)
+        goto err;
+
+    if (matchpathcon(newpath, buf.st_mode, &fcon) == 0)  {
+        rc = SELinuxSetFilecon(conn, newpath, fcon);
     }
 err:
     VIR_FREE(fcon);

diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c
index 9280906709f2262cdef5273c7a9610dc22573184..4b0da3d7d40814c0932b6e3b980e58e5731c712f 100644 (file)
--- a/src/storage_backend_disk.c
+++ b/src/storage_backend_disk.c
@@ -362,20 +362,16 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
                                unsigned int flags ATTRIBUTE_UNUSED)
 {
     char *part_num = NULL;
-    int n;
-    char devpath[PATH_MAX];
+    int err;
+    char *devpath = NULL;
     char *devname, *srcname;
+    int rc = -1;
 
-    if ((n = readlink(vol->target.path, devpath, sizeof(devpath))) < 0 &&
-        errno != EINVAL) {
-        virReportSystemError(conn, errno,
+    if ((err = virFileResolveLink(vol->target.path, &devpath)) < 0) {
+        virReportSystemError(conn, err,
                              _("Couldn't read volume target path '%s'"),
                              vol->target.path);
-        return -1;
-    } else if (n <= 0) {
-        strncpy(devpath, vol->target.path, PATH_MAX);
-    } else {
-        devpath[n] = '\0';
+        goto cleanup;
     }
 
     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;
 }
 
 

diff --git a/src/util.c b/src/util.c
index 016cb626d7af4c27d30f8570e26faea753f3d0f0..65088b04c57c625a6f09f5c1675cc76e831b5da1 100644 (file)
--- a/src/util.c
+++ b/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;

diff --git a/src/util.h b/src/util.h
index 3fd5d2511c304582e76b5c308577c29147fd440e..6fe03b634fb25f355df21c556f741eed369d4d97 100644 (file)
--- a/src/util.h
+++ b/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);