1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
|
From 257f69d619fed407493156c8a7b952abc8a51314 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@...hat.com>
Date: Mon, 15 May 2017 15:57:28 +0100
Subject: [spice-server 1/3] reds: Disconnect when receiving overly big
ClientMonitorsConfig
Total message size received from the client was unlimited. There is
a 2kiB size check on individual agent messages, but the MonitorsConfig
message can be split in multiple chunks, and the size of the
non-chunked MonitorsConfig message was never checked. This could easily
lead to memory exhaustion on the host.
Signed-off-by: Frediano Ziglio <fziglio@...hat.com>
---
server/reds.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/server/reds.c b/server/reds.c
index f439a3668..7be85fdfc 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -993,19 +993,34 @@ static void reds_client_monitors_config_cleanup(void)
static void reds_on_main_agent_monitors_config(
MainChannelClient *mcc, void *message, size_t size)
{
+ const unsigned int MAX_MONITORS = 256;
+ const unsigned int MAX_MONITOR_CONFIG_SIZE =
+ sizeof(VDAgentMonitorsConfig) + MAX_MONITORS * sizeof(VDAgentMonConfig);
+
VDAgentMessage *msg_header;
VDAgentMonitorsConfig *monitors_config;
RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
+ // limit size of message sent by the client as this can cause a DoS through
+ // memory exhaustion, or potentially some integer overflows
+ if (sizeof(VDAgentMessage) + MAX_MONITOR_CONFIG_SIZE - cmc->buffer_size < size) {
+ goto overflow;
+ }
cmc->buffer_size += size;
cmc->buffer = realloc(cmc->buffer, cmc->buffer_size);
spice_assert(cmc->buffer);
cmc->mcc = mcc;
memcpy(cmc->buffer + cmc->buffer_pos, message, size);
cmc->buffer_pos += size;
+ if (sizeof(VDAgentMessage) > cmc->buffer_size) {
+ spice_debug("not enough data yet. %d", cmc->buffer_size);
+ return;
+ }
msg_header = (VDAgentMessage *)cmc->buffer;
- if (sizeof(VDAgentMessage) > cmc->buffer_size ||
- msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
+ if (msg_header->size > MAX_MONITOR_CONFIG_SIZE) {
+ goto overflow;
+ }
+ if (msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
spice_debug("not enough data yet. %d", cmc->buffer_size);
return;
}
@@ -1013,6 +1028,12 @@ static void reds_on_main_agent_monitors_config(
spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
red_dispatcher_client_monitors_config(monitors_config);
reds_client_monitors_config_cleanup();
+ return;
+
+overflow:
+ spice_warning("received invalid MonitorsConfig request from client, disconnecting");
+ red_channel_client_disconnect(main_channel_client_get_base(mcc));
+ reds_client_monitors_config_cleanup();
}
void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
--
2.13.0
From ff2b4ef70181087d5abd50bad76d026ec5088a93 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@...hat.com>
Date: Mon, 15 May 2017 15:57:28 +0100
Subject: [spice-server 2/3] reds: Avoid integer overflows handling monitor
configuration
Avoid VDAgentMessage::size integer overflows.
Signed-off-by: Frediano Ziglio <fziglio@...hat.com>
---
server/reds.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/server/reds.c b/server/reds.c
index 7be85fdfc..e1c8c1086 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1024,6 +1024,9 @@ static void reds_on_main_agent_monitors_config(
spice_debug("not enough data yet. %d", cmc->buffer_size);
return;
}
+ if (msg_header->size < sizeof(VDAgentMonitorsConfig)) {
+ goto overflow;
+ }
monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer + sizeof(*msg_header));
spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
red_dispatcher_client_monitors_config(monitors_config);
--
2.13.0
From 8cc3d7df2792751939cc832f4110c57e2addfca5 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@...hat.com>
Date: Mon, 15 May 2017 15:57:28 +0100
Subject: [spice-server 3/3] reds: Avoid buffer overflows handling monitor
configuration
It was also possible for a malicious client to set
VDAgentMonitorsConfig::num_of_monitors to a number larger
than the actual size of VDAgentMOnitorsConfig::monitors.
This would lead to buffer overflows, which could allow the guest to
read part of the host memory. This might cause write overflows in the
host as well, but controlling the content of such buffers seems
complicated.
Signed-off-by: Frediano Ziglio <fziglio@...hat.com>
---
server/reds.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/server/reds.c b/server/reds.c
index e1c8c1086..3a42c3755 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1000,6 +1000,7 @@ static void reds_on_main_agent_monitors_config(
VDAgentMessage *msg_header;
VDAgentMonitorsConfig *monitors_config;
RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
+ uint32_t max_monitors;
// limit size of message sent by the client as this can cause a DoS through
// memory exhaustion, or potentially some integer overflows
@@ -1028,6 +1029,12 @@ static void reds_on_main_agent_monitors_config(
goto overflow;
}
monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer + sizeof(*msg_header));
+ // limit the monitor number to avoid buffer overflows
+ max_monitors = (msg_header->size - sizeof(VDAgentMonitorsConfig)) /
+ sizeof(VDAgentMonConfig);
+ if (monitors_config->num_of_monitors > max_monitors) {
+ goto overflow;
+ }
spice_debug("%s: %d", __func__, monitors_config->num_of_monitors);
red_dispatcher_client_monitors_config(monitors_config);
reds_client_monitors_config_cleanup();
--
2.13.0
|
