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

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

[Keith Seitz]

[-- 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}

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

[Jan Kratochvil]

On Sat, 20 Nov 2010 00:06:15 +0100, Keith Seitz wrote:
> --- 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);

./python/py-auto-load.c: In function ‘source_section_scripts’:
./python/py-auto-load.c:222:10: error: too few arguments to function ‘find_and_open_script’
./cli/cli-cmds.h:128:12: note: declared here


> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ testsuite/gdb.base/gdbinit.exp	19 Nov 2010 22:38:55 -0000

> +global GDB
> +global GDBFLAGS
> +global gdb_prompt
> +global gdb_spawn_id;

Redundant.


> +set env(HOME) [pwd]

Some other env vars from default_gdb_start are missing there.

At least TERM will fix up gdb.log:
	-^[[?1034hGNU gdb (GDB) 7.2.50.20101120-cvs^M
	+GNU gdb (GDB) 7.2.50.20101120-cvs^M

And there were some reason even for the others:
	set env(LC_CTYPE) C
	set env(INPUTRC) "/dev/null"
	set env(TERM) "vt100"


> +remote_exec build "rm .gdbinit"

Shouldn't be `rm -f' here?  (Also for `cp' and `chmod' below.)

This way it deletes src-shipped `gdb/testsuite/.gdbinit' when you run the test
in the src tree.


> +remote_exec build "cp $srcdir/$subdir/gdbinit.sample .gdbinit"


> +gdb_expect 360 {

I would prefer
	gdb_test_multiple "" testname
(and thus also dropping the "timeout" case)

> +    -re "warning: file .*\.gdbinit.* is untrusted.*Read file anyway\?.*" {

Backslashes should be doubled here in "..." to have the desired effect (and not
considered as a regex metacharacter).


> +gdb_expect 360 {

gdb_test_multiple again.


> +    -re "warning: file.*\.gdbinit.* is untrusted.*Read file anyway\?.*" {

Backslashes again.


> +        fail "trusted .gdbinit allowed."
> +    }
> +    -re ".*reading gdbinit.*$gdb_prompt $"     {

Redundant leading `.*'.



Thanks,
Jan

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

[Eli Zaretskii]

> Date: Fri, 19 Nov 2010 15:06:15 -0800
> From: Keith Seitz <keiths@redhat.com>
> 
> 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.

Thanks.

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?

I'm also unsure whether we should disregard this issue on Windows.  If
it's important to make sure .gdbinit is safe, it should also be
important to do that on Windows (using the NTFS file security calls).

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.

> +	  warning (_("file \"%s\" is untrusted"), file);

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.

> +	  if (!query (_("Read file anyway? ")))

This could be automatically answered YES in some situations.  Do we
care?

> +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.

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), ...

The doco part is OK with those changes.

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

[Keith Seitz]

On 11/19/2010 06:50 PM, Jan Kratochvil wrote:

> ./python/py-auto-load.c: In function ‘source_section_scripts’:
> ./python/py-auto-load.c:222:10: error: too few arguments to function ‘find_and_open_script’
> ./cli/cli-cmds.h:128:12: note: declared here

Sorry, I actually have that in my patchset, I just forgot to include it
in the submission.

I've addressed all of your issues in the testsuite ("-f" and
gdb_test_multiple). I will attach a revised version of this in reply
Eli's message.

Thank you for the review,
Keith

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

[Keith Seitz]

[-- 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}

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

[Eli Zaretskii]

> Date: Tue, 23 Nov 2010 10:26:31 -0800
> From: Keith Seitz <keiths@redhat.com>
> 
> 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).

Thanks.  I'm happy with the doco part.

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

[Doug Evans]

On Tue, Nov 23, 2010 at 10:26 AM, Keith Seitz <keiths@redhat.com> wrote:
> [...]
> 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.

Hi.
A few comments inline.

>-    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.
Feel free to save for a separate patch.  Just mentioning it to prime
the pumps doing something like this down the road.

>+   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).

We're combining two concepts here: "is the command from the tty?" and
"do security checks?".
IWBN to keep them separate here.
Maybe specify both separately or just have check_security instead of from_tty?

>+	  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.
Then again, why not do this security check for system.gdbinit too?

>       opened = find_and_open_script (file, 1 /*search_path*/,
>-				     &stream, &full_path);
>+				     &stream, &full_path, 1 /* from_tty */);

Passing 1 for from_tty feels wrong here.
If find_and_open_script had a check_security parameter instead of
from_tty, then one could just pass 0 here.

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

[Jan Kratochvil]

On Tue, 23 Nov 2010 19:26:31 +0100, Keith Seitz wrote:
> +      /* If our group ID matches the file, read it in
> +	 without warning/querying the user.  */
> +#ifdef HAVE_GETGID
> +      if (statbuf.st_gid != getgid ())
> +#endif

So currently on `chmod 666 .gdbinit' it no longer traps...

BTW do people really need this feature?  I always use my alias gdbn to force
-nx to not load any .gdbinit, as I generally do not expect which project which
way changes my GDB behavior.

src/gdb/.gdbinit is the most invasive one, it breaks GDB completely. :-)

Other projects incl. bash could also use such `$PWD/.*rc' reading policies.
They do not.


Thanks,
Jan

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

[Keith Seitz]

On 11/24/2010 01:23 PM, Jan Kratochvil wrote:

> So currently on `chmod 666 .gdbinit' it no longer traps...

That's what (I interpreted?) Andreas as suggesting.

> BTW do people really need this feature?  I always use my alias gdbn to force
> -nx to not load any .gdbinit, as I generally do not expect which project which
> way changes my GDB behavior.

I have no idea, really. I'm just trying to upstream a Fedora patch. From 
the original discussion, I gather that several linux distros are 
carrying similar patches. I presume there is a reason for it. [Was there 
a CERT alert that was published?]

In any case, I'm rethinking a bunch of this... Stay tuned.

Keith

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

[Keith Seitz]

[-- 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}