From 0897ec15810bca3420ea7b8a91e491ed45780202 Mon Sep 17 00:00:00 2001 From: Nick Clifton Date: Tue, 27 Jan 2015 17:32:23 +0000 Subject: [PATCH] Fixes for invalid memory accesses triggered by running windres on corrupt binaries. PR binutils/17512 * rcparse.y: Add checks to avoid integer divide by zero. * rescoff.c (read_coff_rsrc): Add check on the size of the resource section. (read_coff_res_dir): Add check on the nesting level. Check for resource names overrunning the buffer. * resrc.c (write_rc_messagetable): Update formatting. Add check of 'elen' being zero. --- binutils/ChangeLog | 8 ++++ binutils/rcparse.y | 9 ++-- binutils/rescoff.c | 22 +++++++++- binutils/resrc.c | 102 +++++++++++++++++++++++++-------------------- 4 files changed, 89 insertions(+), 52 deletions(-) diff --git a/binutils/ChangeLog b/binutils/ChangeLog index 53ec0725c5..924f35c84c 100644 --- a/binutils/ChangeLog +++ b/binutils/ChangeLog @@ -7,6 +7,14 @@ * addr2line.c (slurp_symtab): If the symcount is zero, free the symbol table pointer. + * rcparse.y: Add checks to avoid integer divide by zero. + * rescoff.c (read_coff_rsrc): Add check on the size of the + resource section. + (read_coff_res_dir): Add check on the nesting level. + Check for resource names overrunning the buffer. + * resrc.c (write_rc_messagetable): Update formatting. + Add check of 'elen' being zero. + 2015-01-23 Nick Clifton * nlmconv.c (powerpc_mangle_relocs): Fix build errors introduced diff --git a/binutils/rcparse.y b/binutils/rcparse.y index 78ac57eee9..0cf6c2c1f2 100644 --- a/binutils/rcparse.y +++ b/binutils/rcparse.y @@ -1887,12 +1887,12 @@ sizednumexpr: } | sizednumexpr '/' sizednumexpr { - $$.val = $1.val / $3.val; + $$.val = $1.val / ($3.val ? $3.val : 1); $$.dword = $1.dword || $3.dword; } | sizednumexpr '%' sizednumexpr { - $$.val = $1.val % $3.val; + $$.val = $1.val % ($3.val ? $3.val : 1); $$.dword = $1.dword || $3.dword; } | sizednumexpr '+' sizednumexpr @@ -1966,12 +1966,13 @@ sizedposnumexpr: } | sizedposnumexpr '/' sizednumexpr { - $$.val = $1.val / $3.val; + $$.val = $1.val / ($3.val ? $3.val : 1); $$.dword = $1.dword || $3.dword; } | sizedposnumexpr '%' sizednumexpr { - $$.val = $1.val % $3.val; + /* PR 17512: file: 89105a25. */ + $$.val = $1.val % ($3.val ? $3.val : 1); $$.dword = $1.dword || $3.dword; } | sizedposnumexpr '+' sizednumexpr diff --git a/binutils/rescoff.c b/binutils/rescoff.c index 0004c3aa35..3e63e49901 100644 --- a/binutils/rescoff.c +++ b/binutils/rescoff.c @@ -142,8 +142,14 @@ read_coff_rsrc (const char *filename, const char *target) set_windres_bfd (&wrbfd, abfd, sec, WR_KIND_BFD); size = bfd_section_size (abfd, sec); - data = (bfd_byte *) res_alloc (size); + /* PR 17512: file: 1b25ba5d + The call to get_file_size here may be expensive + but there is no other way to determine if the section size + is reasonable. */ + if (size > (bfd_size_type) get_file_size (filename)) + fatal (_("%s: .rsrc section is bigger than the file!"), filename); + data = (bfd_byte *) res_alloc (size); get_windres_bfd_content (&wrbfd, data, 0, size); flaginfo.filename = filename; @@ -185,6 +191,13 @@ read_coff_res_dir (windres_bfd *wrbfd, const bfd_byte *data, rc_res_entry **pp; const struct extern_res_entry *ere; + /* PR 17512: file: 09d80f53. + Whilst in theory resources can nest to any level, in practice + Microsoft only defines 3 levels. Corrupt files however might + claim to use more. */ + if (level > 4) + overrun (flaginfo, _("Resources nest too deep")); + if ((size_t) (flaginfo->data_end - data) < sizeof (struct extern_res_directory)) overrun (flaginfo, _("directory")); @@ -234,7 +247,12 @@ read_coff_res_dir (windres_bfd *wrbfd, const bfd_byte *data, re->id.u.n.length = length; re->id.u.n.name = (unichar *) res_alloc (length * sizeof (unichar)); for (j = 0; j < length; j++) - re->id.u.n.name[j] = windres_get_16 (wrbfd, ers + j * 2 + 2, 2); + { + /* PR 17512: file: 05dc4a16. */ + if (length < 0 || ers >= (bfd_byte *) ere || ers + j * 2 + 4 >= (bfd_byte *) ere) + overrun (flaginfo, _("resource name")); + re->id.u.n.name[j] = windres_get_16 (wrbfd, ers + j * 2 + 2, 2); + } if (level == 0) type = &re->id; diff --git a/binutils/resrc.c b/binutils/resrc.c index 4e0b24c90f..f0cacd16b1 100644 --- a/binutils/resrc.c +++ b/binutils/resrc.c @@ -2932,53 +2932,63 @@ write_rc_messagetable (FILE *e, rc_uint_type length, const bfd_byte *data) if (length < BIN_MESSAGETABLE_SIZE) has_error = 1; else - do { - rc_uint_type m, i; - mt = (const struct bin_messagetable *) data; - m = windres_get_32 (&wrtarget, mt->cblocks, length); - if (length < (BIN_MESSAGETABLE_SIZE + m * BIN_MESSAGETABLE_BLOCK_SIZE)) - { - has_error = 1; - break; - } - for (i = 0; i < m; i++) - { - rc_uint_type low, high, offset; - const struct bin_messagetable_item *mti; + do + { + rc_uint_type m, i; + + mt = (const struct bin_messagetable *) data; + m = windres_get_32 (&wrtarget, mt->cblocks, length); + + if (length < (BIN_MESSAGETABLE_SIZE + m * BIN_MESSAGETABLE_BLOCK_SIZE)) + { + has_error = 1; + break; + } + for (i = 0; i < m; i++) + { + rc_uint_type low, high, offset; + const struct bin_messagetable_item *mti; + + low = windres_get_32 (&wrtarget, mt->items[i].lowid, 4); + high = windres_get_32 (&wrtarget, mt->items[i].highid, 4); + offset = windres_get_32 (&wrtarget, mt->items[i].offset, 4); + while (low <= high) + { + rc_uint_type elen, flags; + if ((offset + BIN_MESSAGETABLE_ITEM_SIZE) > length) + { + has_error = 1; + break; + } + mti = (const struct bin_messagetable_item *) &data[offset]; + elen = windres_get_16 (&wrtarget, mti->length, 2); + flags = windres_get_16 (&wrtarget, mti->flags, 2); + if ((offset + elen) > length) + { + has_error = 1; + break; + } + wr_printcomment (e, "MessageId = 0x%x", low); + wr_printcomment (e, ""); + + /* PR 17512: file: 5c3232dc. */ + if (elen) + { + if ((flags & MESSAGE_RESOURCE_UNICODE) == MESSAGE_RESOURCE_UNICODE) + unicode_print (e, (const unichar *) mti->data, + (elen - BIN_MESSAGETABLE_ITEM_SIZE) / 2); + else + ascii_print (e, (const char *) mti->data, + (elen - BIN_MESSAGETABLE_ITEM_SIZE)); + } + wr_printcomment (e,""); + ++low; + offset += elen; + } + } + } + while (0); - low = windres_get_32 (&wrtarget, mt->items[i].lowid, 4); - high = windres_get_32 (&wrtarget, mt->items[i].highid, 4); - offset = windres_get_32 (&wrtarget, mt->items[i].offset, 4); - while (low <= high) - { - rc_uint_type elen, flags; - if ((offset + BIN_MESSAGETABLE_ITEM_SIZE) > length) - { - has_error = 1; - break; - } - mti = (const struct bin_messagetable_item *) &data[offset]; - elen = windres_get_16 (&wrtarget, mti->length, 2); - flags = windres_get_16 (&wrtarget, mti->flags, 2); - if ((offset + elen) > length) - { - has_error = 1; - break; - } - wr_printcomment (e, "MessageId = 0x%x", low); - wr_printcomment (e, ""); - if ((flags & MESSAGE_RESOURCE_UNICODE) == MESSAGE_RESOURCE_UNICODE) - unicode_print (e, (const unichar *) mti->data, - (elen - BIN_MESSAGETABLE_ITEM_SIZE) / 2); - else - ascii_print (e, (const char *) mti->data, - (elen - BIN_MESSAGETABLE_ITEM_SIZE)); - wr_printcomment (e,""); - ++low; - offset += elen; - } - } - } while (0); if (has_error) wr_printcomment (e, "Illegal data"); wr_print_flush (e); -- 2.34.1