[rfc] Do not crash reading UPX binaries

[rfc] Do not crash reading UPX binaries

[Daniel Jacobowitz]

This patch issues an error instead of a segfault on the testcase
in PR 2280.  UPX is a binary compression system; it's infamous for
producing very strange files, which are only "just valid enough".
In this case, it claims that the symbol table is at a large offset
in a very small file.

I don't think it's worth supporting files this modified.  Does anyone
think we need to do better, or shall I check in the attached?

-- 
Daniel Jacobowitz
CodeSourcery

2007-07-01  Daniel Jacobowitz  <dan@codesourcery.com>

	PR gdb/2280
	* coffread.c (read_one_sym): Check for read errors.

Index: coffread.c
===================================================================
RCS file: /cvs/src/src/gdb/coffread.c,v
retrieving revision 1.73
diff -u -p -r1.73 coffread.c
--- coffread.c	19 Jun 2007 17:21:51 -0000	1.73
+++ coffread.c	1 Jul 2007 21:53:06 -0000
@@ -1118,20 +1118,29 @@ read_one_sym (struct coff_symbol *cs,
 	      union internal_auxent *aux)
 {
   int i;
+  bfd_size_type bytes;
 
   cs->c_symnum = symnum;
-  bfd_bread (temp_sym, local_symesz, nlist_bfd_global);
+  bytes = bfd_bread (temp_sym, local_symesz, nlist_bfd_global);
+  if (bytes != local_symesz)
+    error ("%s: error reading symbols", current_objfile->name);
   bfd_coff_swap_sym_in (symfile_bfd, temp_sym, (char *) sym);
   cs->c_naux = sym->n_numaux & 0xff;
   if (cs->c_naux >= 1)
     {
-      bfd_bread (temp_aux, local_auxesz, nlist_bfd_global);
+      bytes  = bfd_bread (temp_aux, local_auxesz, nlist_bfd_global);
+      if (bytes != local_auxesz)
+	error ("%s: error reading symbols", current_objfile->name);
       bfd_coff_swap_aux_in (symfile_bfd, temp_aux, sym->n_type, sym->n_sclass,
 			    0, cs->c_naux, (char *) aux);
       /* If more than one aux entry, read past it (only the first aux
          is important). */
       for (i = 1; i < cs->c_naux; i++)
-	bfd_bread (temp_aux, local_auxesz, nlist_bfd_global);
+	{
+	  bytes = bfd_bread (temp_aux, local_auxesz, nlist_bfd_global);
+	  if (bytes != local_auxesz)
+	    error ("%s: error reading symbols", current_objfile->name);
+	}
     }
   cs->c_name = getsymname (sym);
   cs->c_value = sym->n_value;

Re: [rfc] Do not crash reading UPX binaries

[Ismail Dönmez]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On Monday 02 July 2007 00:55:49 Daniel Jacobowitz wrote:
> This patch issues an error instead of a segfault on the testcase
> in PR 2280.  UPX is a binary compression system; it's infamous for
> producing very strange files, which are only "just valid enough".
> In this case, it claims that the symbol table is at a large offset
> in a very small file.
>
> I don't think it's worth supporting files this modified.  Does anyone
> think we need to do better, or shall I check in the attached?

Please not that attached gdbupx is from a security advisory [0] [1] but it 
looks like a simple DoS.

[0] http://blog.xwings.net/?p=71
[1] http://blogs.securiteam.com/index.php/archives/922

Regards,
ismail

-- 
Perfect is the enemy of good

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [rfc] Do not crash reading UPX binaries

[Daniel Jacobowitz]

On Mon, Jul 02, 2007 at 01:12:27AM +0300, Ismail Dönmez wrote:
> Please not that attached gdbupx is from a security advisory [0] [1] but it 
> looks like a simple DoS.
> 
> [0] http://blog.xwings.net/?p=71
> [1] http://blogs.securiteam.com/index.php/archives/922

That would be useful in the report next time :-)  I spent twenty
minutes figuring out what was going on, and yes, it was the invalid
symbol table pointer.  I'm happy to hear that this won't affect all
UPX files after all, just truncated ones.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Do not crash reading UPX binaries

[Ismail Dönmez]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On Monday 02 July 2007 01:27:47 Daniel Jacobowitz wrote:
> On Mon, Jul 02, 2007 at 01:12:27AM +0300, Ismail Dönmez wrote:
> > Please not that attached gdbupx is from a security advisory [0] [1] but
> > it looks like a simple DoS.
> >
> > [0] http://blog.xwings.net/?p=71
> > [1] http://blogs.securiteam.com/index.php/archives/922
>
> That would be useful in the report next time :-)  I spent twenty
> minutes figuring out what was going on, and yes, it was the invalid
> symbol table pointer.  I'm happy to hear that this won't affect all
> UPX files after all, just truncated ones.

The reporting system was scary, and it didn't return me back to the bug report 
to let me add the references. Sorry and thanks for the fast fix.

Regards,
ismail

-- 
Perfect is the enemy of good

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [rfc] Do not crash reading UPX binaries

[Daniel Jacobowitz]

On Mon, Jul 02, 2007 at 01:30:55AM +0300, Ismail Dönmez wrote:
> The reporting system was scary, and it didn't return me back to the bug report 
> to let me add the references. Sorry and thanks for the fast fix.

Blech.  Yes, I anticipate that we will (someday...) switch away from gnats.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Do not crash reading UPX binaries

[Daniel Jacobowitz]

On Sun, Jul 01, 2007 at 05:55:49PM -0400, Daniel Jacobowitz wrote:
> 2007-07-01  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	PR gdb/2280
> 	* coffread.c (read_one_sym): Check for read errors.

I finally remembered to commit this.

-- 
Daniel Jacobowitz
CodeSourcery