Re: CVE-2019-3813: spice: Off-by-one error in array access in spice/server/memslot.c

[Peter Korsgaard <peter () korsgaard com>]

> > > > > "Scott" == Scott Gayou <sgayou () redhat com> writes:

> Hello,
> spice versions 0.5.2 through 0.14.1 are vulnerable to an out-of-bounds read
> due to an off-by-one error in memslot_get_virt. This may lead to a
> denial-of-service, or, in the worst case, code-execution by unauthenticated
> attackers.

> The attached patch fixes the issue in spice and is planned to be included
> in forthcoming release spice 0.14.2.

> This issue was reported by Christophe Fergeau (Red Hat).

> References:
> https://bugzilla.redhat.com/show_bug.cgi?id=1665371

> Thank you.

> -- 
> Scott Gayou / Red Had Product Security

> From 6eff47e72cb2f23d168be58bab8bdd60df49afd0 Mon Sep 17 00:00:00 2001
> From: Christophe Fergeau <cfergeau () redhat com>
> Date: Thu, 29 Nov 2018 14:18:39 +0100
> Subject: [spice-server] 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.

> Signed-off-by: Christophe Fergeau <cfergeau () redhat com>
> ---
>  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 b27324efb..fb3d5cfd5 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) {
>          g_critical("group_id too big");

What version is this patch against? I don't see memslot.c using
g_critical() neither on the 0.14 branch (which doesn't have 0.14.1) or
master?

https://gitlab.freedesktop.org/spice/spice/blob/master/server/memslot.c#L97

-- 
Bye, Peter Korsgaard