[RFA] Add $pdir as entry for libthread-db-search-path.

[RFA] Add $pdir as entry for libthread-db-search-path.

[Doug Evans]

Hi.

(1) gdb currently searches for libthread_db in libthread-db-search-path,
the system libthread_db, and finally tries the libthread_db in the
directory of libpthread.
[For completeness sake,
system libthread_db is a bit of a misnomer.  What this really is
is the libthread_db that the ld.so specified in the gdb/gdbserver
binary would find.]
A better default is to look in the directory of libpthread first - that is
the best default.

(2) If the user overrides libthread-db-search-path we want to obey the
user's choice.  Thus, for example, we don't want to hardcode looking
in the directory of libpthread first.

Combining (1) and (2) yields the following patch.

gdbserver has an existing bug where it won't look in the directory
of libpthread, differing in behaviour from gdb.  Blech.
Fixing this requires having gdbserver know where libpthread lives
but that's a bit much for the scope of this patch.
So this patch ignores $pdir in gdbserver.

Tested on amd64-linux with a non-trivial default libthread-db-search-path
and three different libthread_dbs, and cvs head.

Ok to check in?

2011-04-28  Doug Evans  <dje@google.com>

	Support $pdir in libthread-db-search-path.
	* gdb_thread_db.h (LIBTHREAD_DB_SEARCH_PATH): Add $pdir.
	* linux-thread-db.c (try_thread_db_load_from_pdir): New function.
	(thread_db_load_search): Handle $pdir.
	(has_libpthread): New function.
	(thread_db_load): Remove search for libthread_db in directory of
	libpthread - that is now done via $pdir.

	gdbserver/
	* thread-db.c (thread_db_load_search): Ignore $pdir.

	doc/
	* gdb.texinfo (Threads): Document $pdir.
	(Server): Ditto.

Index: gdb_thread_db.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb_thread_db.h,v
retrieving revision 1.16
diff -u -p -r1.16 gdb_thread_db.h
--- gdb_thread_db.h	7 Jan 2011 19:36:17 -0000	1.16
+++ gdb_thread_db.h	29 Apr 2011 03:30:11 -0000
@@ -6,7 +6,7 @@
 #endif
 
 #ifndef LIBTHREAD_DB_SEARCH_PATH
-#define LIBTHREAD_DB_SEARCH_PATH ""
+#define LIBTHREAD_DB_SEARCH_PATH "$pdir"
 #endif
 
 #else
Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.82
diff -u -p -r1.82 linux-thread-db.c
--- linux-thread-db.c	17 Apr 2011 19:11:07 -0000	1.82
+++ linux-thread-db.c	29 Apr 2011 03:30:11 -0000
@@ -639,7 +639,7 @@ dladdr_to_soname (const void *addr)
   return NULL;
 }
 
-/* Attempt to initialize dlopen()ed libthread_db, described by HANDLE.
+/* Attempt to initialize dlopen()ed libthread_db, described by INFO.
    Return 1 on success.
    Failure could happen if libthread_db does not have symbols we expect,
    or when it refuses to work with the current inferior (e.g. due to
@@ -802,6 +802,44 @@ try_thread_db_load (const char *library)
   return 0;
 }
 
+/* Handle $pdir in libthread-db-search-path.
+   Look for libthread_db in the directory of libpthread.  */
+
+static int
+try_thread_db_load_from_pdir (void)
+{
+  struct objfile *obj;
+
+  ALL_OBJFILES (obj)
+    if (libpthread_name_p (obj->name))
+      {
+	char path[PATH_MAX], *cp;
+
+	gdb_assert (strlen (obj->name) < sizeof (path));
+	strcpy (path, obj->name);
+	cp = strrchr (path, '/');
+
+	if (cp == NULL)
+	  {
+	    warning (_("Expected absolute pathname for libpthread in the"
+		       " inferior, but got %s."), path);
+	  }
+	else if (cp + 1 + strlen (LIBTHREAD_DB_SO) + 1 > path + sizeof (path))
+	  {
+	    warning (_("Unexpected: path to libpthread in the inferior is"
+		       " too long: %s"), path);
+	  }
+	else
+	  {
+	    strcpy (cp + 1, LIBTHREAD_DB_SO);
+	    if (try_thread_db_load (path))
+	      return 1;
+	  }
+	return 0;
+      }
+
+  return 0;
+}
 
 /* Search libthread_db_search_path for libthread_db which "agrees"
    to work on current inferior.  */
@@ -850,12 +888,24 @@ thread_db_load_search (void)
 	  memcpy (path, search_path, len + 1);
 	  search_path += len;
 	}
-      strcat (path, "/");
-      strcat (path, LIBTHREAD_DB_SO);
-      if (try_thread_db_load (path))
+
+      if (strcmp (path, "$pdir") == 0)
 	{
-	  rc = 1;
-	  break;
+	  if (try_thread_db_load_from_pdir ())
+	    {
+	      rc = 1;
+	      break;
+	    }
+	}
+      else
+	{
+	  strcat (path, "/");
+	  strcat (path, LIBTHREAD_DB_SO);
+	  if (try_thread_db_load (path))
+	    {
+	      rc = 1;
+	      break;
+	    }
 	}
     }
   if (rc == 0)
@@ -863,13 +913,26 @@ thread_db_load_search (void)
   return rc;
 }
 
+/* Return non-zero if the inferior has a libpthread.  */
+
+static int
+has_libpthread (void)
+{
+  struct objfile *obj;
+
+  ALL_OBJFILES (obj)
+    if (libpthread_name_p (obj->name))
+      return 1;
+
+  return 0;
+}
+
 /* Attempt to load and initialize libthread_db.
    Return 1 on success.  */
 
 static int
 thread_db_load (void)
 {
-  struct objfile *obj;
   struct thread_db_info *info;
 
   info = get_thread_db_info (GET_PID (inferior_ptid));
@@ -889,39 +952,15 @@ thread_db_load (void)
   if (thread_db_load_search ())
     return 1;
 
-  /* None of the libthread_db's on our search path, not the system default
-     ones worked.  If the executable is dynamically linked against
-     libpthread, try loading libthread_db from the same directory.  */
-
-  ALL_OBJFILES (obj)
-    if (libpthread_name_p (obj->name))
-      {
-	char path[PATH_MAX], *cp;
-
-	gdb_assert (strlen (obj->name) < sizeof (path));
-	strcpy (path, obj->name);
-	cp = strrchr (path, '/');
-
-	if (cp == NULL)
-	  {
-	    warning (_("Expected absolute pathname for libpthread in the"
-		       " inferior, but got %s."), path);
-	  }
-	else if (cp + 1 + strlen (LIBTHREAD_DB_SO) + 1 > path + sizeof (path))
-	  {
-	    warning (_("Unexpected: path to libpthread in the inferior is"
-		       " too long: %s"), path);
-	  }
-	else
-	  {
-	    strcpy (cp + 1, LIBTHREAD_DB_SO);
-	    if (try_thread_db_load (path))
-	      return 1;
-	  }
-	warning (_("Unable to find libthread_db matching inferior's thread"
-		   " library, thread debugging will not be available."));
-	return 0;
+  /* We couldn't find a libthread_db.
+     If the inferior has a libpthread warn the user.  */
+  if (has_libpthread ())
+    {
+      warning (_("Unable to find libthread_db matching inferior's thread"
+		 " library, thread debugging will not be available."));
+      return 0;
     }
+
   /* Either this executable isn't using libpthread at all, or it is
      statically linked.  Since we can't easily distinguish these two cases,
      no warning is issued.  */
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.827
diff -u -p -r1.827 gdb.texinfo
--- doc/gdb.texinfo	27 Apr 2011 20:03:04 -0000	1.827
+++ doc/gdb.texinfo	29 Apr 2011 03:30:12 -0000
@@ -2864,8 +2864,12 @@ On @sc{gnu}/Linux and Solaris systems, @
 @code{libthread_db} library to obtain information about threads in the
 inferior process.  @value{GDBN} will use @samp{libthread-db-search-path}
 to find @code{libthread_db}.  If that fails, @value{GDBN} will continue
-with default system shared library directories, and finally the directory
-from which @code{libpthread} was loaded in the inferior process.
+with default system shared library directories.
+
+A special entry @samp{$pdir} for @samp{libthread-db-search-path} is
+supported, and refers to the directory from which @code{libpthread}
+was loaded in the inferior process.  It is normally the first entry
+in the search path.
 
 For any @code{libthread_db} library @value{GDBN} finds in above directories,
 @value{GDBN} attempts to initialize it with the current inferior process.
@@ -16357,6 +16361,9 @@ directories to search for @code{libthrea
 libthread-db-search-path}).  If you omit @var{path},
 @samp{libthread-db-search-path} will be reset to an empty list.
 
+The special entry @samp{$pdir} for @samp{libthread-db-search-path} is
+not supported in @code{gdbserver}.
+
 @item monitor exit
 Tell gdbserver to exit immediately.  This command should be followed by
 @code{disconnect} to close the debugging session.  @code{gdbserver} will
Index: gdbserver/thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/thread-db.c,v
retrieving revision 1.39
diff -u -p -r1.39 thread-db.c
--- gdbserver/thread-db.c	6 Jan 2011 00:14:09 -0000	1.39
+++ gdbserver/thread-db.c	29 Apr 2011 03:30:12 -0000
@@ -743,14 +743,24 @@ thread_db_load_search (void)
 	  memcpy (path, search_path, len + 1);
 	  search_path += len;
 	}
-      strcat (path, "/");
-      strcat (path, LIBTHREAD_DB_SO);
-      if (debug_threads)
-	fprintf (stderr, "thread_db_load_search trying %s\n", path);
-      if (try_thread_db_load (path))
+
+      if (strcmp (path, "$pdir") == 0)
+	{
+	  /* We don't maintain a list of loaded libraries so we don't know
+	     where libpthread lives.  We *could* fetch the info, but we don't
+	     do that yet.  Ignore it.  */
+	}
+      else
 	{
-	  rc = 1;
-	  break;
+	  strcat (path, "/");
+	  strcat (path, LIBTHREAD_DB_SO);
+	  if (debug_threads)
+	    fprintf (stderr, "thread_db_load_search trying %s\n", path);
+	  if (try_thread_db_load (path))
+	    {
+	      rc = 1;
+	      break;
+	    }
 	}
     }
   if (rc == 0)

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Jan Kratochvil]

On Fri, 29 Apr 2011 05:58:37 +0200, Doug Evans wrote:
> A better default is to look in the directory of libpthread first - that is
> the best default.

This is insecure default.  It is something like the FSF GDB insecure .gdbinit
behavior which many distros (at least Fedora but even others) have to patch.

While it is always insecure to run a foreign binary it should be secure to for
example load a foreign binary and its core file.


> (2) If the user overrides libthread-db-search-path we want to obey the
> user's choice.

I agree if the user explicitly wishes so it should be followed.


Thanks,
Jan

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Doug Evans]

On Fri, Apr 29, 2011 at 5:36 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 29 Apr 2011 05:58:37 +0200, Doug Evans wrote:
>> A better default is to look in the directory of libpthread first - that is
>> the best default.
>
> This is insecure default.  It is something like the FSF GDB insecure .gdbinit
> behavior which many distros (at least Fedora but even others) have to patch.

Does Fedora turn off the autoloading of python?
How do your pretty printers Just Work?
[Or maybe you only autoload if the directory is in $prefix/lib/debug
or some such?]

Plus I wonder how easy it would be to build a program that used an
accompanying libpthread that didn't match the system libthread_db -
gdb would then pick the accompanying libthread_db.  Or does Fedora not
ever look in the directory of libpthread for its libthread_db?

> While it is always insecure to run a foreign binary it should be secure to for
> example load a foreign binary and its core file.

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Jan Kratochvil]

On Fri, 29 Apr 2011 18:49:09 +0200, Doug Evans wrote:
> On Fri, Apr 29, 2011 at 5:36 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > This is insecure default.  It is something like the FSF GDB insecure .gdbinit
> > behavior which many distros (at least Fedora but even others) have to patch.
> 
> Does Fedora turn off the autoloading of python?

No.

> How do your pretty printers Just Work?
> [Or maybe you only autoload if the directory is in $prefix/lib/debug
> or some such?]

You are right it is a security hole, I have not tracked to Python autoloading
much.  It should get CVE and security errata assigned as it is the same
category of a security breach as was:
	http://cve.mitre.org/cgi-bin/cvename.cgi?name=2006-4146


> Plus I wonder how easy it would be to build a program that used an
> accompanying libpthread that didn't match the system libthread_db -
> gdb would then pick the accompanying libthread_db.  Or does Fedora not
> ever look in the directory of libpthread for its libthread_db?

This may be also a security exploit I did not catch.


Thanks,
Jan

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Doug Evans]

On Fri, Apr 29, 2011 at 12:00 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Apr 29, 2011 at 10:08 AM, Jan Kratochvil <jan.kratochvil@redhat.com>
> wrote:
>>
>> On Fri, 29 Apr 2011 18:49:09 +0200, Doug Evans wrote:
>> > On Fri, Apr 29, 2011 at 5:36 AM, Jan Kratochvil
>> > <jan.kratochvil@redhat.com> wrote:
>> > > This is insecure default.  It is something like the FSF GDB insecure
>> > > .gdbinit
>> > > behavior which many distros (at least Fedora but even others) have to
>> > > patch.
>> >
>> > Does Fedora turn off the autoloading of python?
>>
>> No.
>>
>> > How do your pretty printers Just Work?
>> > [Or maybe you only autoload if the directory is in $prefix/lib/debug
>> > or some such?]
>>
>> You are right it is a security hole, I have not tracked to Python
>> autoloading
>> much.  It should get CVE and security errata assigned as it is the same
>> category of a security breach as was:
>>        http://cve.mitre.org/cgi-bin/cvename.cgi?name=2006-4146
>>
>>
>> > Plus I wonder how easy it would be to build a program that used an
>> > accompanying libpthread that didn't match the system libthread_db -
>> > gdb would then pick the accompanying libthread_db.  Or does Fedora not
>> > ever look in the directory of libpthread for its libthread_db?
>>
>> This may be also a security exploit I did not catch.
>
> I wonder if gdb needs to record a list of trusted paths.
> btw, is system_gdbinit trustable?

I'd like to keep this patch moving, but I don't know what to do next.

Some thoughts:

1) This is a patch for the FSF tree, not Fedora.
If this kind of security concern is the rule for the FSF tree then I
think it's required to be documented somewhere.
[Maybe it already is and I've missed it?  If not, let's get it documented.]

2) Can we satisfy the security concern by adding to gdb a list of
trusted paths and then everywhere we open a file that can expose a
such a security concern we see if it's on a path on the list?
As for how to handle the case of not being on the list I suppose one
could have a restrictive/permissive mode.
User-written pretty-printers should Just Work - I could argue for
either choice being the default.

Or ... ?

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Jan Kratochvil]

On Sun, 01 May 2011 20:34:02 +0200, Doug Evans wrote:
> 1) This is a patch for the FSF tree, not Fedora.
> If this kind of security concern is the rule for the FSF tree

As both libthread_db and pretty printers have the same attack surface (*) as
	DWARF expression overflow
	http://cve.mitre.org/cgi-bin/cvename.cgi?name=2006-4146
where this CVE lists all public GNU/Linux vendors I do not think such security
requirement is Fedora specific.

(*) That is a foreign binary which is enough to just load into GDB.

OTOH the other attack
	.gdbinit current directory execution
	http://cve.mitre.org/cgi-bin/cvename.cgi?name=2005-1705
also lists multiple GNU/Linux vendors and the issue is not yet fixed in FSF
GDB.  But this is IMO just still work in prograss / unfinished, not rejected:
	[RFA] .gdbinit security (revived) [incl doc]
	http://sourceware.org/ml/gdb-patches/2010-11/msg00276.html


Thanks,
Jan

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Doug Evans]

On Mon, May 2, 2011 at 12:14 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Sun, 01 May 2011 20:34:02 +0200, Doug Evans wrote:
>> 1) This is a patch for the FSF tree, not Fedora.
>> If this kind of security concern is the rule for the FSF tree
>
> As both libthread_db and pretty printers have the same attack surface (*) as
>        DWARF expression overflow
>        http://cve.mitre.org/cgi-bin/cvename.cgi?name=2006-4146
> where this CVE lists all public GNU/Linux vendors I do not think such security
> requirement is Fedora specific.
>
> (*) That is a foreign binary which is enough to just load into GDB.
>
> OTOH the other attack
>        .gdbinit current directory execution
>        http://cve.mitre.org/cgi-bin/cvename.cgi?name=2005-1705
> also lists multiple GNU/Linux vendors and the issue is not yet fixed in FSF
> GDB.  But this is IMO just still work in prograss / unfinished, not rejected:
>        [RFA] .gdbinit security (revived) [incl doc]
>        http://sourceware.org/ml/gdb-patches/2010-11/msg00276.html

Thanks, but I'm still stuck ...

Question for the group at large (and I it doesn't matter to me which
way we go, I just want to make forward progress ...).
Do we enforce such security concerns in FSF gdb?
And if so, let's get these issues documented (I have a pet peeve
regarding rules/issues that aren't written down).
I see some things are documented (grep for security in gdb.texinfo)
and we do have "remote system-call-allowed", but there's not yet any
mention of libthread_db or autoloading of python code (a quick scan of
the bugzilla didn't reveal anything).

Second,
If we address these security concerns what is the solution?
One proposal is on the table.
[Maintain a list of trusted paths in gdb and have a flag for
permissive/restrictive mode.
If in restrictive mode libthread_db and autoloaded python/gdbinit code
has to come from a trusted path.
I think one could take this further though.]

Last,
Do we need to address this before adding my $pdir patch?

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> Thanks, but I'm still stuck ...

I have gone back and forth on this a few times.

On the one hand, I think people running gdb on an untrusted executable
are acting naively.  I think this is true even for a python-less build
using -nx -- I just don't think gdb or bfd has had enough scrutiny along
these lines to warrant trust.

On the other hand, I think it makes sense to aim for trustworthiness as
a goal, because gdb is a powerful tool for inspecting executables.

I think my overall preference would be for gdb to run securely by
default, with some runtime settings to let users override this.

Also I don't have any problem recognizing that different organizations
build gdb in different ways for their own reasons, and making
accommodations for that.  That is, a configure option to make $pdir the
default seems fine to me, if you want something like that.

Doug> Question for the group at large (and I it doesn't matter to me which
Doug> way we go, I just want to make forward progress ...).
Doug> Do we enforce such security concerns in FSF gdb?

IMO, yes.

Doug> Second,
Doug> If we address these security concerns what is the solution?
Doug> One proposal is on the table.
Doug> [Maintain a list of trusted paths in gdb and have a flag for
Doug> permissive/restrictive mode.
Doug> If in restrictive mode libthread_db and autoloaded python/gdbinit code
Doug> has to come from a trusted path.
Doug> I think one could take this further though.]

It seems reasonable to me.

Doug> Last,
Doug> Do we need to address this before adding my $pdir patch?

IMO, no, but it would be nicer that way.

Tom

Re: [RFA] Add $pdir as entry for libthread-db-search-path.

[Doug Evans]

On Fri, May 6, 2011 at 11:40 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> Thanks, but I'm still stuck ...
>
> I have gone back and forth on this a few times.
>
> On the one hand, I think people running gdb on an untrusted executable
> are acting naively.  I think this is true even for a python-less build
> using -nx -- I just don't think gdb or bfd has had enough scrutiny along
> these lines to warrant trust.
>
> On the other hand, I think it makes sense to aim for trustworthiness as
> a goal, because gdb is a powerful tool for inspecting executables.
>
> I think my overall preference would be for gdb to run securely by
> default, with some runtime settings to let users override this.
>
> Also I don't have any problem recognizing that different organizations
> build gdb in different ways for their own reasons, and making
> accommodations for that.  That is, a configure option to make $pdir the
> default seems fine to me, if you want something like that.
>
> Doug> Question for the group at large (and I it doesn't matter to me which
> Doug> way we go, I just want to make forward progress ...).
> Doug> Do we enforce such security concerns in FSF gdb?
>
> IMO, yes.
>
> Doug> Second,
> Doug> If we address these security concerns what is the solution?
> Doug> One proposal is on the table.
> Doug> [Maintain a list of trusted paths in gdb and have a flag for
> Doug> permissive/restrictive mode.
> Doug> If in restrictive mode libthread_db and autoloaded python/gdbinit code
> Doug> has to come from a trusted path.
> Doug> I think one could take this further though.]
>
> It seems reasonable to me.
>
> Doug> Last,
> Doug> Do we need to address this before adding my $pdir patch?
>
> IMO, no, but it would be nicer that way.

If I also add $sdir to specify a plain dlopen (LIBTHREAD_DB) and put
that ahead of $pdir ("s" for system), then I can have $pdir and not
change the current behaviour (though I still think $pdir should come
first - we can move it first after whatever security model is added).