From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9222 invoked by alias); 30 Nov 2010 00:23:21 -0000 Received: (qmail 9199 invoked by uid 22791); 30 Nov 2010 00:23:16 -0000 X-SWARE-Spam-Status: No, hits=-5.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 30 Nov 2010 00:23:08 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oAU0N4Ug008234 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 29 Nov 2010 19:23:04 -0500 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id oAU0N17c002161 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Mon, 29 Nov 2010 19:23:03 -0500 Message-ID: <4CF442CA.5020909@redhat.com> Date: Tue, 30 Nov 2010 00:23:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9) Gecko/20100921 Fedora/3.1.4-1.fc13 Lightning/1.0b3pre Thunderbird/3.1.4 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches@sourceware.org Subject: Re: [RFA] .gdbinit security (revived) [incl doc] References: <4CE702E7.4050504@redhat.com> <83d3q0babs.fsf@gnu.org> <4CEC0757.6000503@redhat.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------040901010309060801030501" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-11/txt/msg00488.txt.bz2 This is a multi-part message in MIME format. --------------040901010309060801030501 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 4152 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 Based on work from Daniel Jacobowitz and Jeff Johnston : * 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 * gdb.texinfo (Startup): Document security handling of .gdbinit files. --------------040901010309060801030501 Content-Type: text/plain; name="gdbinit-security-3.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gdbinit-security-3.patch" Content-length: 7971 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} --------------040901010309060801030501--