libvirt/CVE-2010-223x-0003.patch

>From f8d3e740cee4903bd2eef1a072e8190f5e9e92b9 Mon Sep 17 00:00:00 2001
From: Daniel P. Berrange <berrange@redhat.com>
Date: Tue, 15 Jun 2010 14:58:10 +0100
Subject: [PATCH 03/10] Refactor virStorageFileGetMetadataFromFD to separate functionality

The virStorageFileGetMetadataFromFD did two jobs in one. First
it probed for storage type, then it extracted metadata for the
type. It is desirable to be able to separate these jobs, allowing
probing without querying metadata, and querying metadata without
probing.

To prepare for this, split out probing code into a new pair of
methods

  virStorageFileProbeFormatFromFD
  virStorageFileProbeFormat

* src/util/storage_file.c, src/util/storage_file.h,
  src/libvirt_private.syms: Introduce virStorageFileProbeFormat
  and virStorageFileProbeFormatFromFD
---
 src/libvirt_private.syms |    2 +
 src/util/storage_file.c  |  393 ++++++++++++++++++++++++++++++++--------------
 src/util/storage_file.h  |    4 +
 3 files changed, 281 insertions(+), 118 deletions(-)

Index: libvirt-0.8.1/src/libvirt_private.syms
===================================================================
--- libvirt-0.8.1.orig/src/libvirt_private.syms
+++ libvirt-0.8.1/src/libvirt_private.syms
@@ -619,6 +619,8 @@ virStorageGenerateQcowPassphrase;
 # storage_file.h
 virStorageFileFormatTypeToString;
 virStorageFileFormatTypeFromString;
+virStorageFileProbeFormat;
+virStorageFileProbeFormatFromFD;
 virStorageFileGetMetadata;
 virStorageFileGetMetadataFromFD;
 
Index: libvirt-0.8.1/src/util/storage_file.c
===================================================================
--- libvirt-0.8.1.orig/src/util/storage_file.c
+++ libvirt-0.8.1/src/util/storage_file.c
@@ -85,8 +85,8 @@ static int vmdk4GetBackingStore(char **,
 
 
 static struct FileTypeInfo const fileTypeInfo[] = {
-    [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN, 0, 0, 0, 0, 0, 0, NULL },
-    [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN, 0, 0, 0, 0, 0, 0, NULL },
+    [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL },
+    [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL },
     [VIR_STORAGE_FILE_BOCHS] = {
         /*"Bochs Virtual HD Image", */ /* Untested */ NULL,
         NULL,
@@ -390,146 +390,302 @@ absolutePathFromBaseFile(const char *bas
     return res;
 }
 
+static int
+virStorageFileMatchesMagic(int format,
+                           unsigned char *buf,
+                           size_t buflen)
+{
+    int mlen;
+
+    if (fileTypeInfo[format].magic == NULL)
+        return 0;
+
+    /* Validate magic data */
+    mlen = strlen(fileTypeInfo[format].magic);
+    if (mlen > buflen)
+        return 0;
+
+    if (memcmp(buf, fileTypeInfo[format].magic, mlen) != 0)
+        return 0;
+
+    return 1;
+}
+
+
+static int
+virStorageFileMatchesExtension(int format,
+                               const char *path)
+{
+    if (fileTypeInfo[format].extension == NULL)
+        return 0;
+
+    if (virFileHasSuffix(path, fileTypeInfo[format].extension))
+        return 1;
+
+    return 0;
+}
+
+
+static int
+virStorageFileMatchesVersion(int format,
+                             unsigned char *buf,
+                             size_t buflen)
+{
+    int version;
+
+    /* Validate version number info */
+    if (fileTypeInfo[format].versionOffset == -1)
+        return 0;
+
+    if ((fileTypeInfo[format].versionOffset + 4) > buflen)
+        return 0;
+
+    if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) {
+        version =
+            (buf[fileTypeInfo[format].versionOffset+3] << 24) |
+            (buf[fileTypeInfo[format].versionOffset+2] << 16) |
+            (buf[fileTypeInfo[format].versionOffset+1] << 8) |
+            (buf[fileTypeInfo[format].versionOffset]);
+    } else {
+        version =
+            (buf[fileTypeInfo[format].versionOffset] << 24) |
+            (buf[fileTypeInfo[format].versionOffset+1] << 16) |
+            (buf[fileTypeInfo[format].versionOffset+2] << 8) |
+            (buf[fileTypeInfo[format].versionOffset+3]);
+    }
+    if (version != fileTypeInfo[format].versionNumber)
+        return 0;
+
+    return 1;
+}
+
+
+static int
+virStorageFileGetMetadataFromBuf(int format,
+                                 const char *path,
+                                 unsigned char *buf,
+                                 size_t buflen,
+                                 virStorageFileMetadata *meta)
+{
+    /* XXX we should consider moving virStorageBackendUpdateVolInfo
+     * code into this method, for non-magic files
+     */
+    if (!fileTypeInfo[format].magic) {
+        return 0;
+    }
+
+    /* Optionally extract capacity from file */
+    if (fileTypeInfo[format].sizeOffset != -1) {
+        if ((fileTypeInfo[format].sizeOffset + 8) > buflen)
+            return 1;
+
+        if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) {
+            meta->capacity =
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+7] << 56) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+6] << 48) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+5] << 40) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+4] << 32) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+3] << 24) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+2] << 16) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+1] << 8) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset]);
+        } else {
+            meta->capacity =
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset] << 56) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+1] << 48) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+2] << 40) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+3] << 32) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+4] << 24) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+5] << 16) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+6] << 8) |
+                ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+7]);
+        }
+        /* Avoid unlikely, but theoretically possible overflow */
+        if (meta->capacity > (ULLONG_MAX / fileTypeInfo[format].sizeMultiplier))
+            return 1;
+        meta->capacity *= fileTypeInfo[format].sizeMultiplier;
+    }
+
+    if (fileTypeInfo[format].qcowCryptOffset != -1) {
+        int crypt_format;
+
+        crypt_format =
+            (buf[fileTypeInfo[format].qcowCryptOffset] << 24) |
+            (buf[fileTypeInfo[format].qcowCryptOffset+1] << 16) |
+            (buf[fileTypeInfo[format].qcowCryptOffset+2] << 8) |
+            (buf[fileTypeInfo[format].qcowCryptOffset+3]);
+        meta->encrypted = crypt_format != 0;
+    }
+
+    if (fileTypeInfo[format].getBackingStore != NULL) {
+        char *backing;
+        int backingFormat;
+        int ret = fileTypeInfo[format].getBackingStore(&backing,
+                                                       &backingFormat,
+                                                       buf, buflen);
+        if (ret == BACKING_STORE_INVALID)
+            return 1;
+
+        if (ret == BACKING_STORE_ERROR)
+            return -1;
+
+        if (backing != NULL) {
+            meta->backingStore = absolutePathFromBaseFile(path, backing);
+            VIR_FREE(backing);
+            if (meta->backingStore == NULL) {
+                virReportOOMError();
+                return -1;
+            }
+            meta->backingStoreFormat = backingFormat;
+        } else {
+            meta->backingStore = NULL;
+            meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
+        }
+    }
+
+    return 0;
+}
+
+
+static int
+virStorageFileProbeFormatFromBuf(const char *path,
+                                 unsigned char *buf,
+                                 size_t buflen)
+{
+    int format = VIR_STORAGE_FILE_RAW;
+    int i;
+
+    /* First check file magic */
+    for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) {
+        if (virStorageFileMatchesMagic(i, buf, buflen) &&
+            virStorageFileMatchesVersion(i, buf, buflen)) {
+            format = i;
+            goto cleanup;
+        }
+    }
+
+    /* No magic, so check file extension */
+    for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) {
+        if (virStorageFileMatchesExtension(i, path)) {
+            format = i;
+            goto cleanup;
+        }
+    }
+
+cleanup:
+    return format;
+}
+
 /**
- * Probe the header of a file to determine what type of disk image
- * it is, and info about its capacity if available.
+ * virStorageFileProbeFormatFromFD:
+ *
+ * Probe for the format of 'fd' (which is an open file descriptor
+ * pointing to 'path'), returning the detected disk format.
+ *
+ * Callers are advised never to trust the returned 'format'
+ * unless it is listed as VIR_STORAGE_FILE_RAW, since a
+ * malicious guest can turn a file into any other non-raw
+ * format at will.
+ *
+ * Best option: Don't use this function
  */
 int
-virStorageFileGetMetadataFromFD(const char *path,
-                                int fd,
-                                virStorageFileMetadata *meta)
+virStorageFileProbeFormatFromFD(const char *path, int fd)
 {
     unsigned char head[20*512]; /* vmdk4GetBackingStore needs this much. */
-    int len, i;
+    int len;
 
-    /* If all else fails, call it a raw file */
-    meta->format = VIR_STORAGE_FILE_RAW;
+    if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
+        virReportSystemError(errno, _("cannot set to start of '%s'"), path);
+        return -1;
+    }
 
     if ((len = read(fd, head, sizeof(head))) < 0) {
         virReportSystemError(errno, _("cannot read header '%s'"), path);
         return -1;
     }
 
-    /* First check file magic */
-    for (i = 0 ; i < ARRAY_CARDINALITY(fileTypeInfo) ; i++) {
-        int mlen;
+    return virStorageFileProbeFormatFromBuf(path, head, len);
+}
 
-        if (fileTypeInfo[i].magic == NULL)
-            continue;
+/**
+ * virStorageFileProbeFormat:
+ *
+ * Probe for the format of 'path', returning the detected
+ * disk format.
+ *
+ * Callers are advised never to trust the returned 'format'
+ * unless it is listed as VIR_STORAGE_FILE_RAW, since a
+ * malicious guest can turn a file into any other non-raw
+ * format at will.
+ *
+ * Best option: Don't use this function
+ */
+int
+virStorageFileProbeFormat(const char *path)
+{
+    int fd, ret;
 
-        /* Validate magic data */
-        mlen = strlen(fileTypeInfo[i].magic);
-        if (mlen > len)
-            continue;
-        if (memcmp(head, fileTypeInfo[i].magic, mlen) != 0)
-            continue;
-
-        /* Validate version number info */
-        if (fileTypeInfo[i].versionNumber != -1) {
-            int version;
-
-            if (fileTypeInfo[i].endian == LV_LITTLE_ENDIAN) {
-                version = (head[fileTypeInfo[i].versionOffset+3] << 24) |
-                    (head[fileTypeInfo[i].versionOffset+2] << 16) |
-                    (head[fileTypeInfo[i].versionOffset+1] << 8) |
-                    head[fileTypeInfo[i].versionOffset];
-            } else {
-                version = (head[fileTypeInfo[i].versionOffset] << 24) |
-                    (head[fileTypeInfo[i].versionOffset+1] << 16) |
-                    (head[fileTypeInfo[i].versionOffset+2] << 8) |
-                    head[fileTypeInfo[i].versionOffset+3];
-            }
-            if (version != fileTypeInfo[i].versionNumber)
-                continue;
-        }
+    if ((fd = open(path, O_RDONLY)) < 0) {
+        virReportSystemError(errno, _("cannot open file '%s'"), path);
+        return -1;
+    }
 
-        /* Optionally extract capacity from file */
-        if (fileTypeInfo[i].sizeOffset != -1) {
-            if (fileTypeInfo[i].endian == LV_LITTLE_ENDIAN) {
-                meta->capacity =
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+7] << 56) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+6] << 48) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+5] << 40) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+4] << 32) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+3] << 24) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+2] << 16) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+1] << 8) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset]);
-            } else {
-                meta->capacity =
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset] << 56) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+1] << 48) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+2] << 40) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+3] << 32) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+4] << 24) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+5] << 16) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+6] << 8) |
-                    ((unsigned long long)head[fileTypeInfo[i].sizeOffset+7]);
-            }
-            /* Avoid unlikely, but theoretically possible overflow */
-            if (meta->capacity > (ULLONG_MAX / fileTypeInfo[i].sizeMultiplier))
-                continue;
-            meta->capacity *= fileTypeInfo[i].sizeMultiplier;
-        }
+    ret = virStorageFileProbeFormatFromFD(path, fd);
 
-        if (fileTypeInfo[i].qcowCryptOffset != -1) {
-            int crypt_format;
+    close(fd);
 
-            crypt_format = (head[fileTypeInfo[i].qcowCryptOffset] << 24) |
-                (head[fileTypeInfo[i].qcowCryptOffset+1] << 16) |
-                (head[fileTypeInfo[i].qcowCryptOffset+2] << 8) |
-                head[fileTypeInfo[i].qcowCryptOffset+3];
-            meta->encrypted = crypt_format != 0;
-        }
+    return ret;
+}
 
-        /* Validation passed, we know the file format now */
-        meta->format = i;
-        if (fileTypeInfo[i].getBackingStore != NULL) {
-            char *backing;
-            int backingFormat;
-
-            switch (fileTypeInfo[i].getBackingStore(&backing,
-                                                    &backingFormat,
-                                                    head, len)) {
-            case BACKING_STORE_OK:
-                break;
+/**
+ * virStorageFileGetMetadataFromFD:
+ *
+ * Probe for the format of 'fd' (which is an open file descriptor
+ * for the file 'path'), filling 'meta' with the detected
+ * format and other associated metadata.
+ *
+ * Callers are advised never to trust the returned 'meta->format'
+ * unless it is listed as VIR_STORAGE_FILE_RAW, since a
+ * malicious guest can turn a file into any other non-raw
+ * format at will.
+ */
+int
+virStorageFileGetMetadataFromFD(const char *path,
+                                int fd,
+                                virStorageFileMetadata *meta)
+{
+    unsigned char head[20*512]; /* vmdk4GetBackingStore needs this much. */
+    int len;
 
-            case BACKING_STORE_INVALID:
-                continue;
+    memset(meta, 0, sizeof (*meta));
 
-            case BACKING_STORE_ERROR:
-                return -1;
-            }
-            if (backing != NULL) {
-                meta->backingStore = absolutePathFromBaseFile(path, backing);
-                VIR_FREE(backing);
-                if (meta->backingStore == NULL) {
-                    virReportOOMError();
-                    return -1;
-                }
-                meta->backingStoreFormat = backingFormat;
-            } else {
-                meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
-            }
-        }
-        return 0;
+    if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
+        virReportSystemError(errno, _("cannot set to start of '%s'"), path);
+        return -1;
     }
 
-    /* No magic, so check file extension */
-    for (i = 0 ; i < ARRAY_CARDINALITY(fileTypeInfo) ; i++) {
-        if (fileTypeInfo[i].extension == NULL)
-            continue;
-
-        if (!virFileHasSuffix(path, fileTypeInfo[i].extension))
-            continue;
-
-        meta->format = i;
-        return 0;
+    if ((len = read(fd, head, sizeof(head))) < 0) {
+        virReportSystemError(errno, _("cannot read header '%s'"), path);
+        return -1;
     }
 
-    return 0;
+    meta->format = virStorageFileProbeFormatFromBuf(path, head, len);
+
+    return virStorageFileGetMetadataFromBuf(meta->format, path, head, len, meta);
 }
 
+/**
+ * virStorageFileGetMetadata:
+ *
+ * Probe for the format of 'path', filling 'meta' with the detected
+ * format and other associated metadata.
+ *
+ * Callers are advised never to trust the returned 'meta->format'
+ * unless it is listed as VIR_STORAGE_FILE_RAW, since a
+ * malicious guest can turn a file into any other non-raw
+ * format at will.
+ */
 int
 virStorageFileGetMetadata(const char *path,
                           virStorageFileMetadata *meta)
Index: libvirt-0.8.1/src/util/storage_file.h
===================================================================
--- libvirt-0.8.1.orig/src/util/storage_file.h
+++ libvirt-0.8.1/src/util/storage_file.h
@@ -57,6 +57,10 @@ typedef struct _virStorageFileMetadata {
 #  define DEV_BSIZE 512
 # endif
 
+int virStorageFileProbeFormat(const char *path);
+int virStorageFileProbeFormatFromFD(const char *path,
+                                    int fd);
+
 int virStorageFileGetMetadata(const char *path,
                               virStorageFileMetadata *meta);
 int virStorageFileGetMetadataFromFD(const char *path,