[patch] Fix find_separate_debug_file buffer overrun [Re: gdb crash during read of separate debuginfo]

[patch] Fix find_separate_debug_file buffer overrun [Re: gdb crash during read of separate debuginfo]

[Jan Kratochvil]

Hi Richard,

On Sun, 02 Aug 2009 11:40:12 +0200, Richard Guenther wrote:
> 
> We experienced crashes when running gdb inside out installation
> system which has /usr symlinked to some location beyond /mnt.
> The issue is that the code doesn't deal with the case that
> the result of lrealpath is longer than its argument.

thanks, posting updated patch for FSF GDB as it is not a Fedora regression.
This attached patch has not been reviewed by Richard Guenther.

The testcase would no longer reproduce the bug after some patch like this one
would get accepted as the IMO only exploitable code path gets removed:
	Re: [patch] Replace reread_symbols by load+free calls
	http://sourceware.org/ml/gdb-patches/2009-06/msg00696.html

At the same time this reread_symbols patch fixes some bug not further
investigated currently exposed by commented-out runto_main in this testcase.

I do not push much to get the testcase accepted.

No regressions on {x86_64,x86_64-m32,i686}-fedora11-linux-gnu.


Thanks,
Jan


gdb/
2009-08-02  Richard Guenther  <rguenther@suse.de>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix memory corruption on reread of file through a symbolic link.
	* symfile.c (find_separate_debug_file): Initialize CANON_NAME earlier.
	Allocate DEBUGFILE with length based on CANON_NAME.  Free CANON_NAME on
	all the return paths.

gdb/testsuite/
2009-08-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/debuginfo-lrealpath-overflow.exp: New.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1388,8 +1388,14 @@ find_separate_debug_file (struct objfile *objfile)
   gdb_assert (i >= 0 && IS_DIR_SEPARATOR (dir[i]));
   dir[i+1] = '\0';
 
+  /* Set I to max (strlen (canon_name), strlen (dir)). */
+  canon_name = lrealpath (dir);
+  i = strlen (dir);
+  if (canon_name && strlen (canon_name) > i)
+    i = strlen (canon_name);
+
   debugfile = alloca (strlen (debug_file_directory) + 1
-                      + strlen (dir)
+                      + i
                       + strlen (DEBUG_SUBDIRECTORY)
                       + strlen ("/")
                       + strlen (basename)
@@ -1403,6 +1409,7 @@ find_separate_debug_file (struct objfile *objfile)
     {
       xfree (basename);
       xfree (dir);
+      xfree (canon_name);
       return xstrdup (debugfile);
     }
 
@@ -1416,6 +1423,7 @@ find_separate_debug_file (struct objfile *objfile)
     {
       xfree (basename);
       xfree (dir);
+      xfree (canon_name);
       return xstrdup (debugfile);
     }
 
@@ -1429,12 +1437,12 @@ find_separate_debug_file (struct objfile *objfile)
     {
       xfree (basename);
       xfree (dir);
+      xfree (canon_name);
       return xstrdup (debugfile);
     }
 
   /* If the file is in the sysroot, try using its base path in the
      global debugfile directory.  */
-  canon_name = lrealpath (dir);
   if (canon_name
       && strncmp (canon_name, gdb_sysroot, strlen (gdb_sysroot)) == 0
       && IS_DIR_SEPARATOR (canon_name[strlen (gdb_sysroot)]))
@@ -1449,6 +1457,7 @@ find_separate_debug_file (struct objfile *objfile)
 	  xfree (canon_name);
 	  xfree (basename);
 	  xfree (dir);
+	  xfree (canon_name);
 	  return xstrdup (debugfile);
 	}
     }
--- /dev/null
+++ b/gdb/testsuite/gdb.base/debuginfo-lrealpath-overflow.exp
@@ -0,0 +1,67 @@
+# Copyright (C) 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test debuginfo-lrealpath-overflow
+set binfile ${objdir}/${subdir}/${test}
+set binfileloadbase ${test}-load
+set binfileload ${objdir}/${subdir}/${binfileloadbase}
+
+# Length 250 should be under NAME_MAX and still big enough for a stack overflow.
+set pattern [string repeat 0123456789 25]
+set longdirbase [string range ${test}-${pattern} 0 249]
+set longdir ${objdir}/${subdir}/${longdirbase}
+
+if {[build_executable ${test}.exp ${test} start.c debug] == -1} {
+    return -1
+}
+
+# find_separate_debug_file is during the initial load called by
+# symbol_file_add_with_addrs_or_offsets on objfile->name which is already the
+# resolved long name.  Use the reread_separate_symbols caller with
+# objfile->name created at time when still the long directory name and symlink
+# still did not exist.
+
+remote_exec build "rm -f ${binfileload}"
+remote_exec build "mkdir -p ${binfileload}"
+remote_exec build "mv -f ${binfile} ${binfileload}/${test}"
+
+clean_restart ${binfileloadbase}/${test}
+
+remote_exec build "rm -rf ${longdir}"
+remote_exec build "mv -f ${binfileload} ${longdir}"
+remote_exec build "ln -s ${longdirbase} ${binfileload}"
+
+# Note: the procedure gdb_gnu_strip_debug will produce an executable called
+# ${binfile}, which is just like the executable ($binfile) but without
+# the debuginfo. Instead $binfile has a .gnudebuglink section which contains
+# the name of a debuginfo only file. This file will be stored in the
+# gdb.base/.debug subdirectory.
+
+if [gdb_gnu_strip_debug ${binfileload}/${test}] {
+    # check that you have a recent version of strip and objcopy installed
+    unsupported "cannot produce separate debug info files"
+    return -1
+}
+
+# Delete the separate debug info file to call lrealpath at all.
+remote_exec build "rm -f ${binfileload}/.debug/${test}.debug"
+
+sleep 1
+remote_exec build "touch ${binfileload}/${test}"
+
+# runto_main could expose a reread_symbols bug giving false FAIL here.
+
+gdb_run_cmd
+gdb_test "" "" "catch the prompt"

Re: [patch] Fix find_separate_debug_file buffer overrun [Re: gdb crash during read of separate debuginfo]

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Richard> We experienced crashes when running gdb inside out installation
Richard> system which has /usr symlinked to some location beyond /mnt.
Richard> The issue is that the code doesn't deal with the case that
Richard> the result of lrealpath is longer than its argument.

Jan> thanks, posting updated patch for FSF GDB as it is not a Fedora regression.
Jan> This attached patch has not been reviewed by Richard Guenther.

Jan> 2009-08-02  Richard Guenther  <rguenther@suse.de>
Jan> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	Fix memory corruption on reread of file through a symbolic link.
Jan> 	* symfile.c (find_separate_debug_file): Initialize CANON_NAME earlier.
Jan> 	Allocate DEBUGFILE with length based on CANON_NAME.  Free CANON_NAME on
Jan> 	all the return paths.

This looks good to me.
Ok.

Jan> I do not push much to get the testcase accepted.

Is there something in particular you think is wrong with it?  It looks
ok to me, but your comment makes me wonder what subtlety I missed.

Tom

Re: [patch] Fix find_separate_debug_file buffer overrun [Re: gdb crash during read of separate debuginfo]

[Jan Kratochvil]

On Mon, 03 Aug 2009 18:46:34 +0200, Tom Tromey wrote:
> Jan> 2009-08-02  Richard Guenther  <rguenther@suse.de>
> Jan> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	Fix memory corruption on reread of file through a symbolic link.
> Jan> 	* symfile.c (find_separate_debug_file): Initialize CANON_NAME earlier.
> Jan> 	Allocate DEBUGFILE with length based on CANON_NAME.  Free CANON_NAME on
> Jan> 	all the return paths.
> 
> This looks good to me.
> Ok.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-08/msg00009.html


> Jan> I do not push much to get the testcase accepted.
> 
> Is there something in particular you think is wrong with it?  It looks
> ok to me, but your comment makes me wonder what subtlety I missed.

Function under the test is: find_separate_debug_file

Calling paths to find_separate_debug_file are only:

reread_symbols -> reread_separate_symbols -> find_separate_debug_file
	- It should get sooner or later dropped by
		Re: [patch] Replace reread_symbols by load+free calls
		http://sourceware.org/ml/gdb-patches/2009-06/msg00696.html
	  or some similiar patch which is IMO inevitable.
	- This path is currently exploited by the testcase.

symbol_file_add -> symbol_file_add_from_bfd
	- Not exploitable as:
		symbol_file_add -> symfile_bfd_open -> openp -> xfullpath
	  so that objfile->name has already expanded any symbolic links.

symbol_file_add_from_bfd -> symbol_file_add_with_addrs_or_offsets -> find_separate_debug_file
	- Exploitable caller may be symbol_add_stub through solib_add, I will
	  check it later more.

The testcase was written before I found out it is not easily exploitable.
Only now found the last option.  Therefore not checking it now in.


Thanks,
Jan