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

Keith Seitz
2010-11-19T23:10:09Z

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