[prev in list] [next in list] [prev in thread] [next in thread] 

List:       oss-security
Subject:    Re: [oss-security] CVE-2019-3813: spice: Off-by-one error in array access in spice/server/memslot.c
From:       Peter Korsgaard <peter () korsgaard ! com>
Date:       2019-01-28 22:25:06
Message-ID: 87munkbet9.fsf () dell ! be ! 48ers ! dk
[Download RAW message or body]

>>>>> "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
[prev in list] [next in list] [prev in thread] [next in thread]