[Spice-devel] [spice-server] memslot: Fix off-by-one error in group/slot boundary check

[Frediano Ziglio]

> 
> 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>

Does not compile with standard options:


test-qxl-parsing.c: In function 'test_memslot_invalid_addresses':
test-qxl-parsing.c:106:5: error: 'g_test_trap_subprocess' is deprecated: Not available before 2.38 [-Werror=deprecated-declarations]
     g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/group_id", 0, 0);
     ^~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/glib-2.0/glib.h:85,
                 from ../../server/red-common.h:26,
                 from ../../server/memslot.h:24,
                 from test-qxl-parsing.c:32:
/usr/include/glib-2.0/glib/gtestutils.h:270:10: note: declared here
 void     g_test_trap_subprocess         (const char           *test_path,
          ^~~~~~~~~~~~~~~~~~~~~~
test-qxl-parsing.c:109:5: error: 'g_test_trap_subprocess' is deprecated: Not available before 2.38 [-Werror=deprecated-declarations]
     g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/slot_id", 0, 0);
     ^~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/glib-2.0/glib.h:85,
                 from ../../server/red-common.h:26,
                 from ../../server/memslot.h:24,
                 from test-qxl-parsing.c:32:
/usr/include/glib-2.0/glib/gtestutils.h:270:10: note: declared here
 void     g_test_trap_subprocess         (const char           *test_path,
          ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


> ---
> Forgot to send this for upstream when the CVE went public :-/
> 
>  server/memslot.c                |  4 ++--
>  server/tests/test-qxl-parsing.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/server/memslot.c b/server/memslot.c
> index c29313214..97311b2e5 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 324f7fdc9..edccfee47 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);
>