From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85378 invoked by alias); 26 Jul 2015 19:26:47 -0000 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 Received: (qmail 85359 invoked by uid 89); 26 Jul 2015 19:26:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sun, 26 Jul 2015 19:26:45 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 68A0714AA7; Sun, 26 Jul 2015 19:26:44 +0000 (UTC) Received: from localhost (unused-10-15-17-51.yyz.redhat.com [10.15.17.51]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6QJQheH001619 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sun, 26 Jul 2015 15:26:43 -0400 From: Sergio Durigan Junior To: Doug Evans Cc: GDB Patches , Eli Zaretskii Subject: Re: [PATCH v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command References: <1437761993-18758-1-git-send-email-sergiodj@redhat.com> <1437869674-7880-1-git-send-email-sergiodj@redhat.com> X-URL: http://blog.sergiodj.net Date: Sun, 26 Jul 2015 19:26:00 -0000 In-Reply-To: (Doug Evans's message of "Sun, 26 Jul 2015 01:05:43 -0700") Message-ID: <874mkq4t58.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00778.txt.bz2 On Sunday, July 26 2015, Doug Evans wrote: > On Sat, Jul 25, 2015 at 5:14 PM, Sergio Durigan Junior > wrote: >> It is known that GDB needs a valid shell to start the inferior and to >> offer the "shell" command to the user. This has recently been the >> cause of a problem on the MIPS buildslave, because $SHELL was set to >> /sbin/nologin and several tests were failing. The thread is here: >> >> >> >> However, I think we can do better than that. If 'startup-with-shell' >> is on, which is the default, we blindly trust that the user will >> provide a valid shell for us, and this may not be true all the time. >> So I am proposing a patch to increment the tests made by GDB before >> running the inferior to decide whether it will use $SHELL or not. >> Particularly, I created a new function, called "valid_shell", which >> defines the concept of a valid shell for GDB: >> >> - A file that exists and is executable by the user >> >> - A file that is not {,/usr}/sbin/nologin, nor /bin/false >> >> For now, I think this is enough to cover most cases. The default >> action when an invalid shell is found is to use /bin/sh instead (we >> already do that when $SHELL is not defined, for example), and also >> issue a warning to the user. This applies for when we are starting >> the inferior and when we are executing the "shell" command. >> >> To make things more robust, I made the code also check /bin/sh and >> make sure it is also valid. If it is not, and if we are starting the >> inferior, then GDB will use fork+exec instead. If we are executing >> the "shell" command and we cannot find a valid shell, GDB will error >> out. >> >> I updated the documentation to reflect the new behavior, and created a >> testcase to exercise the code. This patch has been regression-tested >> on Fedora 22 x86_64. >> >> OK to apply? >> >> Changes from v2: >> >> - Rewrote "Valid Shell" section in the documentation to mention that >> the tests performed are not exhaustive. Included a small example >> to demonstrate what happens if the user tries to use /bin/ls as >> the $SHELL (a "valid shell", in theory). >> >> Changes from v1: >> >> - Using @pxref instead of @ref in the documentation >> >> - Don't run the testcase when the target is mingw, cygwin, or remote >> >> - Including /usr/sbin/nologin and /bin/false in the list of invalid >> shells >> >> gdb/ChangeLog: >> 2015-07-24 Sergio Durigan Junior >> >> * cli/cli-cmds.c (shell_escape): Check if the selected shell is >> valid. >> * fork-child.c (check_startup_with_shell): New function. >> (fork_inferior): Move code to the new function above. Call it. >> * utils.c (valid_shell): New function. >> * utils.h (valid_shell): New prototype. >> >> gdb/doc/ChangeLog: >> 2015-07-24 Sergio Durigan Junior >> >> * gdb.texinfo (Shell Commands): Mention that the shell needs to be >> valid; point to "Valid Shell" subsection. >> (Valid Shell): New subsection. >> (Starting your Program): Mention that the shell needs to be valid; >> point to "Valid Shell" subsection. >> (Your Program's Arguments): Likewise. >> (Your Program's Environment): Likewise. >> >> gdb/testsuite/ChangeLog: >> 2015-07-24 Sergio Durigan Junior >> >> * gdb.base/invalid-shell.exp: New file. > > Hi. > > I'd like to not have this patch checked in, at least not yet. > > I'm going to leave security as a separate thread. > The topic here is just convenience and assistance (IIUC - > please correct me if I'm wrong). It is just assistance, indeed.. Security is definitely not the focus here. > Having an internally hardcoded list of shells (good or bad) suggests > to me there's got to be a better way. I'm definitely open to suggestions. > Another thing that bothers me is that if SHELL > is set to something gdb thinks is bad, gdb will > try to be "clever" and override that setting. > If a tool is going to be helpful, I think it > also needs a mode to not be. It's hard to > work around hardwired cleverness when > you don't want it. Hopefully in this instance > we can avoid adding an option though. Yeah. This can be easily fixed with (yet another) setting. 'set use-valid-shell on/off', maybe? > As a strawman, what if gdb first tests $SHELL > (e.g., $SHELL -c 'exit 42' or some such) > and if that doesn't work warn the user, > but otherwise leave things as is? > One could defer doing the test until the first > need for $SHELL. > And if $SHELL isn't usable, leave it to the > user to fix the problem. So you're suggesting that we only warn the user about the invalid shell, instead of deciding to use /bin/sh without asking her? As much as I think it *is* useful to have GDB default to /bin/sh if $SHELL is /sbin/nologin (for example), I am OK with just warning the user without taking any action. So, to summarize: what would you think of a patch that: - tested $SHELL (as you proposed; $SHELL -c 'exit 42'). - if the test fails, warn the user about it. If 'set use-valid-shell' is on, continue using /bin/sh; otherwise, just error out. ? Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/