From 9d3c0bdfa40f71cc5b75f6251bd2ac7743e88091 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Thu, 4 Dec 2014 17:32:53 +0100 Subject: [PATCH 1/6] process_color_table(): Prevent NULL pointer dereference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prevents crashes reported by Hanno Böck at: https://lists.gnu.org/archive/html/bug-unrtf/2014-11/msg00000.html with input file id:000072,src:000000,op:flip1,pos:344 and id:000078,src:000000,op:flip1,pos:345. --- src/convert.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/convert.c b/src/convert.c index e563473..f94cce4 100644 --- a/src/convert.c +++ b/src/convert.c @@ -870,7 +870,10 @@ process_color_table (Word *w) while(w) { char *s = word_string (w); - if (!strncmp("\\red",s,4)) { + if (s == NULL) { + warning_handler ("Skipping invalid word while processing color table!"); + } + else if (!strncmp("\\red",s,4)) { r = atoi(&s[4]); while(r>255) r>>=8; } -- 2.1.2 From 5d0f4e0c53e6285720b6558985712dd339e70e35 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Thu, 4 Dec 2014 17:52:43 +0100 Subject: [PATCH 2/6] process_font_table(): Prevent NULL pointer dereference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prevents crashes reported by Hanno Böck at: https://lists.gnu.org/archive/html/bug-unrtf/2014-11/msg00000.html with input files id:000010,src:000000,op:flip1,pos:37, id:000011,src:000000,op:flip1,pos:38 and id:000025,src:000000,op:flip1,pos:119. --- src/convert.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/convert.c b/src/convert.c index f94cce4..edc94f6 100644 --- a/src/convert.c +++ b/src/convert.c @@ -524,7 +524,9 @@ process_font_table (Word *w) if ((w2 = w->child)) { tmp = word_string(w2); - if (!strncmp("\\f", tmp, 2)) { + if (tmp == NULL) { + warning_handler ("Skipping unexpected word while processing font table!"); + } else if (!strncmp("\\f", tmp, 2)) { num = atoi(&tmp[2]); name[0] = 0; int cpgcp = -1; -- 2.1.2 From bb57bc669fa3b8f5a75db3dc7e8bd0cf2d7c872e Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Thu, 4 Dec 2014 17:50:44 +0100 Subject: [PATCH 3/6] Adjust a couple of sanity checks to detect negative font color values Prevents segmentation faults like the one reported by Michal Zalewski at: https://lists.gnu.org/archive/html/bug-unrtf/2014-11/msg00001.html --- src/convert.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/convert.c b/src/convert.c index edc94f6..efbb558 100644 --- a/src/convert.c +++ b/src/convert.c @@ -926,7 +926,7 @@ static int cmd_cf (Word *w, int align, char has_param, int num) { char str[40]; - if (!has_param || num>=total_colors) { + if (!has_param || (unsigned)num>=total_colors) { warning_handler ("font color change attempted is invalid"); } else @@ -953,7 +953,7 @@ static int cmd_cb (Word *w, int align, char has_param, int num) { char str[40]; - if (!has_param || num>=total_colors) { + if (!has_param || (unsigned)num>=total_colors) { warning_handler ("font color change attempted is invalid"); } else @@ -1158,7 +1158,7 @@ cmd_highlight (Word *w, int align, char has_param, int num) { char str[40]; - if (!has_param || num>=total_colors) { + if (!has_param || (unsigned)num>=total_colors) { warning_handler ("font background color change attempted is invalid"); } else -- 2.1.2 From 90af2a52bfcecd7bde4c2984d9ca9ee22727ce89 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Thu, 4 Dec 2014 18:15:29 +0100 Subject: [PATCH 4/6] attrstack_drop(): Properly drop the last stack element Previously stack_of_stacks_top would point to free'd memory, resulting in: ==38960== Invalid read of size 4 ==38960== at 0x402853: attr_get_param (attr.c:355) ==38960== by 0x40818A: word_print_core (convert.c:3412) ==38960== by 0x406DBC: word_print (convert.c:3451) ==38960== by 0x40CA27: main (main.c:267) ==38960== Address 0x1e065e0 is 90,000 bytes inside a block of size 90,016 free'd ==38960== at 0x1068498: free (in /usr/local/lib/valgrind/vgpreload_memcheck-amd64-freebsd.so) ==38960== by 0x40CBD3: my_free (malloc.c:91) ==38960== by 0x402E8C: attrstack_drop (attr.c:582) ==38960== by 0x40812F: word_print_core (convert.c:3403) ==38960== by 0x406DBC: word_print (convert.c:3451) ==38960== by 0x40CA27: main (main.c:267) ==38960== --- src/attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/attr.c b/src/attr.c index bc19b6c..2c2552b 100644 --- a/src/attr.c +++ b/src/attr.c @@ -571,7 +571,7 @@ attrstack_drop () while(prev_stack && prev_stack->next && prev_stack->next != stack) prev_stack = prev_stack->next; - if (prev_stack) { + if (prev_stack && (prev_stack != stack_of_stacks_top)) { stack_of_stacks_top = prev_stack; prev_stack->next = NULL; } else { -- 2.1.2 From b3e92e5d92e94f807c68f427b3e3d07358fb85a8 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Thu, 4 Dec 2014 18:20:12 +0100 Subject: [PATCH 5/6] attr_get_param(): Silence a warning message again attr_get_param(ATTR_ENCODING) is always called once without a stack being available, but previously the use-after-free prevented the warning. --- src/attr.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/attr.c b/src/attr.c index 2c2552b..0337fd0 100644 --- a/src/attr.c +++ b/src/attr.c @@ -348,8 +348,14 @@ attr_get_param(int attr) int i; AttrStack *stack = stack_of_stacks_top; if (!stack) { - warning_handler("No stack to get attribute from"); - return; + if (attr != ATTR_ENCODING) { + /* + * attr_get_param(ATTR_ENCODING) is always called + * called once without a stack being available. + */ + warning_handler("No stack to get attribute from"); + } + return NULL; } i=stack->tos; -- 2.1.2 From 419251480132d4e5f289f6e794b8bad8332bdd59 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Fri, 5 Dec 2014 12:29:34 +0100 Subject: [PATCH 6/6] hash_get_string(): Reject invalid indexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should fix crashes reported by Hanno Böck that somehow are only reproducible with afl-fuzz and afl-showmap. http://seclists.org/oss-sec/2014/q4/909 --- src/hash.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/hash.c b/src/hash.c index b886d1e..76821da 100644 --- a/src/hash.c +++ b/src/hash.c @@ -207,6 +207,10 @@ hash_get_string (unsigned long value) HashItem *hi; index = value >> 24; + if ((unsigned)index > 255) { + warning_handler("Hash index out of range"); + return NULL; + } hi = hash[index]; while (hi) { if (hi->value == value) -- 2.1.2