[Spice-commits] 2 commits - configure.ac server/memslot.c server/tests

[GitLab Mirror]

 configure.ac                    |    4 ++--
 server/memslot.c                |    4 ++--
 server/tests/test-qxl-parsing.c |   30 ++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

New commits:
commit a4a16ac42d2f19a17e36556546aa94d5cd83745f
Author: Christophe Fergeau <cfergeau at redhat.com>
Date:   Thu Nov 29 14:18:39 2018 +0100

    memslot: Fix off-by-one error in group/slot boundary check
    
    RedMemSlotInfo keeps an array of groups, and each group contains an
    array of slots. Unfortunately, these checks are off by 1, they check
    that the index is greater or equal to the number of elements in the
    array, while these arrays are 0 based. The check should only check for
    strictly greater than the number of elements.
    
    For the group array, this is not a big issue, as these memslot groups
    are created by spice-server users (eg QEMU), and the group ids used to
    index that array are also generated by the spice-server user, so it
    should not be possible for the guest to set them to arbitrary values.
    
    The slot id is more problematic, as it's calculated from a QXLPHYSICAL
    address, and such addresses are usually set by the guest QXL driver, so
    the guest can set these to arbitrary values, including malicious values,
    which are probably easy to build from the guest PCI configuration.
    
    This patch fixes the arrays bound check, and adds a test case for this.
    This fixes CVE-2019-3813.
    
    Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
    Acked-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/memslot.c b/server/memslot.c
index c2931321..97311b2e 100644
--- a/server/memslot.c
+++ b/server/memslot.c
@@ -97,13 +97,13 @@ void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
 
     MemSlot *slot;
 
-    if (group_id > info->num_memslots_groups) {
+    if (group_id >= info->num_memslots_groups) {
         spice_critical("group_id too big");
         return NULL;
     }
 
     slot_id = memslot_get_id(info, addr);
-    if (slot_id > info->num_memslots) {
+    if (slot_id >= info->num_memslots) {
         print_memslots(info);
         spice_critical("slot_id %d too big, addr=%" PRIx64, slot_id, addr);
         return NULL;
diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c
index 324f7fdc..edccfee4 100644
--- a/server/tests/test-qxl-parsing.c
+++ b/server/tests/test-qxl-parsing.c
@@ -85,6 +85,31 @@ static void deinit_qxl_surface(QXLSurfaceCmd *qxl)
     g_free(from_physical(qxl->u.surface_create.data));
 }
 
+static void test_memslot_invalid_group_id(void)
+{
+    RedMemSlotInfo mem_info;
+    init_meminfo(&mem_info);
+
+    memslot_get_virt(&mem_info, 0, 16, 1);
+}
+
+static void test_memslot_invalid_slot_id(void)
+{
+    RedMemSlotInfo mem_info;
+    init_meminfo(&mem_info);
+
+    memslot_get_virt(&mem_info, 1 << mem_info.memslot_id_shift, 16, 0);
+}
+
+static void test_memslot_invalid_addresses(void)
+{
+    g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/group_id", 0, 0);
+    g_test_trap_assert_stderr("*group_id too big*");
+
+    g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/slot_id", 0, 0);
+    g_test_trap_assert_stderr("*slot_id 1 too big*");
+}
+
 static void test_no_issues(void)
 {
     RedMemSlotInfo mem_info;
@@ -269,6 +294,11 @@ int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
 
+    /* try to use invalid memslot group/slot */
+    g_test_add_func("/server/memslot-invalid-addresses", test_memslot_invalid_addresses);
+    g_test_add_func("/server/memslot-invalid-addresses/subprocess/group_id", test_memslot_invalid_group_id);
+    g_test_add_func("/server/memslot-invalid-addresses/subprocess/slot_id", test_memslot_invalid_slot_id);
+
     /* try to create a surface with no issues, should succeed */
     g_test_add_func("/server/qxl-parsing-no-issues", test_no_issues);
 
commit 03d46e9e19cab32c432215fd842777bf1d6127f0
Author: Christophe Fergeau <cfergeau at redhat.com>
Date:   Tue Feb 5 12:17:11 2019 +0100

    build-sys: Raise glib requirement to 2.38
    
    meson is already using 2.38, and most distros have a newer version:
        - Fedora 28 has 2.56
        - CentOS 7 has 2.46
        - Debian 9 has 2.50
    
    This also matches what spice-common requires.
    
    Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
    Acked-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
    Acked-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/configure.ac b/configure.ac
index fa79af7f..604a41b2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -165,8 +165,8 @@ SPICE_PROTOCOL_MIN_VER=0.12.16
 PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= $SPICE_PROTOCOL_MIN_VER])
 AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 
-GLIB2_REQUIRED=2.32
-GLIB2_ENCODED_VERSION="GLIB_VERSION_2_32"
+GLIB2_REQUIRED=2.38
+GLIB2_ENCODED_VERSION="GLIB_VERSION_2_38"
 PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= $GLIB2_REQUIRED gio-2.0 >= $GLIB2_REQUIRED])
 GLIB2_CFLAGS="$GLIB2_CFLAGS -DGLIB_VERSION_MIN_REQUIRED=$GLIB2_ENCODED_VERSION \
   -DGLIB_VERSION_MAX_ALLOWED=$GLIB2_ENCODED_VERSION"