[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Thu, 22 Feb 2018 19:29:26 +0100
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Cc: Dietmar Maurer <dietmar@...xmox.com>
Subject: review of LibVNCServer/vncterm proxmox/vncterm proxmox/spiceterm xenserver/vncterm qemu/ui/console.c
Hi,
Well, this is not a proper review. Rather, I just took a quick look at
more of these today.
Turns out there are at least 3 (sub-)projects named vncterm, and
apparently they aren't even forks of each other: there's a vncterm that
used to be part of LibVNCServer and is now maintained in a nearby repo,
another vncterm in xenserver derived from QEMU's ui/console.c, and yet
another one in proxmox. There's also spiceterm in proxmox, which
duplicates code from their vncterm (or vice versa).
I already pointed out one issue with LibVNCServer's vncterm in here a
few days ago:
http://www.openwall.com/lists/oss-security/2018/02/18/2
and I also opened a GitHub issue with them suggesting that they
"Document that the code is unsafe for future extension":
https://github.com/LibVNC/vncterm/issues/7
To me, the code only appears to dodge other issues because of its
presently limited functionality - more limited than that of the
alternatives that the rest of this message is about.
proxmox/vncterm and proxmox/spiceterm look the worst to me. One issue
is that vncterm_putchar() case ESgetpars appears to be willing to
increase vt->esc_count indefinitely, and that's a signed int variable,
which (despite of this being UB in C) may eventually (e.g., after two
gigabytes of semicolons?) wraparound to negative and then pass one of
the many "vt->esc_count < MAX_ESC_PARAMS" checks present throughout the
code, resulting in an out-of-bounds write or/and read relative to the
vt->esc_buf[] array:
case ESgetpars:
if (ch >= '0' && ch <= '9') {
vt->esc_has_par = 1;
if (vt->esc_count < MAX_ESC_PARAMS) {
vt->esc_buf[vt->esc_count] = vt->esc_buf[vt->esc_count] * 10 + ch - '0';
}
break;
} else if (ch == ';') {
vt->esc_count++;
break;
} else {
if (vt->esc_has_par) {
vt->esc_count++;
}
vt->tty_state = ESgotpars;
}
Notice that the "vt->esc_count < MAX_ESC_PARAMS" check here does not
prevent the two instances of "vt->esc_count++;" from being reached.
The relevant excerpts from vncterm.h are:
#define MAX_ESC_PARAMS 16
int esc_buf[MAX_ESC_PARAMS];
int esc_count;
If you'd like to track this unconfirmed issue (I only skimmed the code -
I didn't try to trigger the issue), please use OVE-20180222-0001.
Also seen above is the lack of sanity limits on the values that
vt->esc_buf[] elements can get. (The math above can also overflow,
formally speaking triggering UB.) A consequence of this is that they
can become negative, too. Then some cursor movement commands assume
that the values are non-negative nor can overflow the math, e.g.:
case 'A':
/* move cursor up */
if (vt->esc_buf[0] == 0) {
vt->esc_buf[0] = 1;
}
vt->cy -= vt->esc_buf[0];
if (vt->cy < 0) {
vt->cy = 0;
}
break;
case 'B':
case 'e':
/* move cursor down */
if (vt->esc_buf[0] == 0) {
vt->esc_buf[0] = 1;
}
vt->cy += vt->esc_buf[0];
if (vt->cy >= vt->height) {
vt->cy = vt->height - 1;
}
break;
case 'C':
case 'a':
/* move cursor right */
if (vt->esc_buf[0] == 0) {
vt->esc_buf[0] = 1;
}
vt->cx += vt->esc_buf[0];
if (vt->cx >= vt->width) {
vt->cx = vt->width - 1;
}
break;
case 'D':
/* move cursor left */
if (vt->esc_buf[0] == 0) {
vt->esc_buf[0] = 1;
}
vt->cx -= vt->esc_buf[0];
if (vt->cx < 0) {
vt->cx = 0;
}
break;
Notice how these only perform sanity checks in the expected direction of
the cursor movement, but not in the opposite direction (which can occur
with a negative value in vt->esc_buf[0], or with a value large enough to
cause integer wraparound right here). This unconfirmed issue is
OVE-20180222-0002.
There might or might not be more problematic commands like this.
However, some look OK, e.g.:
case 'G':
case '`':
/* move cursor to column */
vncterm_gotoxy (vt, vt->esc_buf[0] - 1, vt->cy);
break;
case 'd':
/* move cursor to row */
vncterm_gotoxy (vt, vt->cx , vt->esc_buf[0] - 1);
break;
case 'f':
case 'H':
/* move cursor to row, column */
vncterm_gotoxy (vt, vt->esc_buf[1] - 1, vt->esc_buf[0] - 1);
break;
where vncterm_gotoxy() performs all the necessary sanity checks.
There are also many places that are difficult to review. At first, this
looked like it'd overflow vt->esc_buf[] over multiple invocations:
case ESpalette:
if ((ch >= '0' && ch <= '9') || (ch >= 'A' && ch <= 'F')
|| (ch >= 'a' && ch <= 'f')) {
vt->esc_buf[vt->esc_count++] = (ch > '9' ? (ch & 0xDF) - 'A' + 10 : ch - '0');
if (vt->esc_count == 7) {
// fixme: this does not work - please test
rfbColourMap *cmap =&vt->screen->colourMap;
int i = color_table[vt->esc_buf[0]] * 3, j = 1;
cmap->data.bytes[i] = 16 * vt->esc_buf[j++];
cmap->data.bytes[i++] += vt->esc_buf[j++];
cmap->data.bytes[i] = 16 * vt->esc_buf[j++];
cmap->data.bytes[i++] += vt->esc_buf[j++];
cmap->data.bytes[i] = 16 * vt->esc_buf[j++];
cmap->data.bytes[i] += vt->esc_buf[j];
//set_palette(vc); ?
vt->tty_state = ESnormal;
}
} else
vt->tty_state = ESnormal;
break;
However, upon a closer look "vt->tty_state = ESnormal;" prevents this
code from being reached again without having vt->esc_count reset to 0 as
the only place setting vt->tty_state to ESpalette is:
case ESnonstd: /* Operating System Controls */
vt->tty_state = ESnormal;
switch (ch) {
case 'P': /* palette escape sequence */
for(i = 0; i < MAX_ESC_PARAMS; i++) {
vt->esc_buf[i] = 0;
}
vt->esc_count = 0;
vt->tty_state = ESpalette;
break;
so in the end this looks OK to me. It's just I would have preferred an
explicit range check or resetting of vt->esc_count right in or after the
ESpalette code.
Perhaps there are more issues than those I noticed. The numbered issues
above are both also present in proxmox/spiceterm/spiceterm.[ch]. (Same
OVE IDs apply.)
xenserver/vncterm looks the best to me. Apparently, it's been hardened
over time relative to the QEMU-derived original. For example, it does:
static int handle_params(TextConsole *s, int ch)
{
int i;
dprintf("putchar csi %02x '%c'\n", ch, ch > 0x1f ? ch : ' ');
if (ch >= '0' && ch <= '9') {
if (s->nb_esc_params < MAX_ESC_PARAMS && (s->esc_params[s->nb_esc_params] < 10000)) {
s->esc_params[s->nb_esc_params] =
s->esc_params[s->nb_esc_params] * 10 + ch - '0';
}
s->has_esc_param = 1;
return 0;
} else {
if (s->has_esc_param && s->nb_esc_params < MAX_ESC_PARAMS)
s->nb_esc_params++;
s->has_esc_param = 0;
if (ch == '?') {
s->has_qmark = 1;
return 0;
}
if (ch == ';')
return 0;
Notice that s->nb_esc_params is never increased to/beyond
MAX_ESC_PARAMS, and s->esc_params[] values are limited to at most ~100k.
The cursor movement commands use set_cursor(), which invokes clip_xy(),
which in turn performs all the needed range checks.
QEMU's ui/console.c looks OK'ish, but not nearly as hardened:
case TTY_STATE_CSI: /* handle escape sequence parameters */
if (ch >= '0' && ch <= '9') {
if (s->nb_esc_params < MAX_ESC_PARAMS) {
int *param = &s->esc_params[s->nb_esc_params];
int digit = (ch - '0');
*param = (*param <= (INT_MAX - digit) / 10) ?
*param * 10 + digit : INT_MAX;
}
} else {
if (s->nb_esc_params < MAX_ESC_PARAMS)
s->nb_esc_params++;
if (ch == ';' || ch == '?') {
break;
}
s->nb_esc_params is also never increased to/beyond MAX_ESC_PARAMS, and
integer overflow of s->esc_params[] values right here is prevented, but
they are not sanity-limited. This allows for integer overflows
(normally benign, albeit formally UB) e.g. in:
case 'B':
/* move cursor down */
if (s->esc_params[0] == 0) {
s->esc_params[0] = 1;
}
set_cursor(s, s->x, s->y + s->esc_params[0]);
break;
but then set_cursor()'s range checks prevent any out-of-bounds values
from staying in s->x and s->y.
Alexander
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
