Patchwork Fix CVE-2017-9778

login
register
mail settings
Submitter Sandra Loosemore
Date April 24, 2019, 4:27 p.m.
Message ID <e2b9a14b-97de-a926-9e82-1f678e893944@codesourcery.com>
Download mbox | patch
Permalink /patch/32403/
State New
Headers show

Comments

Sandra Loosemore - April 24, 2019, 4:27 p.m.
We had a request from a customer to fix CVE-2017-9778 (aka PR 
gdb/21600).  They don't really care about this particular bug, just that 
they can cross it off the list of known vulnerabilities in GDB.

This patch is based on the one attached to the issue.  I also cleaned up 
a bunch of pointless conversions between signed and unsigned 
representations of the length field, and made sure the length == 0 case 
retains its special meaning.

OK to commit?

-Sandra
Kevin Buettner - April 25, 2019, 12:56 a.m.
On Wed, 24 Apr 2019 10:27:39 -0600
Sandra Loosemore <sandra@codesourcery.com> wrote:

>     GDB was failing to catch cases where a corrupt ELF or core file
>     contained an invalid length value in a Dwarf debug frame FDE header.
>     It was checking for buffer overflow but not cases where the length was
>     negative or caused pointer wrap-around.
>     
>     In addition to the additional validity check, this patch cleans up the
>     multiple signed/unsigned conversions on the length field so that an
>     unsigned representation is used consistently throughout.
>     
>     2019-04-24  Sandra Loosemore  <sandra@codesourcery.com>
>     	    Kang Li <kanglictf@gmail.com>
>     
>     	PR gdb/21600
>     
>     	* dwarf2-frame.c (read_initial_length): Be consistent about using
>     	unsigned representation of length.
>     	(decode_frame_entry_1): Likewise.  Check for wraparound of
>     	end pointer as well as buffer overflow.

This is okay.

Kevin
Simon Marchi - April 25, 2019, 3:25 a.m.
On 2019-04-24 20:56, Kevin Buettner wrote:
> On Wed, 24 Apr 2019 10:27:39 -0600
> Sandra Loosemore <sandra@codesourcery.com> wrote:
> 
>>     GDB was failing to catch cases where a corrupt ELF or core file
>>     contained an invalid length value in a Dwarf debug frame FDE 
>> header.
>>     It was checking for buffer overflow but not cases where the length 
>> was
>>     negative or caused pointer wrap-around.
>> 
>>     In addition to the additional validity check, this patch cleans up 
>> the
>>     multiple signed/unsigned conversions on the length field so that 
>> an
>>     unsigned representation is used consistently throughout.
>> 
>>     2019-04-24  Sandra Loosemore  <sandra@codesourcery.com>
>>     	    Kang Li <kanglictf@gmail.com>
>> 
>>     	PR gdb/21600
>> 
>>     	* dwarf2-frame.c (read_initial_length): Be consistent about using
>>     	unsigned representation of length.
>>     	(decode_frame_entry_1): Likewise.  Check for wraparound of
>>     	end pointer as well as buffer overflow.
> 
> This is okay.
> 
> Kevin

I would just suggest using a more descriptive commit title, stating what 
the commit actually changes in the code.  It's still good to reference 
the CVE number, but by itself is not very descriptive.

Thanks,

Simon
Sandra Loosemore - April 25, 2019, 2:34 p.m.
On 4/24/19 9:25 PM, Simon Marchi wrote:
> On 2019-04-24 20:56, Kevin Buettner wrote:
>> On Wed, 24 Apr 2019 10:27:39 -0600
>> Sandra Loosemore <sandra@codesourcery.com> wrote:
>>
>>>     GDB was failing to catch cases where a corrupt ELF or core file
>>>     contained an invalid length value in a Dwarf debug frame FDE header.
>>>     It was checking for buffer overflow but not cases where the 
>>> length was
>>>     negative or caused pointer wrap-around.
>>>
>>>     In addition to the additional validity check, this patch cleans 
>>> up the
>>>     multiple signed/unsigned conversions on the length field so that an
>>>     unsigned representation is used consistently throughout.
>>>
>>>     2019-04-24  Sandra Loosemore  <sandra@codesourcery.com>
>>>             Kang Li <kanglictf@gmail.com>
>>>
>>>         PR gdb/21600
>>>
>>>         * dwarf2-frame.c (read_initial_length): Be consistent about 
>>> using
>>>         unsigned representation of length.
>>>         (decode_frame_entry_1): Likewise.  Check for wraparound of
>>>         end pointer as well as buffer overflow.
>>
>> This is okay.
>>
>> Kevin
> 
> I would just suggest using a more descriptive commit title, stating what 
> the commit actually changes in the code.  It's still good to reference 
> the CVE number, but by itself is not very descriptive.

Done.  I pushed it as "Detect invalid length field in debug frame FDE 
header."

-Sandra
Kevin Buettner - April 25, 2019, 3:53 p.m.
On Wed, 24 Apr 2019 23:25:45 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2019-04-24 20:56, Kevin Buettner wrote:
> > On Wed, 24 Apr 2019 10:27:39 -0600
> > Sandra Loosemore <sandra@codesourcery.com> wrote:
> >   
> >>     GDB was failing to catch cases where a corrupt ELF or core file
> >>     contained an invalid length value in a Dwarf debug frame FDE 
> >> header.
> >>     It was checking for buffer overflow but not cases where the length 
> >> was
> >>     negative or caused pointer wrap-around.
> >> 
> >>     In addition to the additional validity check, this patch cleans up 
> >> the
> >>     multiple signed/unsigned conversions on the length field so that 
> >> an
> >>     unsigned representation is used consistently throughout.
> >> 
> >>     2019-04-24  Sandra Loosemore  <sandra@codesourcery.com>
> >>     	    Kang Li <kanglictf@gmail.com>
> >> 
> >>     	PR gdb/21600
> >> 
> >>     	* dwarf2-frame.c (read_initial_length): Be consistent about using
> >>     	unsigned representation of length.
> >>     	(decode_frame_entry_1): Likewise.  Check for wraparound of
> >>     	end pointer as well as buffer overflow.  
> > 
> > This is okay.
> > 
> > Kevin  
> 
> I would just suggest using a more descriptive commit title, stating what 
> the commit actually changes in the code.  It's still good to reference 
> the CVE number, but by itself is not very descriptive.

Yes, good point.

I'm glad that Sandra saw your suggestion prior to pushing that commit.

Kevin

Patch

commit 30d251c35871b0e8aff16d60df3db3d327e34bfa
Author: Sandra Loosemore <sandra@codesourcery.com>
Date:   Wed Apr 24 08:53:26 2019 -0700

    Fix CVE-2017-9778.
    
    GDB was failing to catch cases where a corrupt ELF or core file
    contained an invalid length value in a Dwarf debug frame FDE header.
    It was checking for buffer overflow but not cases where the length was
    negative or caused pointer wrap-around.
    
    In addition to the additional validity check, this patch cleans up the
    multiple signed/unsigned conversions on the length field so that an
    unsigned representation is used consistently throughout.
    
    2019-04-24  Sandra Loosemore  <sandra@codesourcery.com>
    	    Kang Li <kanglictf@gmail.com>
    
    	PR gdb/21600
    
    	* dwarf2-frame.c (read_initial_length): Be consistent about using
    	unsigned representation of length.
    	(decode_frame_entry_1): Likewise.  Check for wraparound of
    	end pointer as well as buffer overflow.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5c40683..deab585 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@ 
+2019-04-24  Sandra Loosemore  <sandra@codesourcery.com>
+	    Kang Li <kanglictf@gmail.com>
+
+	PR gdb/21600
+
+	* dwarf2-frame.c (read_initial_length): Be consistent about using
+	unsigned representation of length.
+	(decode_frame_entry_1): Likewise.  Check for wraparound of
+	end pointer as well as buffer overflow.
+
 2019-04-23  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* s12z-tdep.c (s12z_unwind_pc): Delete.
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index e2bf61b..b697afa 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1487,7 +1487,7 @@  static ULONGEST
 read_initial_length (bfd *abfd, const gdb_byte *buf,
 		     unsigned int *bytes_read_ptr)
 {
-  LONGEST result;
+  ULONGEST result;
 
   result = bfd_get_32 (abfd, buf);
   if (result == 0xffffffff)
@@ -1788,7 +1788,7 @@  decode_frame_entry_1 (struct comp_unit *unit, const gdb_byte *start,
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   const gdb_byte *buf, *end;
-  LONGEST length;
+  ULONGEST length;
   unsigned int bytes_read;
   int dwarf64_p;
   ULONGEST cie_id;
@@ -1799,15 +1799,15 @@  decode_frame_entry_1 (struct comp_unit *unit, const gdb_byte *start,
   buf = start;
   length = read_initial_length (unit->abfd, buf, &bytes_read);
   buf += bytes_read;
-  end = buf + length;
-
-  /* Are we still within the section?  */
-  if (end > unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-    return NULL;
+  end = buf + (size_t) length;
 
   if (length == 0)
     return end;
 
+  /* Are we still within the section?  */
+  if (end <= buf || end > unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+    return NULL;
+
   /* Distinguish between 32 and 64-bit encoded frame info.  */
   dwarf64_p = (bytes_read == 12);