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

[Keith Seitz <keiths@redhat.com>]

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

Hi, Doug,

On 11/23/2010 03:18 PM, Doug Evans wrote:
>> -    catch_command_errors (source_script, home_gdbinit, 0, RETURN_MASK_ALL);
>> +    catch_command_errors (source_script, home_gdbinit, -1, RETURN_MASK_ALL);
>
> I don't mind using -1 for from_tty here  (especially if there is
> precedent :-)), but a #define/enum would be nicer.
> catch_command_errors has a limited API so overloading from_tty is a
> pragmatic tradeoff.
[snip]
> Maybe specify both separately or just have check_security instead of
> from_tty?

Actually, I've changed this a little bit on reflection. I've added a new 
wrapper which will call find_and_open_script with parameter 
"security_check" set. So I'm not overloading the use of from_tty at all; 
it can stay a simple "boolean."

>
>> +	  if (statbuf.st_uid != getuid ())
>
> I wonder if you also need to watch for file owner == root (and not
> world writable).  E.g. scripts like --with-system-gdbinit.
> That won't happen with the patch as is, but that feels like a
> high-level detail that this function shouldn't have to know about.

Yeah, this is the problem: we're starting to get into a lot of detail 
here, and, like Daniel, I didn't really want to dig myself a hole on 
security issues. I'm far from an expert on that. Heck, I'm probably not 
too far from novice!

IMO, it is a delicate balancing act between adding a bunch of 
site-specific knowledge [What if the system-wide gdbinit was not 
installed by root, but by some other user/group?] and maintenance 
[Should we add configure options for which group/user to implicitly 
trust?]. It seems like it could all easily get out of control.

If a policy can be decided on, I am, of course, happy to follow it through.

Here are my thoughts on it. The goal is to prevent a gdbinit (or ANY 
script that may be automatically read) from automatically executing if 
it could possibly contain malicious commands.

"If it could possibly contain malicious commands" means (to me), "Can 
someone other than the user write to the file?" If so, gdb should warn 
and query the user. [I don't pretend to prevent stupid users from doing 
stupid things. It's a debugger; I like to think that all our users are 
intelligent people.]

Trying to formalize, I think this is:
1) If the script is world-writable --> warn/query the user
2) If the script is group-writable --> warn/query
3) If the script is not owned by you or root --> warn/query

Now I can accept an argument that #2 should be dropped, but for the sake 
of discussion, I've kept it in the attached patch.

> Then again, why not do this security check for system.gdbinit too?

My only guess is that this is presumed "safe," since I suppose a trusted 
source installed/created that. But that is my only guess. I've included 
this check FWIW.

Full disclosure: There are two (or perhaps more?) other places where a 
security check should be performed that I haven't attempted to implement:
1) when sourcing a file from .gdbinit
2) in python autoloading?

Those are a little trickier and something not addressed by any of the 
patches out in the wild.

Comments?
Keith

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

	Based on work from  Daniel Jacobowitz  <dan@codesourcery.com>
	and Jeff Johnston  <jjohnstn@redhat.com>:
	* cli/cli-cmds.h (source_script_with_security_check): New
	function.
	* cli/cli-cmds.c (source_script_with_security_check): Likewise.
	(find_and_open_script): Add SECURITY_CHECK parameter.
	Implement a basic security check of the script file before
	executing it.
	(source_script_with_search): Add SECURITY_CHECK parameter and
	pass it to find_and_open_script.
	(source_script): Update call to find_and_open_script, performing
	no security check of the file.
	(source_command): Likewise.
	(source_script_with_security_check): New function.
	* main.c (captured_main): When reading init files, use
	source_script_with_security_check.
	* python/py-auto-load.c (source_section_scripts): Update call
	to find_and_open_script, performing no security check.

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

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

[-- Attachment #2: gdbinit-security-3.patch --]
[-- Type: text/plain, Size: 7971 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	29 Nov 2010 23:59:25 -0000
@@ -123,10 +123,13 @@ extern void quit_command (char *, int);
 
 extern void source_script (char *, int);
 
+extern void source_script_with_security_check (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 flags);
 
 /* 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	29 Nov 2010 23:59:25 -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,18 @@ 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 SECURITY_CHECK is non-zero, then this script is subject
+   to a security check (supported only on hosts with HAVE_GETUID).
+
+   TODO: Platforms without HAVE_GETUID (most notably Windows) are still
+   susceptible to executing untrusted script files until an appropriate
+   permissions check can be performed.  */
 
 int
 find_and_open_script (const char *script_file, int search_path,
-		      FILE **streamp, char **full_pathp)
+		      FILE **streamp, char **full_pathp, int security_check)
 {
   char *file;
   int fd;
@@ -513,6 +521,54 @@ find_and_open_script (const char *script
       return 0;
     }
 
+#ifdef HAVE_GETUID
+  /* The filesystem persmissions have already been applied above, i.e.,
+     if we get this far, the file is readable by the user.  */
+  if (security_check)
+    {
+      int ask = 0;
+      struct stat statbuf;
+
+      if (fstat (fd, &statbuf) < 0)
+	{
+	  int save_errno = errno;
+	  close (fd);
+	  do_cleanups (old_cleanups);
+	  errno = save_errno;
+	  return 0;
+	}
+
+      /* Warn/query if the file is world-writable.  */
+      if (statbuf.st_mode & S_IWOTH)
+	{
+	  warning (_("file \"%s\" is world-writable"), file);
+	  ask = 1;
+	}
+      /* Warn/query if the file is group-writable.  */
+      else if (statbuf.st_mode & S_IWGRP)
+	{
+	  warning (_("file \"%s\" is group-writable"), file);
+	  ask = 1;
+	}
+      /* Warn/query if the user (or superuser) is not the owner of the
+	 file.  */
+      else if (statbuf.st_uid != 0 && statbuf.st_uid != getuid ())
+	{
+	  warning (_("file \"%s\" is owned by another user"), file);
+	  ask = 1;
+	}
+
+      /* FILE gets freed by do_cleanups (old_cleanups).  */
+      if (ask && !nquery (_("Read file anyway? ")))
+	{
+	  close (fd);
+	  do_cleanups (old_cleanups);
+	  errno = EPERM;
+	  return 0;
+	}
+    }
+#endif
+
   do_cleanups (old_cleanups);
 
   *streamp = fdopen (fd, FOPEN_RT);
@@ -560,10 +616,14 @@ source_script_from_stream (FILE *stream,
 /* Worker to perform the "source" command.
    Load script FILE.
    If SEARCH_PATH is non-zero, and the file isn't found in cwd,
-   search for it in the source search path.  */
+   search for it in the source search path.
+
+   If SECURITY_CHECK is non-zero, a security check will be performed
+   on the file (in find_and_open_script).  */
 
 static void
-source_script_with_search (const char *file, int from_tty, int search_path)
+source_script_with_search (const char *file, int from_tty, int search_path,
+			   int security_check)
 {
   FILE *stream;
   char *full_path;
@@ -572,7 +632,8 @@ 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,
+			     security_check))
     {
       /* The script wasn't found, or was otherwise inaccessible.
 	 If the source command was invoked interactively, throw an error.
@@ -595,7 +656,16 @@ source_script_with_search (const char *f
 void
 source_script (char *file, int from_tty)
 {
-  source_script_with_search (file, from_tty, 0);
+  source_script_with_search (file, from_tty, 0, 0);
+}
+
+/* Another wrapper around source_script_with_search which will
+   cause a security check on the script file before executing it.  */
+
+void
+source_script_with_security_check (char *file, int from_tty)
+{
+  source_script_with_search (file, from_tty, 0, 1);
 }
 
 /* Return the source_verbose global variable to its previous state
@@ -658,7 +728,7 @@ source_command (char *args, int from_tty
       file = args;
     }
 
-  source_script_with_search (file, from_tty, search_path);
+  source_script_with_search (file, from_tty, search_path, 0);
 
   do_cleanups (old_cleanups);
 }
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	29 Nov 2010 23:59:25 -0000
@@ -796,7 +796,8 @@ 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_with_security_check, home_gdbinit,
+			  0, RETURN_MASK_ALL);
 
   /* Now perform all the actions indicated by the arguments.  */
   if (cdarg != NULL)
@@ -870,7 +871,8 @@ 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_with_security_check, local_gdbinit,
+			  0, 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: python/py-auto-load.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-auto-load.c,v
retrieving revision 1.5
diff -u -p -r1.5 py-auto-load.c
--- python/py-auto-load.c	22 Sep 2010 20:00:53 -0000	1.5
+++ python/py-auto-load.c	29 Nov 2010 23:59:25 -0000
@@ -219,7 +219,8 @@ source_section_scripts (struct objfile *
 	}
 
       opened = find_and_open_script (file, 1 /*search_path*/,
-				     &stream, &full_path);
+				     &stream, &full_path,
+				     0 /* security_check */);
 
       /* If the file is not found, we still record the file in the hash table,
 	 we only want to print an error message once.
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.778
diff -u -p -r1.778 gdb.texinfo
--- doc/gdb.texinfo	29 Nov 2010 23:20:57 -0000	1.778
+++ doc/gdb.texinfo	29 Nov 2010 23:59:34 -0000
@@ -1286,6 +1286,10 @@ 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.
 
+On some platorms, @value{GDBN} will perform a security check of @file{.gdbinit}
+before it is executed.  If @file{.gdbinit} is not owned by the current user
+or the superuser, or the file is either group- or world-writable,
+@value{GDBN} will warn the user and ask if the file should be read anyway.
 
 @node Quitting GDB
 @section Quitting @value{GDBN}