[RFA] .gdbinit security (revived) [incl doc]

[Keith Seitz <keiths@redhat.com>]

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

Hi,

A long time ago, Daniel posted a patch which would do a security check 
of .gdbinit files and refuse to execute them if they were untrusted. See 
http://sourceware.org/ml/gdb-patches/2005-05/msg00637.html . I would 
like to resurrect that discussion.

At the time, there was some debate about whether simply refusing to read 
the file was particularly user-unfriendly for a lot of developers. 
Someone suggested adding an option to override the behavior and so on. 
Overall, people agreed that doing something was correct.

I have implemented a slightly different option: ask the user if he would 
like to run the untrusted file any way, much like removing a 
write-protected file IMO.

Fedora has been using a version of this patch (essentially Daniel's 
original patch) for several years, and I'm sure that other distros have 
their own versions, too.

No regressions on x86_64-linux. [mingw32 does not appear to have getuid. 
It builds without HAVE_GETUID.]

Comments?
Keith

ChangeLog
2010-11-19  Keith Seitz  <keiths@redhat.com>

	From Daniel Jacobowitz  <dan@codesourcery.com>
	and Jeff Johnston  <jjohnstn@redhat.com>:
	* cli/cli-cmds.h (find_and_open_script): Add from_tty argument.
	* cli/cli-cmds.c (find_and_open_script): Likewise.  When
	from_tty is -1, perform a security check of the file.  If it
	fails, warn the user and whether he wants to read the file anyway.
	(source_script_with_search): Update call find_and_open_script.
	Only print an error if from_tty is greater than zero.
	* main.c (captured_main): Pass from_tty = -1 when sourcing
	gdbinit files.

testsuite/ChangeLog
2010-11-19  Keith Seitz <keiths@redhat.com>

	From Jeff Johnston  <jjohnstn@redhat.com>:
	* gdb.base/gdbinit.sample: New file.
	* gdb.base/gdbinit.exp: New file.


doc/ChangeLog
2010-11-19  Keith Seitz  <keiths@redhat.com>

	* gdb.texinfo (Startup): Document security handling of
	.gdbinit files.


[-- Attachment #2: gdbinit-security.patch --]
[-- Type: text/plain, Size: 8034 bytes --]

Index: cli/cli-cmds.h
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-cmds.h,v
retrieving revision 1.16
diff -u -p -r1.16 cli-cmds.h
--- cli/cli-cmds.h	2 May 2010 23:52:14 -0000	1.16
+++ cli/cli-cmds.h	19 Nov 2010 22:38:54 -0000
@@ -126,7 +126,8 @@ extern void source_script (char *, int);
 /* Exported to objfiles.c.  */
 
 extern int find_and_open_script (const char *file, int search_path,
-				 FILE **streamp, char **full_path);
+				 FILE **streamp, char **full_path,
+				 int from_tty);
 
 /* Command tracing state.  */
 
Index: cli/cli-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-cmds.c,v
retrieving revision 1.105
diff -u -p -r1.105 cli-cmds.c
--- cli/cli-cmds.c	27 Jul 2010 20:33:40 -0000	1.105
+++ cli/cli-cmds.c	19 Nov 2010 22:38:54 -0000
@@ -39,6 +39,7 @@
 #include "source.h"
 #include "disasm.h"
 #include "tracepoint.h"
+#include "gdb_stat.h"
 
 #include "ui-out.h"
 
@@ -483,11 +484,15 @@ Script filename extension recognition is
    search for it in the source search path.
 
    NOTE: This calls openp which uses xfullpath to compute the full path
-   instead of gdb_realpath.  Symbolic links are not resolved.  */
+   instead of gdb_realpath.  Symbolic links are not resolved.
+
+   If FROM_TTY is -1, then this script is being automatically loaded
+   at runtime, and a security check will be performed on the file
+   (supported only on hosts with HAVE_GETUID).  */
 
 int
 find_and_open_script (const char *script_file, int search_path,
-		      FILE **streamp, char **full_pathp)
+		      FILE **streamp, char **full_pathp, int from_tty)
 {
   char *file;
   int fd;
@@ -513,6 +518,35 @@ find_and_open_script (const char *script
       return 0;
     }
 
+#ifdef HAVE_GETUID
+  if (from_tty == -1)
+    {
+      struct stat statbuf;
+
+      if (fstat (fd, &statbuf) < 0)
+	{
+	  int save_errno = errno;
+
+	  close (fd);
+	  do_cleanups (old_cleanups);
+	  errno = save_errno;
+	  return 0;
+	}
+      if (statbuf.st_uid != getuid () || (statbuf.st_mode & S_IWOTH))
+	{
+	  /* FILE gets freed by do_cleanups (old_cleanups).  */
+	  warning (_("file \"%s\" is untrusted"), file);
+	  if (!query (_("Read file anyway? ")))
+	    {
+	      close (fd);
+	      do_cleanups (old_cleanups);
+	      errno = EPERM;
+	      return 0;
+	    }
+	}
+    }
+#endif
+
   do_cleanups (old_cleanups);
 
   *streamp = fdopen (fd, FOPEN_RT);
@@ -572,13 +606,14 @@ source_script_with_search (const char *f
   if (file == NULL || *file == 0)
     error (_("source command requires file name of file to source."));
 
-  if (!find_and_open_script (file, search_path, &stream, &full_path))
+  if (!find_and_open_script (file, search_path, &stream, &full_path,
+			     from_tty))
     {
       /* The script wasn't found, or was otherwise inaccessible.
 	 If the source command was invoked interactively, throw an error.
 	 Otherwise (e.g. if it was invoked by a script), silently ignore
 	 the error.  */
-      if (from_tty)
+      if (from_tty > 0)
 	perror_with_name (file);
       else
 	return;
Index: main.c
===================================================================
RCS file: /cvs/src/src/gdb/main.c,v
retrieving revision 1.87
diff -u -p -r1.87 main.c
--- main.c	22 Sep 2010 19:59:15 -0000	1.87
+++ main.c	19 Nov 2010 22:38:55 -0000
@@ -796,7 +796,7 @@ Excess command line arguments ignored. (
      debugging or what directory you are in.  */
 
   if (home_gdbinit && !inhibit_gdbinit)
-    catch_command_errors (source_script, home_gdbinit, 0, RETURN_MASK_ALL);
+    catch_command_errors (source_script, home_gdbinit, -1, RETURN_MASK_ALL);
 
   /* Now perform all the actions indicated by the arguments.  */
   if (cdarg != NULL)
@@ -870,7 +870,7 @@ Can't attach to process and specify a co
   /* Read the .gdbinit file in the current directory, *if* it isn't
      the same as the $HOME/.gdbinit file (it should exist, also).  */
   if (local_gdbinit && !inhibit_gdbinit)
-    catch_command_errors (source_script, local_gdbinit, 0, RETURN_MASK_ALL);
+    catch_command_errors (source_script, local_gdbinit, -1, RETURN_MASK_ALL);
 
   /* Now that all .gdbinit's have been read and all -d options have been
      processed, we can read any scripts mentioned in SYMARG.
Index: testsuite/gdb.base/gdbinit.sample
===================================================================
RCS file: testsuite/gdb.base/gdbinit.sample
diff -N testsuite/gdb.base/gdbinit.sample
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/gdbinit.sample	19 Nov 2010 22:38:55 -0000
@@ -0,0 +1 @@
+echo \nreading gdbinit\n
Index: testsuite/gdb.base/gdbinit.exp
===================================================================
RCS file: testsuite/gdb.base/gdbinit.exp
diff -N testsuite/gdb.base/gdbinit.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/gdbinit.exp	19 Nov 2010 22:38:55 -0000
@@ -0,0 +1,86 @@
+#   Copyright 2005, 2010
+#   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/>.  */
+
+# This file was written by Jeff Johnston <jjohnstn@redhat.com>.
+
+# Skip this test if the target is remote.
+if [is_remote target] {
+    return
+}
+
+global GDB
+global GDBFLAGS
+global gdb_prompt
+global gdb_spawn_id;
+
+gdb_exit
+
+gdb_stop_suppressing_tests
+verbose "Spawning $GDB -nw"
+if {[info exists gdb_spawn_id]} {
+    return 0
+}
+
+if {![is_remote host]} {
+   if {[which $GDB] == 0} {
+        perror "$GDB does not exist."
+        exit 1
+    }
+}
+
+set env(HOME) [pwd]
+remote_exec build "rm .gdbinit"
+remote_exec build "cp $srcdir/$subdir/gdbinit.sample .gdbinit"
+remote_exec build "chmod 646 .gdbinit"
+
+gdb_exit
+set res [remote_spawn host "$GDB -nw [host_info gdb_opts]"];
+if { $res < 0 || $res == "" } {
+    perror "Spawning $GDB failed."
+    return 1;
+}
+gdb_expect 360 {
+    -re "warning: file .*\.gdbinit.* is untrusted.*Read file anyway\?.*" {
+	gdb_test "y" ".*reading gdbinit.*" "read untrusted file"
+    }
+    -re ".*reading gdbinit.*$gdb_prompt $" {
+	fail "untrusted .gdbinit caught"
+    }
+    timeout {
+        fail "(timeout) untrusted .gdbinit caught"
+    }
+}
+
+remote_exec build "chmod 644 .gdbinit"
+gdb_exit
+set res [remote_spawn host "$GDB -nw [host_info gdb_opts]"];
+if { $res < 0 || $res == "" } {
+    perror "Spawning $GDB failed."
+    return 1;
+}
+gdb_expect 360 {
+    -re "warning: file.*\.gdbinit.* is untrusted.*Read file anyway\?.*" {
+        fail "trusted .gdbinit allowed."
+    }
+    -re ".*reading gdbinit.*$gdb_prompt $"     {
+        pass "trusted .gdbinit allowed."
+    }
+    timeout {
+        fail "(timeout) trusted .gdbinit allowed."
+    }
+}
+
+remote_exec build "rm .gdbinit"
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.775
diff -u -p -r1.775 gdb.texinfo
--- doc/gdb.texinfo	12 Nov 2010 20:49:41 -0000	1.775
+++ doc/gdb.texinfo	19 Nov 2010 22:39:04 -0000
@@ -1280,6 +1280,9 @@ ports of @value{GDBN} use the standard n
 @file{gdb.ini} file, they warn you about that and suggest to rename
 the file to the standard name.
 
+If @file{.gdbinit} is untrusted (it is not owned by the current user
+or the file is world-writable), @value{GDBN} will warn the user and ask
+if the file should be read anyway.
 
 @node Quitting GDB
 @section Quitting @value{GDBN}