Hi Jean-Francois,
Are you fine with attached patch? I saw that two other cmd_ functions
follow the same pattern, so they are probably also vulnerable, right?
thanks
Willi
Am 2016-12-30 um 19:16 schrieb Jean-Francois Dockes:
> Willi Mann writes:
> > Hi Dave,
> > Hi Jean-Francois,
> >
> > I got the following bug report, apparrently describing a buffer overflow
> > in unrtf - which I can reproduce. Do you have a suggestion for a fix?
> >
> > I'm also CCing debian's security team.
> >
> > WM
>
> I guess that you can just add a package patch to increate the str[] buffer
> size, something like
>
> - char str[10];
> + char str[15];
>
> (I'm sure that you could get by with less than 15 but I don't see the
> point).
>
> For completeness, sprintf() could be changed to snprintf(), but maybe this
> can be left for the next release?
>
> attr_push() does an strdup of the 2nd parameter, so the increased size
> should not be an issue there.
>
> I've not tested the change, but I'm foolishly confident that it should fix the
> issue. I'll give it a better look in the following days (and also look for
> possible other instances of the problem).
>
> jf
>
>
> > Am 2016-12-30 um 01:44 schrieb Skylake:
> > > Package: unrtf
> > > Version: 0.21.9-clean-2
> > >
> > > I've found a Stack-based buffer overflow in unrtf 0.21.9, which affects
> three
> > > functions including: cmd_expand, cmd_emboss and cmd_engrave.
> > >
> > > # convert.c
> > >
> > > static int
> > > cmd_expand (Word *w, int align, char has_param, int param) {
> > > char str[10];
> > > if (has_param) {
> > > sprintf(str, "%d", param/4); // Overflow, 9-digit negative
> value
> > > triggers the bug
> > > if (!param)
> > > attr_pop(ATTR_EXPAND);
> > > else
> > > attr_push(ATTR_EXPAND, str);
> > > }
> > > return FALSE;
> > > }
> > >
> > > Apparently writing a negative integer to the buffer can trigger the
> overflow
> > > (Minus sign needs an extra byte).
> > >
> > > * How to trigger the bug *
> > >
> > > $ echo "\expnd-400000000" > poc
> > > $ unrtf poc
> > >
> > > <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
> > > <html>
> > > <head>
> > > <meta http-equiv="content-type" content="text/html; charset=utf-8">
> > > <!-- Translation from RTF performed by UnRTF, version 0.21.9 -->
> > > *** buffer overflow detected ***: unrtf terminated
> > > ======= Backtrace: =========
> > > /lib/i386-linux-gnu/libc.so.6(+0x6737a)[0xb764f37a]
> > > /lib/i386-linux-gnu/libc.so.6(__fortify_fail+0x37)[0xb76dfe07]
> > > /lib/i386-linux-gnu/libc.so.6(+0xf60a8)[0xb76de0a8]
> > > /lib/i386-linux-gnu/libc.so.6(+0xf58b8)[0xb76dd8b8]
> > > /lib/i386-linux-gnu/libc.so.6(_IO_default_xsputn+0xa6)[0xb7653bf6]
> > > /lib/i386-linux-gnu/libc.so.6(_IO_vfprintf+0xf66)[0xb762b1d6]
> > > /lib/i386-linux-gnu/libc.so.6(__vsprintf_chk+0x90)[0xb76dd950]
> > > /lib/i386-linux-gnu/libc.so.6(__sprintf_chk+0x20)[0xb76dd8a0]
> > > unrtf[0x804c7b8]
> > > unrtf[0x804f77d]
> > > unrtf[0x804f9e7]
> > > unrtf[0x804920b]
> > > /lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf6)[0xb7600276]
> > > unrtf[0x804953c]
> > > ======= Memory map: ========
> > > 08048000-0805b000 r-xp 00000000 08:01 405354 /usr/bin/unrtf
> > > 0805b000-0805c000 r--p 00012000 08:01 405354 /usr/bin/unrtf
> > > 0805c000-0805d000 rw-p 00013000 08:01 405354 /usr/bin/unrtf
> > > 0805d000-08085000 rw-p 00000000 00:00 0
> > > 0952d000-0954e000 rw-p 00000000 00:00 0 [heap]
> > > b75ca000-b75e6000 r-xp 00000000 08:01 393233
> > > /usr/lib/i386-linux-gnu/libgcc_s.so.1
> > > b75e6000-b75e7000 r--p 0001b000 08:01 393233
> > > /usr/lib/i386-linux-gnu/libgcc_s.so.1
> > > b75e7000-b75e8000 rw-p 0001c000 08:01 393233
> > > /usr/lib/i386-linux-gnu/libgcc_s.so.1
> > > b75e8000-b7799000 r-xp 00000000 08:01 395818
> > > /usr/lib/i386-linux-gnu/libc-2.24.so
> > > b7799000-b779b000 r--p 001b0000 08:01 395818
> > > /usr/lib/i386-linux-gnu/libc-2.24.so
> > > b779b000-b779c000 rw-p 001b2000 08:01 395818
> > > /usr/lib/i386-linux-gnu/libc-2.24.so
> > > b779c000-b779f000 rw-p 00000000 00:00 0
> > > b77a3000-b77a6000 rw-p 00000000 00:00 0
> > > b77a6000-b77a8000 r--p 00000000 00:00 0 [vvar]
> > > b77a8000-b77aa000 r-xp 00000000 00:00 0 [vdso]
> > > b77aa000-b77cc000 r-xp 00000000 08:01 393914
> /usr/lib/i386-linux-gnu/ld-2.24.so
> > > b77cc000-b77cd000 rw-p 00000000 00:00 0
> > > b77cd000-b77ce000 r--p 00022000 08:01 393914
> /usr/lib/i386-linux-gnu/ld-2.24.so
> > > b77ce000-b77cf000 rw-p 00023000 08:01 393914
> /usr/lib/i386-linux-gnu/ld-2.24.so
> > > bf992000-bf9b3000 rw-p 00000000 00:00 0 [stack]
> > > Aborted
> > >
> > > * Test environment *
> > >
> > > Linux debian 4.7.0-1-686-pae #1 SMP Debian 4.7.8-1 (2016-10-19) i686
> GNU/Linux
> > > libc6 2.24-8
> > >
> > > Regards,
> > > Amir
> > >
> > > Sent with ProtonMail <https://protonmail.com> Secure Email.
> > >
> >
> >
>
From: Willi Mann <[email protected]>
Date: Sat, 31 Dec 2016 14:43:10 +0100
Subject: convert.c: Use safe buffer size and snprintf
cmd_expand, cmd_emboss, and cmd_engrave print an integer to a stack buffer.
Unfortunately, the previous buffer size of 10 is to small (e.g., to store -1 *
10^9), such that a buffer overflow could be provoked. This patch increases the
buffer size to 12 and switches to snprintf.
Bug-Debian: https://bugs.debian.org/849705
---
src/convert.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/convert.c b/src/convert.c
index c76d7d6..5294743 100644
--- a/src/convert.c
+++ b/src/convert.c
@@ -1373,9 +1373,9 @@ cmd_ftech (Word *w, int align, char has_param, int param) {
static int
cmd_expand (Word *w, int align, char has_param, int param) {
- char str[10];
+ char str[12];
if (has_param) {
- sprintf(str, "%d", param/4);
+ snprintf(str, 12, "%d", param/4);
if (!param)
attr_pop(ATTR_EXPAND);
else
@@ -1394,7 +1394,7 @@ cmd_expand (Word *w, int align, char has_param, int param) {
static int
cmd_emboss (Word *w, int align, char has_param, int param) {
- char str[10];
+ char str[12];
if (has_param && !param)
#ifdef SUPPORT_UNNESTED
attr_find_pop(ATTR_EMBOSS);
@@ -1403,7 +1403,7 @@ cmd_emboss (Word *w, int align, char has_param, int param) {
#endif
else
{
- sprintf(str, "%d", param);
+ snprintf(str, 12, "%d", param);
attr_push(ATTR_EMBOSS, str);
}
return FALSE;
@@ -1419,12 +1419,12 @@ cmd_emboss (Word *w, int align, char has_param, int param) {
static int
cmd_engrave (Word *w, int align, char has_param, int param) {
- char str[10];
+ char str[12];
if (has_param && !param)
attr_pop(ATTR_ENGRAVE);
else
{
- sprintf(str, "%d", param);
+ snprintf(str, 12, "%d", param);
attr_push(ATTR_ENGRAVE, str);
}
return FALSE;
