mirror of
git://git.proxmox.com/git/pve-libspice-server.git
synced 2025-08-22 10:17:14 +00:00
update to 0.12.8
CVE-2015-3247 ,CVE-2015-5260 and CVE-2015-5261 are fixed upstream.
This commit is contained in:
parent
907c9f0e6e
commit
4df07ad3cf
14
Makefile
14
Makefile
@ -1,8 +1,8 @@
|
||||
RELEASE=4.0
|
||||
|
||||
PACKAGE=pve-libspice-server1
|
||||
PKGVERSION=0.12.5
|
||||
PKGRELEASE=2
|
||||
PKGVERSION=0.12.8
|
||||
PKGRELEASE=1
|
||||
|
||||
PKGDIR=spice-${PKGVERSION}
|
||||
PKGSRC=${PKGDIR}.tar.bz2
|
||||
@ -40,15 +40,7 @@ download:
|
||||
|
||||
.PHONY: upload
|
||||
upload: ${DEBS}
|
||||
umount /pve/${RELEASE}; mount /pve/${RELEASE} -o rw
|
||||
mkdir -p /pve/${RELEASE}/extra
|
||||
rm -f /pve/${RELEASE}/extra/Packages*
|
||||
rm -f /pve/${RELEASE}/extra/pve-libspice-server1_*.deb
|
||||
rm -f /pve/${RELEASE}/extra/pve-libspice-server-dev_*.deb
|
||||
rm -f /pve/${RELEASE}/extra/pve-libspice-server1_*.deb
|
||||
cp ${DEBS} /pve/${RELEASE}/extra
|
||||
cd /pve/${RELEASE}/extra; dpkg-scanpackages . /dev/null > Packages; gzip -9c Packages > Packages.gz
|
||||
umount /pve/${RELEASE}; mount /pve/${RELEASE} -o ro
|
||||
tar cf - ${DEBS}|ssh repoman@repo.proxmox.com upload
|
||||
|
||||
distclean: clean
|
||||
|
||||
|
6
debian/changelog
vendored
6
debian/changelog
vendored
@ -1,3 +1,9 @@
|
||||
pve-libspice-server (0.12.8-1) unstable; urgency=medium
|
||||
|
||||
* update to 0.12.8 (Fix CVE-2016-0749, CVE-2016-2150)
|
||||
|
||||
-- Proxmox Support Team <support@proxmox.com> Thu, 14 Jul 2016 06:35:24 +0200
|
||||
|
||||
pve-libspice-server (0.12.5-2) unstable; urgency=medium
|
||||
|
||||
* fix CVE-2015-3247, CVE-2015-5260, CVE-2015-5261
|
||||
|
115
debian/patches/CVE-2015-3247.patch
vendored
115
debian/patches/CVE-2015-3247.patch
vendored
@ -1,115 +0,0 @@
|
||||
From 524eef10c6c6c2f3f30be28c56b8f96adc7901f0 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 9 Jun 2015 08:50:46 +0100
|
||||
Subject: [PATCH] Avoid race conditions reading monitor configs from guest
|
||||
|
||||
For security reasons do not assume guest do not change structures it
|
||||
pass to Qemu.
|
||||
Guest could change count field while Qemu is copying QXLMonitorsConfig
|
||||
structure leading to heap corruption.
|
||||
This patch avoid it reading count only once.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_worker.c | 46 ++++++++++++++++++++++++++++++++--------------
|
||||
1 file changed, 32 insertions(+), 14 deletions(-)
|
||||
|
||||
--- a/server/red_worker.c
|
||||
+++ b/server/red_worker.c
|
||||
@@ -11051,7 +11051,8 @@ static inline void red_monitors_config_i
|
||||
}
|
||||
|
||||
static void worker_update_monitors_config(RedWorker *worker,
|
||||
- QXLMonitorsConfig *dev_monitors_config)
|
||||
+ QXLMonitorsConfig *dev_monitors_config,
|
||||
+ uint16_t count, uint16_t max_allowed)
|
||||
{
|
||||
int heads_size;
|
||||
MonitorsConfig *monitors_config;
|
||||
@@ -11060,22 +11061,22 @@ static void worker_update_monitors_confi
|
||||
monitors_config_decref(worker->monitors_config);
|
||||
|
||||
spice_debug("monitors config %d(%d)",
|
||||
- dev_monitors_config->count,
|
||||
- dev_monitors_config->max_allowed);
|
||||
- for (i = 0; i < dev_monitors_config->count; i++) {
|
||||
+ count,
|
||||
+ max_allowed);
|
||||
+ for (i = 0; i < count; i++) {
|
||||
spice_debug("+%d+%d %dx%d",
|
||||
dev_monitors_config->heads[i].x,
|
||||
dev_monitors_config->heads[i].y,
|
||||
dev_monitors_config->heads[i].width,
|
||||
dev_monitors_config->heads[i].height);
|
||||
}
|
||||
- heads_size = dev_monitors_config->count * sizeof(QXLHead);
|
||||
+ heads_size = count * sizeof(QXLHead);
|
||||
worker->monitors_config = monitors_config =
|
||||
spice_malloc(sizeof(*monitors_config) + heads_size);
|
||||
monitors_config->refs = 1;
|
||||
monitors_config->worker = worker;
|
||||
- monitors_config->count = dev_monitors_config->count;
|
||||
- monitors_config->max_allowed = dev_monitors_config->max_allowed;
|
||||
+ monitors_config->count = count;
|
||||
+ monitors_config->max_allowed = max_allowed;
|
||||
memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size);
|
||||
}
|
||||
|
||||
@@ -11459,33 +11460,50 @@ void handle_dev_display_migrate(void *op
|
||||
red_migrate_display(worker, rcc);
|
||||
}
|
||||
|
||||
+static inline uint32_t qxl_monitors_config_size(uint32_t heads)
|
||||
+{
|
||||
+ return sizeof(QXLMonitorsConfig) + sizeof(QXLHead) * heads;
|
||||
+}
|
||||
+
|
||||
static void handle_dev_monitors_config_async(void *opaque, void *payload)
|
||||
{
|
||||
RedWorkerMessageMonitorsConfigAsync *msg = payload;
|
||||
RedWorker *worker = opaque;
|
||||
- int min_size = sizeof(QXLMonitorsConfig) + sizeof(QXLHead);
|
||||
int error;
|
||||
+ uint16_t count, max_allowed;
|
||||
QXLMonitorsConfig *dev_monitors_config =
|
||||
(QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config,
|
||||
- min_size, msg->group_id, &error);
|
||||
+ qxl_monitors_config_size(1),
|
||||
+ msg->group_id, &error);
|
||||
|
||||
if (error) {
|
||||
/* TODO: raise guest bug (requires added QXL interface) */
|
||||
return;
|
||||
}
|
||||
worker->driver_cap_monitors_config = 1;
|
||||
- if (dev_monitors_config->count == 0) {
|
||||
+ count = dev_monitors_config->count;
|
||||
+ max_allowed = dev_monitors_config->max_allowed;
|
||||
+ if (count == 0) {
|
||||
spice_warning("ignoring an empty monitors config message from driver");
|
||||
return;
|
||||
}
|
||||
- if (dev_monitors_config->count > dev_monitors_config->max_allowed) {
|
||||
+ if (count > max_allowed) {
|
||||
spice_warning("ignoring malformed monitors_config from driver, "
|
||||
"count > max_allowed %d > %d",
|
||||
- dev_monitors_config->count,
|
||||
- dev_monitors_config->max_allowed);
|
||||
+ count,
|
||||
+ max_allowed);
|
||||
+ return;
|
||||
+ }
|
||||
+ /* get pointer again to check virtual size */
|
||||
+ dev_monitors_config =
|
||||
+ (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config,
|
||||
+ qxl_monitors_config_size(count),
|
||||
+ msg->group_id, &error);
|
||||
+ if (error) {
|
||||
+ /* TODO: raise guest bug (requires added QXL interface) */
|
||||
return;
|
||||
}
|
||||
- worker_update_monitors_config(worker, dev_monitors_config);
|
||||
+ worker_update_monitors_config(worker, dev_monitors_config, count, max_allowed);
|
||||
red_worker_push_monitors_config(worker);
|
||||
}
|
||||
|
@ -1,117 +0,0 @@
|
||||
From dd558bb833254fb49069eca052b92ae1abe3e8ff Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Wed, 9 Sep 2015 12:42:09 +0100
|
||||
Subject: [PATCH 01/19] worker: validate correctly surfaces
|
||||
|
||||
Do not just give warning and continue to use an invalid index into
|
||||
an array.
|
||||
|
||||
Resolves: CVE-2015-5260
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_worker.c | 33 ++++++++++++++++++---------------
|
||||
1 file changed, 18 insertions(+), 15 deletions(-)
|
||||
|
||||
--- a/server/red_worker.c
|
||||
+++ b/server/red_worker.c
|
||||
@@ -1036,6 +1036,7 @@ typedef struct BitmapData {
|
||||
SpiceRect lossy_rect;
|
||||
} BitmapData;
|
||||
|
||||
+static inline int validate_surface(RedWorker *worker, uint32_t surface_id);
|
||||
static void red_draw_qxl_drawable(RedWorker *worker, Drawable *drawable);
|
||||
static void red_current_flush(RedWorker *worker, int surface_id);
|
||||
#ifdef DRAW_ALL
|
||||
@@ -1245,14 +1246,12 @@ static inline int is_primary_surface(Red
|
||||
return FALSE;
|
||||
}
|
||||
|
||||
-static inline void __validate_surface(RedWorker *worker, uint32_t surface_id)
|
||||
-{
|
||||
- spice_warn_if(surface_id >= worker->n_surfaces);
|
||||
-}
|
||||
-
|
||||
static inline int validate_surface(RedWorker *worker, uint32_t surface_id)
|
||||
{
|
||||
- spice_warn_if(surface_id >= worker->n_surfaces);
|
||||
+ if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) {
|
||||
+ spice_warning("invalid surface_id %u", surface_id);
|
||||
+ return 0;
|
||||
+ }
|
||||
if (!worker->surfaces[surface_id].context.canvas) {
|
||||
spice_warning("canvas address is %p for %d (and is NULL)\n",
|
||||
&(worker->surfaces[surface_id].context.canvas), surface_id);
|
||||
@@ -4230,12 +4229,14 @@ static inline void red_create_surface(Re
|
||||
static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface,
|
||||
uint32_t group_id, int loadvm)
|
||||
{
|
||||
- int surface_id;
|
||||
+ uint32_t surface_id;
|
||||
RedSurface *red_surface;
|
||||
uint8_t *data;
|
||||
|
||||
surface_id = surface->surface_id;
|
||||
- __validate_surface(worker, surface_id);
|
||||
+ if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) {
|
||||
+ goto exit;
|
||||
+ }
|
||||
|
||||
red_surface = &worker->surfaces[surface_id];
|
||||
|
||||
@@ -4271,6 +4272,7 @@ static inline void red_process_surface(R
|
||||
default:
|
||||
spice_error("unknown surface command");
|
||||
};
|
||||
+exit:
|
||||
red_put_surface_cmd(surface);
|
||||
free(surface);
|
||||
}
|
||||
@@ -10865,7 +10867,7 @@ void handle_dev_update(void *opaque, voi
|
||||
{
|
||||
RedWorker *worker = opaque;
|
||||
RedWorkerMessageUpdate *msg = payload;
|
||||
- SpiceRect *rect = spice_new0(SpiceRect, 1);
|
||||
+ SpiceRect *rect;
|
||||
RedSurface *surface;
|
||||
uint32_t surface_id = msg->surface_id;
|
||||
const QXLRect *qxl_area = msg->qxl_area;
|
||||
@@ -10873,17 +10875,16 @@ void handle_dev_update(void *opaque, voi
|
||||
QXLRect *qxl_dirty_rects = msg->qxl_dirty_rects;
|
||||
uint32_t clear_dirty_region = msg->clear_dirty_region;
|
||||
|
||||
+ VALIDATE_SURFACE_RET(worker, surface_id);
|
||||
+
|
||||
+ rect = spice_new0(SpiceRect, 1);
|
||||
surface = &worker->surfaces[surface_id];
|
||||
red_get_rect_ptr(rect, qxl_area);
|
||||
flush_display_commands(worker);
|
||||
|
||||
spice_assert(worker->running);
|
||||
|
||||
- if (validate_surface(worker, surface_id)) {
|
||||
- red_update_area(worker, rect, surface_id);
|
||||
- } else {
|
||||
- rendering_incorrect(__func__);
|
||||
- }
|
||||
+ red_update_area(worker, rect, surface_id);
|
||||
free(rect);
|
||||
|
||||
surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects,
|
||||
@@ -10922,6 +10923,7 @@ void handle_dev_del_memslot(void *opaque
|
||||
* surface_id == 0, maybe move the assert upward and merge the two functions? */
|
||||
static inline void destroy_surface_wait(RedWorker *worker, int surface_id)
|
||||
{
|
||||
+ VALIDATE_SURFACE_RET(worker, surface_id);
|
||||
if (!worker->surfaces[surface_id].context.canvas) {
|
||||
return;
|
||||
}
|
||||
@@ -11186,6 +11188,7 @@ void handle_dev_create_primary_surface(v
|
||||
|
||||
static void dev_destroy_primary_surface(RedWorker *worker, uint32_t surface_id)
|
||||
{
|
||||
+ VALIDATE_SURFACE_RET(worker, surface_id);
|
||||
spice_warn_if(surface_id != 0);
|
||||
|
||||
spice_debug(NULL);
|
@ -1,41 +0,0 @@
|
||||
From 097c638b121e595d9daf79285c447088027a58e2 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Wed, 9 Sep 2015 12:45:06 +0100
|
||||
Subject: [PATCH 02/19] worker: avoid double free or double create of surfaces
|
||||
|
||||
A driver can overwrite surface state creating a surface with the same
|
||||
id of a previous one.
|
||||
Also can try to destroy surfaces that are not created.
|
||||
Both requests cause invalid internal states that could lead to crashes
|
||||
or memory corruptions.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_worker.c | 9 ++++++++-
|
||||
1 file changed, 8 insertions(+), 1 deletion(-)
|
||||
|
||||
--- a/server/red_worker.c
|
||||
+++ b/server/red_worker.c
|
||||
@@ -4246,6 +4246,10 @@ static inline void red_process_surface(R
|
||||
int32_t stride = surface->u.surface_create.stride;
|
||||
int reloaded_surface = loadvm || (surface->flags & QXL_SURF_FLAG_KEEP_DATA);
|
||||
|
||||
+ if (red_surface->refs) {
|
||||
+ spice_warning("avoiding creating a surface twice");
|
||||
+ break;
|
||||
+ }
|
||||
data = surface->u.surface_create.data;
|
||||
if (stride < 0) {
|
||||
data -= (int32_t)(stride * (height - 1));
|
||||
@@ -4259,7 +4263,10 @@ static inline void red_process_surface(R
|
||||
break;
|
||||
}
|
||||
case QXL_SURFACE_CMD_DESTROY:
|
||||
- spice_warn_if(!red_surface->context.canvas);
|
||||
+ if (!red_surface->refs) {
|
||||
+ spice_warning("avoiding destroying a surface twice");
|
||||
+ break;
|
||||
+ }
|
||||
set_surface_release_info(worker, surface_id, 0, surface->release_info, group_id);
|
||||
red_handle_depends_on_target_surface(worker, surface_id);
|
||||
/* note that red_handle_depends_on_target_surface must be called before red_current_clear.
|
@ -1,42 +0,0 @@
|
||||
From 0205a6ce63f50af9eda03f14d93b3a2517c42fae Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 11:58:11 +0100
|
||||
Subject: [PATCH 03/19] Define a constant to limit data from guest.
|
||||
|
||||
This limit will prevent guest trying to do nasty things and DoS to host.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 11 +++++++++++
|
||||
1 file changed, 11 insertions(+)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index 5b1befa..3ffa57b 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -21,11 +21,22 @@
|
||||
|
||||
#include <stdbool.h>
|
||||
#include <inttypes.h>
|
||||
+#include <glib.h>
|
||||
#include "common/lz_common.h"
|
||||
#include "red_common.h"
|
||||
#include "red_memslots.h"
|
||||
#include "red_parse_qxl.h"
|
||||
|
||||
+/* Max size in bytes for any data field used in a QXL command.
|
||||
+ * This will for example be useful to prevent the guest from saturating the
|
||||
+ * host memory if it tries to send overlapping chunks.
|
||||
+ * This value should be big enough for all requests but limited
|
||||
+ * to 32 bits. Even better if it fits on 31 bits to detect integer overflows.
|
||||
+ */
|
||||
+#define MAX_DATA_CHUNK 0x7ffffffflu
|
||||
+
|
||||
+G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32);
|
||||
+
|
||||
#if 0
|
||||
static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
|
||||
QXLPHYSICAL addr, uint8_t bytes)
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,64 +0,0 @@
|
||||
From ac5f64a80ae637742ed95fd6c98f66281b3e15c6 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Thu, 17 Sep 2015 15:00:22 +0100
|
||||
Subject: [PATCH 04/19] Fix some integer overflow causing large memory
|
||||
allocations
|
||||
|
||||
Prevent integer overflow when computing image sizes.
|
||||
Image index computations are done using 32 bit so this can cause easily
|
||||
security issues. MAX_DATA_CHUNK is larger than the virtual
|
||||
card limit, so this is not going to cause change in behaviours.
|
||||
Comparing size calculation results with MAX_DATA_CHUNK will allow us to
|
||||
catch overflows.
|
||||
Prevent guest from allocating large amount of memory.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 15 +++++++++++----
|
||||
1 file changed, 11 insertions(+), 4 deletions(-)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -384,7 +384,7 @@ static SpiceImage *red_get_image(RedMemS
|
||||
QXLImage *qxl;
|
||||
SpiceImage *red = NULL;
|
||||
SpicePalette *rp = NULL;
|
||||
- size_t bitmap_size, size;
|
||||
+ uint64_t bitmap_size, size;
|
||||
uint8_t qxl_flags;
|
||||
int error;
|
||||
|
||||
@@ -460,7 +460,10 @@ static SpiceImage *red_get_image(RedMemS
|
||||
red->u.bitmap.palette = rp;
|
||||
red->u.bitmap.palette_id = rp->unique;
|
||||
}
|
||||
- bitmap_size = red->u.bitmap.y * abs(red->u.bitmap.stride);
|
||||
+ bitmap_size = (uint64_t) red->u.bitmap.y * abs(red->u.bitmap.stride);
|
||||
+ if (bitmap_size > MAX_DATA_CHUNK) {
|
||||
+ goto error;
|
||||
+ }
|
||||
if (qxl_flags & QXL_BITMAP_DIRECT) {
|
||||
red->u.bitmap.data = red_get_image_data_flat(slots, group_id,
|
||||
qxl->bitmap.data,
|
||||
@@ -1221,7 +1224,7 @@ int red_get_surface_cmd(RedMemSlotInfo *
|
||||
RedSurfaceCmd *red, QXLPHYSICAL addr)
|
||||
{
|
||||
QXLSurfaceCmd *qxl;
|
||||
- size_t size;
|
||||
+ uint64_t size;
|
||||
int error;
|
||||
|
||||
qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id,
|
||||
@@ -1241,7 +1244,11 @@ int red_get_surface_cmd(RedMemSlotInfo *
|
||||
red->u.surface_create.width = qxl->u.surface_create.width;
|
||||
red->u.surface_create.height = qxl->u.surface_create.height;
|
||||
red->u.surface_create.stride = qxl->u.surface_create.stride;
|
||||
- size = red->u.surface_create.height * abs(red->u.surface_create.stride);
|
||||
+ /* the multiplication can overflow, also abs(-2^31) may return a negative value */
|
||||
+ size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride);
|
||||
+ if (size > MAX_DATA_CHUNK || red->u.surface_create.stride == G_MININT32) {
|
||||
+ return 1;
|
||||
+ }
|
||||
red->u.surface_create.data =
|
||||
(uint8_t*)get_virt(slots, qxl->u.surface_create.data, size, group_id, &error);
|
||||
if (error) {
|
@ -1,73 +0,0 @@
|
||||
From 1eb93baa3c594e1214b1c92bbad8a06e9c7e2d12 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 16:02:59 +0100
|
||||
Subject: [PATCH 05/19] Check properly surface to be created
|
||||
|
||||
Check format is valid.
|
||||
Check stride is at least the size of required bytes for a row.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 35 ++++++++++++++++++++++++++++++++++-
|
||||
1 file changed, 34 insertions(+), 1 deletion(-)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -1220,12 +1220,30 @@ void red_put_message(RedMessage *red)
|
||||
/* nothing yet */
|
||||
}
|
||||
|
||||
+static unsigned int surface_format_to_bpp(uint32_t format)
|
||||
+{
|
||||
+ switch (format) {
|
||||
+ case SPICE_SURFACE_FMT_1_A:
|
||||
+ return 1;
|
||||
+ case SPICE_SURFACE_FMT_8_A:
|
||||
+ return 8;
|
||||
+ case SPICE_SURFACE_FMT_16_555:
|
||||
+ case SPICE_SURFACE_FMT_16_565:
|
||||
+ return 16;
|
||||
+ case SPICE_SURFACE_FMT_32_xRGB:
|
||||
+ case SPICE_SURFACE_FMT_32_ARGB:
|
||||
+ return 32;
|
||||
+ }
|
||||
+ return 0;
|
||||
+}
|
||||
+
|
||||
int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
|
||||
RedSurfaceCmd *red, QXLPHYSICAL addr)
|
||||
{
|
||||
QXLSurfaceCmd *qxl;
|
||||
uint64_t size;
|
||||
int error;
|
||||
+ unsigned int bpp;
|
||||
|
||||
qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id,
|
||||
&error);
|
||||
@@ -1244,9 +1262,24 @@ int red_get_surface_cmd(RedMemSlotInfo *
|
||||
red->u.surface_create.width = qxl->u.surface_create.width;
|
||||
red->u.surface_create.height = qxl->u.surface_create.height;
|
||||
red->u.surface_create.stride = qxl->u.surface_create.stride;
|
||||
+ bpp = surface_format_to_bpp(red->u.surface_create.format);
|
||||
+
|
||||
+ /* check if format is valid */
|
||||
+ if (!bpp) {
|
||||
+ return 1;
|
||||
+ }
|
||||
+
|
||||
+ /* check stride is larger than required bytes */
|
||||
+ size = ((uint64_t) red->u.surface_create.width * bpp + 7u) / 8u;
|
||||
+ /* the uint32_t conversion is here to avoid problems with -2^31 value */
|
||||
+ if (red->u.surface_create.stride == G_MININT32
|
||||
+ || size > (uint32_t) abs(red->u.surface_create.stride)) {
|
||||
+ return 1;
|
||||
+ }
|
||||
+
|
||||
/* the multiplication can overflow, also abs(-2^31) may return a negative value */
|
||||
size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride);
|
||||
- if (size > MAX_DATA_CHUNK || red->u.surface_create.stride == G_MININT32) {
|
||||
+ if (size > MAX_DATA_CHUNK) {
|
||||
return 1;
|
||||
}
|
||||
red->u.surface_create.data =
|
@ -1,38 +0,0 @@
|
||||
From 68a742aaa8d692940ac15d021799b702412887e5 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 10:00:37 +0100
|
||||
Subject: [PATCH 06/19] Fix buffer reading overflow
|
||||
|
||||
Not security risk as just for read.
|
||||
However, this could be used to attempt integer overflows in the
|
||||
following lines.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 9 ++++++++-
|
||||
1 file changed, 8 insertions(+), 1 deletion(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index bdd5917..e2f95e4 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -361,7 +361,14 @@ static const int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = {0, 1, 1, 4, 4, 8, 16, 24,
|
||||
|
||||
static int bitmap_consistent(SpiceBitmap *bitmap)
|
||||
{
|
||||
- int bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format];
|
||||
+ int bpp;
|
||||
+
|
||||
+ if (bitmap->format >= SPICE_N_ELEMENTS(MAP_BITMAP_FMT_TO_BITS_PER_PIXEL)) {
|
||||
+ spice_warning("wrong format specified for image\n");
|
||||
+ return FALSE;
|
||||
+ }
|
||||
+
|
||||
+ bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format];
|
||||
|
||||
if (bitmap->stride < ((bitmap->x * bpp + 7) / 8)) {
|
||||
spice_warning("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n",
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,46 +0,0 @@
|
||||
From 0f58e9da56e0cbbe4349eefcbb300b6f285e0423 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 13:09:35 +0100
|
||||
Subject: [PATCH 07/19] Prevent 32 bit integer overflow in bitmap_consistent
|
||||
|
||||
The overflow may lead to buffer overflow as the row size computed from
|
||||
width (bitmap->x) can be bigger than the size in bytes (bitmap->stride).
|
||||
This can make spice-server accept the invalid sizes.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 7 ++++---
|
||||
1 file changed, 4 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index e2f95e4..40c1c99 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -357,11 +357,12 @@ static const char *bitmap_format_to_string(int format)
|
||||
return "unknown";
|
||||
}
|
||||
|
||||
-static const int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = {0, 1, 1, 4, 4, 8, 16, 24, 32, 32, 8};
|
||||
+static const unsigned int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] =
|
||||
+ {0, 1, 1, 4, 4, 8, 16, 24, 32, 32, 8};
|
||||
|
||||
static int bitmap_consistent(SpiceBitmap *bitmap)
|
||||
{
|
||||
- int bpp;
|
||||
+ unsigned int bpp;
|
||||
|
||||
if (bitmap->format >= SPICE_N_ELEMENTS(MAP_BITMAP_FMT_TO_BITS_PER_PIXEL)) {
|
||||
spice_warning("wrong format specified for image\n");
|
||||
@@ -370,7 +371,7 @@ static int bitmap_consistent(SpiceBitmap *bitmap)
|
||||
|
||||
bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format];
|
||||
|
||||
- if (bitmap->stride < ((bitmap->x * bpp + 7) / 8)) {
|
||||
+ if (bitmap->stride < (((uint64_t) bitmap->x * bpp + 7u) / 8u)) {
|
||||
spice_warning("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n",
|
||||
bitmap->stride, bitmap->x, bpp,
|
||||
bitmap_format_to_string(bitmap->format),
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,42 +0,0 @@
|
||||
From 3dfd1a08286d524a742d51952595fcfb6f0c6f1b Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 10:01:51 +0100
|
||||
Subject: [PATCH 08/19] Fix race condition on red_get_clip_rects
|
||||
|
||||
Do not read multiple time an array size that can be changed.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 8 +++++---
|
||||
1 file changed, 5 insertions(+), 3 deletions(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index 40c1c99..a9f3ca1 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -273,6 +273,7 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id,
|
||||
size_t size;
|
||||
int i;
|
||||
int error;
|
||||
+ uint32_t num_rects;
|
||||
|
||||
qxl = (QXLClipRects *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
|
||||
if (error) {
|
||||
@@ -284,9 +285,10 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id,
|
||||
data = red_linearize_chunk(&chunks, size, &free_data);
|
||||
red_put_data_chunks(&chunks);
|
||||
|
||||
- spice_assert(qxl->num_rects * sizeof(QXLRect) == size);
|
||||
- red = spice_malloc(sizeof(*red) + qxl->num_rects * sizeof(SpiceRect));
|
||||
- red->num_rects = qxl->num_rects;
|
||||
+ num_rects = qxl->num_rects;
|
||||
+ spice_assert(num_rects * sizeof(QXLRect) == size);
|
||||
+ red = spice_malloc(sizeof(*red) + num_rects * sizeof(SpiceRect));
|
||||
+ red->num_rects = num_rects;
|
||||
|
||||
start = (QXLRect*)data;
|
||||
for (i = 0; i < red->num_rects; i++) {
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,72 +0,0 @@
|
||||
From 9235c84e0fbbf5c19305e82fc1607393b35b74ef Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 10:04:10 +0100
|
||||
Subject: [PATCH 09/19] Fix race in red_get_image
|
||||
|
||||
Do not read multiple times data from guest as this could be changed
|
||||
by other vcpu threads.
|
||||
This causes races and security problems if these data are used for
|
||||
buffer allocation or checks.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 18 ++++++++++--------
|
||||
1 file changed, 10 insertions(+), 8 deletions(-)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -397,6 +397,7 @@ static SpiceImage *red_get_image(RedMemS
|
||||
uint64_t bitmap_size, size;
|
||||
uint8_t qxl_flags;
|
||||
int error;
|
||||
+ QXLPHYSICAL palette;
|
||||
|
||||
if (addr == 0) {
|
||||
return NULL;
|
||||
@@ -422,12 +423,16 @@ static SpiceImage *red_get_image(RedMemS
|
||||
switch (red->descriptor.type) {
|
||||
case SPICE_IMAGE_TYPE_BITMAP:
|
||||
red->u.bitmap.format = qxl->bitmap.format;
|
||||
- if (!bitmap_fmt_is_rgb(qxl->bitmap.format) && !qxl->bitmap.palette && !is_mask) {
|
||||
+ red->u.bitmap.x = qxl->bitmap.x;
|
||||
+ red->u.bitmap.y = qxl->bitmap.y;
|
||||
+ red->u.bitmap.stride = qxl->bitmap.stride;
|
||||
+ palette = qxl->bitmap.palette;
|
||||
+ if (!bitmap_fmt_is_rgb(red->u.bitmap.format) && !palette && !is_mask) {
|
||||
spice_warning("guest error: missing palette on bitmap format=%d\n",
|
||||
red->u.bitmap.format);
|
||||
goto error;
|
||||
}
|
||||
- if (qxl->bitmap.x == 0 || qxl->bitmap.y == 0) {
|
||||
+ if (red->u.bitmap.x == 0 || red->u.bitmap.y == 0) {
|
||||
spice_warning("guest error: zero area bitmap\n");
|
||||
goto error;
|
||||
}
|
||||
@@ -435,23 +440,20 @@ static SpiceImage *red_get_image(RedMemS
|
||||
if (qxl_flags & QXL_BITMAP_TOP_DOWN) {
|
||||
red->u.bitmap.flags = SPICE_BITMAP_FLAGS_TOP_DOWN;
|
||||
}
|
||||
- red->u.bitmap.x = qxl->bitmap.x;
|
||||
- red->u.bitmap.y = qxl->bitmap.y;
|
||||
- red->u.bitmap.stride = qxl->bitmap.stride;
|
||||
if (!bitmap_consistent(&red->u.bitmap)) {
|
||||
goto error;
|
||||
}
|
||||
- if (qxl->bitmap.palette) {
|
||||
+ if (palette) {
|
||||
QXLPalette *qp;
|
||||
int i, num_ents;
|
||||
- qp = (QXLPalette *)get_virt(slots, qxl->bitmap.palette,
|
||||
+ qp = (QXLPalette *)get_virt(slots, palette,
|
||||
sizeof(*qp), group_id, &error);
|
||||
if (error) {
|
||||
goto error;
|
||||
}
|
||||
num_ents = qp->num_ents;
|
||||
if (!validate_virt(slots, (intptr_t)qp->ents,
|
||||
- get_memslot_id(slots, qxl->bitmap.palette),
|
||||
+ get_memslot_id(slots, palette),
|
||||
num_ents * sizeof(qp->ents[0]), group_id)) {
|
||||
goto error;
|
||||
}
|
@ -1,57 +0,0 @@
|
||||
From dfaedec7890069b35f513e4a8ab4071ca54259ff Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 10:05:20 +0100
|
||||
Subject: [PATCH 10/19] Fix race condition in red_get_string
|
||||
|
||||
Do not read multiple time an array size that can be changed.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 15 +++++++++------
|
||||
1 file changed, 9 insertions(+), 6 deletions(-)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -810,6 +810,7 @@ static SpiceString *red_get_string(RedMe
|
||||
size_t chunk_size, qxl_size, red_size, glyph_size;
|
||||
int glyphs, bpp = 0, i;
|
||||
int error;
|
||||
+ uint16_t qxl_flags, qxl_length;
|
||||
|
||||
qxl = (QXLString *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
|
||||
if (error) {
|
||||
@@ -826,13 +827,15 @@ static SpiceString *red_get_string(RedMe
|
||||
red_put_data_chunks(&chunks);
|
||||
|
||||
qxl_size = qxl->data_size;
|
||||
+ qxl_flags = qxl->flags;
|
||||
+ qxl_length = qxl->length;
|
||||
spice_assert(chunk_size == qxl_size);
|
||||
|
||||
- if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A1) {
|
||||
+ if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A1) {
|
||||
bpp = 1;
|
||||
- } else if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A4) {
|
||||
+ } else if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A4) {
|
||||
bpp = 4;
|
||||
- } else if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A8) {
|
||||
+ } else if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A8) {
|
||||
bpp = 8;
|
||||
}
|
||||
spice_assert(bpp != 0);
|
||||
@@ -849,11 +852,11 @@ static SpiceString *red_get_string(RedMe
|
||||
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
|
||||
}
|
||||
spice_assert(start <= end);
|
||||
- spice_assert(glyphs == qxl->length);
|
||||
+ spice_assert(glyphs == qxl_length);
|
||||
|
||||
red = spice_malloc(red_size);
|
||||
- red->length = qxl->length;
|
||||
- red->flags = qxl->flags;
|
||||
+ red->length = qxl_length;
|
||||
+ red->flags = qxl_flags;
|
||||
|
||||
start = (QXLRasterGlyph*)data;
|
||||
end = (QXLRasterGlyph*)(data + chunk_size);
|
@ -1,58 +0,0 @@
|
||||
From caec52dc77af6ebdac3219a1b10fe2293af21208 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 10:13:24 +0100
|
||||
Subject: [PATCH 11/19] Fix integer overflow computing glyph_size in
|
||||
red_get_string
|
||||
|
||||
If bpp is int the formula can lead to weird overflows. width and height
|
||||
are uint16_t so the formula is:
|
||||
|
||||
size_t = u16 * (u16 * int + const_int) / const_int;
|
||||
|
||||
so it became
|
||||
|
||||
size_t = (int) u16 * ((int) u16 * int + const_int) / const_int;
|
||||
|
||||
However the (int) u16 * (int) u16 can then became negative to overflow.
|
||||
Under 64 bit architectures size_t is 64 and int usually 32 so converting
|
||||
this negative 32 bit number to a unsigned 64 bit lead to a very big
|
||||
number as the signed is extended and then converted to unsigned.
|
||||
Using unsigned arithmetic prevent extending the sign.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 8 +++++---
|
||||
1 file changed, 5 insertions(+), 3 deletions(-)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -808,7 +808,9 @@ static SpiceString *red_get_string(RedMe
|
||||
uint8_t *data;
|
||||
bool free_data;
|
||||
size_t chunk_size, qxl_size, red_size, glyph_size;
|
||||
- int glyphs, bpp = 0, i;
|
||||
+ int glyphs, i;
|
||||
+ /* use unsigned to prevent integer overflow in multiplication below */
|
||||
+ unsigned int bpp = 0;
|
||||
int error;
|
||||
uint16_t qxl_flags, qxl_length;
|
||||
|
||||
@@ -847,7 +849,7 @@ static SpiceString *red_get_string(RedMe
|
||||
while (start < end) {
|
||||
spice_assert((QXLRasterGlyph*)(&start->data[0]) <= end);
|
||||
glyphs++;
|
||||
- glyph_size = start->height * ((start->width * bpp + 7) / 8);
|
||||
+ glyph_size = start->height * ((start->width * bpp + 7u) / 8u);
|
||||
red_size += sizeof(SpiceRasterGlyph *) + SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4);
|
||||
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
|
||||
}
|
||||
@@ -868,7 +870,7 @@ static SpiceString *red_get_string(RedMe
|
||||
glyph->height = start->height;
|
||||
red_get_point_ptr(&glyph->render_pos, &start->render_pos);
|
||||
red_get_point_ptr(&glyph->glyph_origin, &start->glyph_origin);
|
||||
- glyph_size = glyph->height * ((glyph->width * bpp + 7) / 8);
|
||||
+ glyph_size = glyph->height * ((glyph->width * bpp + 7u) / 8u);
|
||||
spice_assert((QXLRasterGlyph*)(&start->data[glyph_size]) <= end);
|
||||
memcpy(glyph->data, start->data, glyph_size);
|
||||
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
|
@ -1,66 +0,0 @@
|
||||
From 3738478ed7065fe05f3ee4848f8a7fcdf40aa920 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 12:12:19 +0100
|
||||
Subject: [PATCH 12/19] Fix race condition in red_get_data_chunks_ptr
|
||||
|
||||
Do not read multiple times data from guest as this can be changed by
|
||||
other guest vcpus. This causes races and security problems if these
|
||||
data are used for buffer allocation or checks.
|
||||
|
||||
Actually, the 'data' member can't change during read as it is just a
|
||||
pointer to a fixed array contained in qxl. However, this change will
|
||||
make it clear that there can be no race condition.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 17 ++++++++++-------
|
||||
1 file changed, 10 insertions(+), 7 deletions(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index cfa21f9..2863ae2 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -102,30 +102,33 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
|
||||
RedDataChunk *red_prev;
|
||||
size_t data_size = 0;
|
||||
int error;
|
||||
+ QXLPHYSICAL next_chunk;
|
||||
|
||||
red->data_size = qxl->data_size;
|
||||
data_size += red->data_size;
|
||||
- if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) {
|
||||
+ red->data = qxl->data;
|
||||
+ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
|
||||
+ red->data = NULL;
|
||||
return 0;
|
||||
}
|
||||
- red->data = qxl->data;
|
||||
red->prev_chunk = NULL;
|
||||
|
||||
- while (qxl->next_chunk) {
|
||||
+ while ((next_chunk = qxl->next_chunk) != 0) {
|
||||
red_prev = red;
|
||||
red = spice_new(RedDataChunk, 1);
|
||||
- memslot_id = get_memslot_id(slots, qxl->next_chunk);
|
||||
- qxl = (QXLDataChunk *)get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id,
|
||||
+ memslot_id = get_memslot_id(slots, next_chunk);
|
||||
+ qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
|
||||
&error);
|
||||
if (error) {
|
||||
return 0;
|
||||
}
|
||||
red->data_size = qxl->data_size;
|
||||
data_size += red->data_size;
|
||||
- if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) {
|
||||
+ red->data = qxl->data;
|
||||
+ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
|
||||
+ red->data = NULL;
|
||||
return 0;
|
||||
}
|
||||
- red->data = qxl->data;
|
||||
red->prev_chunk = red_prev;
|
||||
red_prev->next_chunk = red;
|
||||
}
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,75 +0,0 @@
|
||||
From f3605979ce3b33d60c33b59334b53618e6d8662a Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 12:14:55 +0100
|
||||
Subject: [PATCH 13/19] Prevent memory leak if red_get_data_chunks_ptr fails
|
||||
|
||||
Free linked list if client tries to do nasty things
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 31 ++++++++++++++++++++-----------
|
||||
1 file changed, 20 insertions(+), 11 deletions(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index 2863ae2..f425869 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -107,34 +107,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
|
||||
red->data_size = qxl->data_size;
|
||||
data_size += red->data_size;
|
||||
red->data = qxl->data;
|
||||
+ red->prev_chunk = red->next_chunk = NULL;
|
||||
if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
|
||||
red->data = NULL;
|
||||
return 0;
|
||||
}
|
||||
- red->prev_chunk = NULL;
|
||||
|
||||
while ((next_chunk = qxl->next_chunk) != 0) {
|
||||
red_prev = red;
|
||||
- red = spice_new(RedDataChunk, 1);
|
||||
+ red = spice_new0(RedDataChunk, 1);
|
||||
+ red->prev_chunk = red_prev;
|
||||
+ red_prev->next_chunk = red;
|
||||
+
|
||||
memslot_id = get_memslot_id(slots, next_chunk);
|
||||
qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
|
||||
&error);
|
||||
- if (error) {
|
||||
- return 0;
|
||||
- }
|
||||
+ if (error)
|
||||
+ goto error;
|
||||
red->data_size = qxl->data_size;
|
||||
data_size += red->data_size;
|
||||
red->data = qxl->data;
|
||||
- if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
|
||||
- red->data = NULL;
|
||||
- return 0;
|
||||
- }
|
||||
- red->prev_chunk = red_prev;
|
||||
- red_prev->next_chunk = red;
|
||||
+ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id))
|
||||
+ goto error;
|
||||
}
|
||||
|
||||
red->next_chunk = NULL;
|
||||
return data_size;
|
||||
+
|
||||
+error:
|
||||
+ while (red->prev_chunk) {
|
||||
+ red_prev = red->prev_chunk;
|
||||
+ free(red);
|
||||
+ red = red_prev;
|
||||
+ }
|
||||
+ red->data_size = 0;
|
||||
+ red->next_chunk = NULL;
|
||||
+ red->data = NULL;
|
||||
+ return 0;
|
||||
}
|
||||
|
||||
static size_t red_get_data_chunks(RedMemSlotInfo *slots, int group_id,
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,102 +0,0 @@
|
||||
From 7d69184037d0abb4fcfd5625c765b822aa458808 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 12:28:54 +0100
|
||||
Subject: [PATCH 14/19] Prevent DoS from guest trying to allocate too much data
|
||||
on host for chunks
|
||||
|
||||
Limit number of chunks to a given amount to avoid guest trying to
|
||||
allocate too much memory. Using circular or nested chunks lists
|
||||
guest could try to allocate huge amounts of memory.
|
||||
Considering the list can be infinite and guest can change data this
|
||||
also prevents strange security attacks from guest.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
|
||||
1 file changed, 41 insertions(+), 8 deletions(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index f425869..5513e82 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -37,6 +37,13 @@
|
||||
|
||||
G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32);
|
||||
|
||||
+/* Limit number of chunks.
|
||||
+ * The guest can attempt to make host allocate too much memory
|
||||
+ * just with a large number of small chunks.
|
||||
+ * Prevent that the chunk list take more memory than the data itself.
|
||||
+ */
|
||||
+#define MAX_CHUNKS (MAX_DATA_CHUNK/1024u)
|
||||
+
|
||||
#if 0
|
||||
static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
|
||||
QXLPHYSICAL addr, uint8_t bytes)
|
||||
@@ -100,9 +107,11 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
|
||||
RedDataChunk *red, QXLDataChunk *qxl)
|
||||
{
|
||||
RedDataChunk *red_prev;
|
||||
- size_t data_size = 0;
|
||||
+ uint64_t data_size = 0;
|
||||
+ uint32_t chunk_data_size;
|
||||
int error;
|
||||
QXLPHYSICAL next_chunk;
|
||||
+ unsigned num_chunks = 0;
|
||||
|
||||
red->data_size = qxl->data_size;
|
||||
data_size += red->data_size;
|
||||
@@ -114,19 +123,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
|
||||
}
|
||||
|
||||
while ((next_chunk = qxl->next_chunk) != 0) {
|
||||
+ /* somebody is trying to use too much memory using a lot of chunks.
|
||||
+ * Or made a circular list of chunks
|
||||
+ */
|
||||
+ if (++num_chunks >= MAX_CHUNKS) {
|
||||
+ spice_warning("data split in too many chunks, avoiding DoS\n");
|
||||
+ goto error;
|
||||
+ }
|
||||
+
|
||||
+ memslot_id = get_memslot_id(slots, next_chunk);
|
||||
+ qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl),
|
||||
+ group_id, &error);
|
||||
+ if (error)
|
||||
+ goto error;
|
||||
+
|
||||
+ /* do not waste space for empty chunks.
|
||||
+ * This could be just a driver issue or an attempt
|
||||
+ * to allocate too much memory or a circular list.
|
||||
+ * All above cases are handled by the check for number
|
||||
+ * of chunks.
|
||||
+ */
|
||||
+ chunk_data_size = qxl->data_size;
|
||||
+ if (chunk_data_size == 0)
|
||||
+ continue;
|
||||
+
|
||||
red_prev = red;
|
||||
red = spice_new0(RedDataChunk, 1);
|
||||
+ red->data_size = chunk_data_size;
|
||||
red->prev_chunk = red_prev;
|
||||
+ red->data = qxl->data;
|
||||
red_prev->next_chunk = red;
|
||||
|
||||
- memslot_id = get_memslot_id(slots, next_chunk);
|
||||
- qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
|
||||
- &error);
|
||||
- if (error)
|
||||
+ data_size += chunk_data_size;
|
||||
+ /* this can happen if client is sending nested chunks */
|
||||
+ if (data_size > MAX_DATA_CHUNK) {
|
||||
+ spice_warning("too much data inside chunks, avoiding DoS\n");
|
||||
goto error;
|
||||
- red->data_size = qxl->data_size;
|
||||
- data_size += red->data_size;
|
||||
- red->data = qxl->data;
|
||||
+ }
|
||||
if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id))
|
||||
goto error;
|
||||
}
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,36 +0,0 @@
|
||||
From a447c4f2ac19a1fa36330ffc90ee70b953b82050 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 8 Sep 2015 13:06:03 +0100
|
||||
Subject: [PATCH 15/19] Fix some possible overflows in red_get_string for 32
|
||||
bit
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 8 +++++++-
|
||||
1 file changed, 7 insertions(+), 1 deletion(-)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -896,6 +896,11 @@ static SpiceString *red_get_string(RedMe
|
||||
glyphs++;
|
||||
glyph_size = start->height * ((start->width * bpp + 7u) / 8u);
|
||||
red_size += sizeof(SpiceRasterGlyph *) + SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4);
|
||||
+ /* do the test correctly, we know end - start->data[0] cannot
|
||||
+ * overflow, don't use start->data[glyph_size] to test for
|
||||
+ * buffer overflow as this on 32 bit can cause overflow
|
||||
+ * on the pointer arithmetic */
|
||||
+ spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]);
|
||||
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
|
||||
}
|
||||
spice_assert(start <= end);
|
||||
@@ -916,7 +921,8 @@ static SpiceString *red_get_string(RedMe
|
||||
red_get_point_ptr(&glyph->render_pos, &start->render_pos);
|
||||
red_get_point_ptr(&glyph->glyph_origin, &start->glyph_origin);
|
||||
glyph_size = glyph->height * ((glyph->width * bpp + 7u) / 8u);
|
||||
- spice_assert((QXLRasterGlyph*)(&start->data[glyph_size]) <= end);
|
||||
+ /* see above for similar test */
|
||||
+ spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]);
|
||||
memcpy(glyph->data, start->data, glyph_size);
|
||||
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
|
||||
glyph = (SpiceRasterGlyph*)
|
@ -1,40 +0,0 @@
|
||||
From 2693e0497e5626642250cff47a59b3b4b2cd432d Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 15 Sep 2015 16:25:17 +0100
|
||||
Subject: [PATCH 16/19] Make sure we can read QXLPathSeg structures
|
||||
|
||||
start pointer points to a QXLPathSeg structure.
|
||||
Before reading from the structure, make sure the structure is contained
|
||||
in the memory range checked.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 4 ++--
|
||||
1 file changed, 2 insertions(+), 2 deletions(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index f21bfa5..281faad 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -256,7 +256,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
|
||||
|
||||
start = (QXLPathSeg*)data;
|
||||
end = (QXLPathSeg*)(data + size);
|
||||
- while (start < end) {
|
||||
+ while (start+1 < end) {
|
||||
n_segments++;
|
||||
count = start->count;
|
||||
segment_size = sizeof(SpicePathSeg) + count * sizeof(SpicePointFix);
|
||||
@@ -272,7 +272,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
|
||||
seg = (SpicePathSeg*)&red->segments[n_segments];
|
||||
n_segments = 0;
|
||||
mem_size2 = sizeof(*red);
|
||||
- while (start < end) {
|
||||
+ while (start+1 < end) {
|
||||
red->segments[n_segments++] = seg;
|
||||
count = start->count;
|
||||
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,31 +0,0 @@
|
||||
From 2b6695f1222f68690ea230e4e37ded7e07188f06 Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Tue, 15 Sep 2015 16:38:23 +0100
|
||||
Subject: [PATCH 17/19] Avoid race condition copying segments in red_get_path
|
||||
|
||||
The guest can attempt to increase the number of segments while
|
||||
spice-server is reading them.
|
||||
Make sure we don't copy more then the allocated segments.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 2 +-
|
||||
1 file changed, 1 insertion(+), 1 deletion(-)
|
||||
|
||||
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
|
||||
index 281faad..c7f8650 100644
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -272,7 +272,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
|
||||
seg = (SpicePathSeg*)&red->segments[n_segments];
|
||||
n_segments = 0;
|
||||
mem_size2 = sizeof(*red);
|
||||
- while (start+1 < end) {
|
||||
+ while (start+1 < end && n_segments < red->num_segments) {
|
||||
red->segments[n_segments++] = seg;
|
||||
count = start->count;
|
||||
|
||||
--
|
||||
2.6.1
|
||||
|
@ -1,24 +0,0 @@
|
||||
From b3be589ab3b32af3e470a9dec19a61fb086f72fc Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Thu, 17 Sep 2015 14:28:36 +0100
|
||||
Subject: [PATCH 18/19] Prevent data_size to be set independently from data
|
||||
|
||||
There was not check for data_size field so one could set data to
|
||||
a small set of data and data_size much bigger than size of data
|
||||
leading to buffer overflow.
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 1 +
|
||||
1 file changed, 1 insertion(+)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -1392,6 +1392,7 @@ static int red_get_cursor(RedMemSlotInfo
|
||||
size = red_get_data_chunks_ptr(slots, group_id,
|
||||
get_memslot_id(slots, addr),
|
||||
&chunks, &qxl->chunk);
|
||||
+ red->data_size = MIN(red->data_size, size);
|
||||
data = red_linearize_chunk(&chunks, size, &free_data);
|
||||
red_put_data_chunks(&chunks);
|
||||
if (free_data) {
|
@ -1,29 +0,0 @@
|
||||
From 6e3547f8b192f5b01d478ca222bf46736f5c700c Mon Sep 17 00:00:00 2001
|
||||
From: Frediano Ziglio <fziglio@redhat.com>
|
||||
Date: Thu, 17 Sep 2015 15:01:05 +0100
|
||||
Subject: [PATCH 19/19] Prevent leak if size from red_get_data_chunks don't
|
||||
match in red_get_image
|
||||
|
||||
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
|
||||
---
|
||||
server/red_parse_qxl.c | 2 ++
|
||||
1 file changed, 2 insertions(+)
|
||||
|
||||
--- a/server/red_parse_qxl.c
|
||||
+++ b/server/red_parse_qxl.c
|
||||
@@ -530,6 +530,7 @@ static SpiceImage *red_get_image(RedMemS
|
||||
&chunks, qxl->bitmap.data);
|
||||
spice_assert(size == bitmap_size);
|
||||
if (size != bitmap_size) {
|
||||
+ red_put_data_chunks(&chunks);
|
||||
goto error;
|
||||
}
|
||||
red->u.bitmap.data = red_get_image_data_chunked(slots, group_id,
|
||||
@@ -550,6 +551,7 @@ static SpiceImage *red_get_image(RedMemS
|
||||
&chunks, (QXLDataChunk *)qxl->quic.data);
|
||||
spice_assert(size == red->u.quic.data_size);
|
||||
if (size != red->u.quic.data_size) {
|
||||
+ red_put_data_chunks(&chunks);
|
||||
goto error;
|
||||
}
|
||||
red->u.quic.data = red_get_image_data_chunked(slots, group_id,
|
10
debian/patches/allow-to-set-sasl-callbacks.patch
vendored
10
debian/patches/allow-to-set-sasl-callbacks.patch
vendored
@ -10,11 +10,11 @@ Index: new/server/spice-server.syms
|
||||
spice_server_get_best_playback_rate;
|
||||
spice_server_set_playback_rate;
|
||||
spice_server_get_best_record_rate;
|
||||
Index: new/server/spice.h
|
||||
Index: new/server/spice-server.h
|
||||
===================================================================
|
||||
--- new.orig/server/spice.h
|
||||
+++ new/server/spice.h
|
||||
@@ -457,6 +457,7 @@ int spice_server_set_exit_on_disconnect(
|
||||
--- new.orig/server/spice-server.h
|
||||
+++ new/server/spice-server.h
|
||||
@@ -53,6 +53,7 @@ int spice_server_set_exit_on_disconnect(
|
||||
int spice_server_set_noauth(SpiceServer *s);
|
||||
int spice_server_set_sasl(SpiceServer *s, int enabled);
|
||||
int spice_server_set_sasl_appname(SpiceServer *s, const char *appname);
|
||||
@ -45,7 +45,7 @@ Index: new/server/reds_stream.c
|
||||
typedef struct RedsSASL {
|
||||
sasl_conn_t *conn;
|
||||
|
||||
@@ -966,7 +978,7 @@ bool reds_sasl_start_auth(RedsStream *st
|
||||
@@ -976,7 +988,7 @@ bool reds_sasl_start_auth(RedsStream *st
|
||||
NULL, /* User realm */
|
||||
localAddr,
|
||||
remoteAddr,
|
||||
|
20
debian/patches/series
vendored
20
debian/patches/series
vendored
@ -1,21 +1 @@
|
||||
allow-to-set-sasl-callbacks.patch
|
||||
CVE-2015-3247.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0001-worker-validate-correctly-surfaces.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0002-worker-avoid-double-free-or-double-create-of-surface.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0003-Define-a-constant-to-limit-data-from-guest.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0004-Fix-some-integer-overflow-causing-large-memory-alloc.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0005-Check-properly-surface-to-be-created.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0006-Fix-buffer-reading-overflow.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0007-Prevent-32-bit-integer-overflow-in-bitmap_consistent.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0008-Fix-race-condition-on-red_get_clip_rects.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0009-Fix-race-in-red_get_image.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0010-Fix-race-condition-in-red_get_string.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0011-Fix-integer-overflow-computing-glyph_size-in-red_get.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0012-Fix-race-condition-in-red_get_data_chunks_ptr.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0013-Prevent-memory-leak-if-red_get_data_chunks_ptr-fails.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0014-Prevent-DoS-from-guest-trying-to-allocate-too-much-d.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0015-Fix-some-possible-overflows-in-red_get_string-for-32.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0016-Make-sure-we-can-read-QXLPathSeg-structures.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0017-Avoid-race-condition-copying-segments-in-red_get_pat.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0018-Prevent-data_size-to-be-set-independently-from-data.patch
|
||||
CVE-2015-5260_CVE-2015-5261/0019-Prevent-leak-if-size-from-red_get_data_chunks-don-t-.patch
|
||||
|
Binary file not shown.
BIN
spice-0.12.8.tar.bz2
Normal file
BIN
spice-0.12.8.tar.bz2
Normal file
Binary file not shown.
Loading…
x
Reference in New Issue
Block a user