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

[Keith Seitz <keiths@redhat.com>]

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

Hi, Eli,

On 11/20/2010 01:42 AM, Eli Zaretskii wrote:
> In that discussion, Andreas suggested to avoid the warning if the user
> belongs to the same group as the file's owner.  I don't see your patch
> addressing that part.  Why not?

It was not clear to me that this was generally considered desirable. 
Daniel noted, "I'm trying not to encode too much site policy
into GDB".

I can add this, but to be honest, I'm no longer certain how to write a 
test for this, since it is not possible (AFAIK) to change permissions of 
a file in the test suite that would cause this to trigger.

> I realize that it would be inappropriate to ask you to do that as a
> prerequisite for accepting the patch, but maybe a TODO comment should
> be placed there about the Windows case.  Then someone else could do
> that at some point.

Done.

> I would suggest to spell out why it is untrusted.  Otherwise the
> warning sounds grave, but doesn't give enough information to make the
> decision.

Done.

>
>> +	  if (!query (_("Read file anyway? ")))
>
> This could be automatically answered YES in some situations.  Do we
> care?

Good question. I always seem to forget that the user can do this. To err 
on the side of safety, I've changed to using nquery.

>> +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
>
> This should be qualified by "on some platforms", because not every
> platform that supports file ownership will issue this warning.

Yes, indeed. Fixed.

> And a minor stylistic issue.  You say "it is not owned" and then "the
> file is world-writable".  This is inconsistent, and could confuse the
> reader into thinking that "it" and "the file" are two different
> things.  Suggest to rephrase:
>
>    If @file{.gdbinit} is @dfn{untrusted} (either not owned by the
>    current user or world-writable), ...

You are absolutely correct. I've added a bit about the group ID addition 
and reworded this to help readability (I hope):

"On some platorms, @value{GDBN} will perform security check of
@file{.gdbinit} before it is executed.  If @file{.gdbinit} 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.  These warnings 
are suppressed when the group ID of the file's owner matches the group 
ID of the user."

I've attached an updated version of the patch (without the test case, 
which I don't think can work without some sort of administrative 
permissions).

Keith

ChangeLog
2010-11-23  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 to 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.
	* python/py-auto-load.c (source_section_scripts): Update call
	to find_and_open_script.

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

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

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

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	23 Nov 2010 17:34:33 -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: 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	23 Nov 2010 17:34:34 -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,19 @@ 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).
+
+   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 from_tty)
 {
   char *file;
   int fd;
@@ -513,6 +522,51 @@ 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 our group ID matches the file, read it in
+	 without warning/querying the user.  */
+#ifdef HAVE_GETGID
+      if (statbuf.st_gid != getgid ())
+#endif
+	{
+	  int ask = 0;
+
+	  if (statbuf.st_uid != getuid ())
+	    {
+	      warning (_("file \"%s\" is not owned by you"), file);
+	      ask = 1;
+	    }
+	  else if (statbuf.st_mode & S_IWOTH)
+	    {
+	      warning (_("file \"%s\" is world-writable"), 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);
@@ -572,13 +626,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: 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	23 Nov 2010 17:34:34 -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: 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	23 Nov 2010 17:34:34 -0000
@@ -219,7 +219,7 @@ source_section_scripts (struct objfile *
 	}
 
       opened = find_and_open_script (file, 1 /*search_path*/,
-				     &stream, &full_path);
+				     &stream, &full_path, 1 /* from_tty */);
 
       /* 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.776
diff -u -p -r1.776 gdb.texinfo
--- doc/gdb.texinfo	23 Nov 2010 14:39:16 -0000	1.776
+++ doc/gdb.texinfo	23 Nov 2010 17:34:43 -0000
@@ -1286,6 +1286,11 @@ 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 security check of @file{.gdbinit}
+before it is executed.  If @file{.gdbinit} 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.  These warnings are suppressed when the
+group ID of the file's owner matches the group ID of the user.
 
 @node Quitting GDB
 @section Quitting @value{GDBN}