This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [patch] Fix BZ #18660 -- overflow in getusershell
- From: Paul Pluzhnikov <ppluzhnikov at gmail dot com>
- To: Paul Pluzhnikov <ppluzhnikov at gmail dot com>, Joseph Myers <joseph at codesourcery dot com>, GLIBC Devel <libc-alpha at sourceware dot org>, Tobias Stoeckmann <tobias at stoeckmann dot org>
- Date: Wed, 19 Aug 2015 09:28:48 -0700
- Subject: Re: [patch] Fix BZ #18660 -- overflow in getusershell
- Authentication-results: sourceware.org; auth=none
- References: <CAPC3xaqdOk4EWQJEiBLidfVxSx1iH5F9k_DTZDamkjQR1xZ3Gw at mail dot gmail dot com> <alpine dot DEB dot 2 dot 10 dot 1508171058110 dot 9234 at digraph dot polyomino dot org dot uk> <CAPC3xaqRbCc1Ytd7AAkmEjgr_wtq_AA-hEvRPZiup2QwHMdS-w at mail dot gmail dot com> <20150819145531 dot GE1584 at vapier>
On Wed, Aug 19, 2015 at 7:55 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> lightly tested from scratch version below. code & data size is smaller than
> the current version in the tree.
> -mike
>
> /* Copyright (C) 2015 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
>
> The GNU C Library is free software; you can redistribute it and/or
> modify it under the terms of the GNU Lesser General Public
> License as published by the Free Software Foundation; either
> version 2.1 of the License, or (at your option) any later version.
>
> The GNU C Library 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
> Lesser General Public License for more details.
>
> You should have received a copy of the GNU Lesser General Public
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> #include <assert.h>
> #include <paths.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
>
> /* Used by tests to point to custom paths. */
> #ifndef _LOCAL_PATH_SHELL
> # define _LOCAL_PATH_SHELLS _PATH_SHELLS
> #endif
>
> /* These functions are not thread safe (by design), so globals are OK. */
> static FILE *shellfp;
> static int okshell_idx;
> static char *shellbuf;
> static size_t buf_len;
>
> char *
> getusershell (void)
> {
> /* Handle the builtin case first.
>
> NB: we do not use a global array here as the initialization needs
> relocations. These interfaces are used so rarely that this is not
> justified. Instead open code it with a switch statement. */
> switch (okshell_idx)
> {
> case 0:
> /* Require the user call setusershell first. */
> assert (shellfp != NULL);
I could find no man page that requires user calling setusershell first.
Is there a definitive description of this interface?
> break;
> case 1:
> okshell_idx++;
> return (char *) _PATH_BSHELL;
> case 2:
> okshell_idx++;
> return (char *) _PATH_CSHELL;
> case 3:
> return NULL;
> }
>
> /* Walk the /etc/shells file. */
> while (getline (&shellbuf, &buf_len, shellfp) != -1)
> {
> char *p;
>
> /* Strip out any comment lines. */
> p = strchr (shellbuf, '#');
> if (p)
> *p = '\0';
> else
> {
> /* Chop the trailing newline. */
> p = strchr (shellbuf, '\n');
Current version chops on white space. Do we want to return "/foo bar"
here? Of course any sysadmin who puts "/foo bar" into /etc/shells gets
what he deserves.
> if (p)
> *p = '\0';
> }
>
> /* Only accept valid shells (which we define as starting with a '/'). */
> if (shellbuf[0] == '/')
Do we want to return "/" as a valid shell here?
> return shellbuf;
> }
>
> /* No more valid shells. */
> return NULL;
> }
>
> hidden_proto (endusershell);
> void
> endusershell (void)
> {
> okshell_idx = 0;
> if (shellfp)
> {
> fclose (shellfp);
> shellfp = NULL;
> free (shellbuf);
> shellbuf = NULL;
> }
> }
> hidden_def (endusershell);
>
> void
> setusershell (void)
> {
> /* We could rewind shellfp here, but we get smaller code this way.
> Keep in mind this is not a performance sensitive API. */
> endusershell ();
>
> shellfp = fopen (_LOCAL_PATH_SHELLS, "rce");
> if (shellfp == NULL)
> okshell_idx = 1;
I think you missed a part here:
else
okshell_idx = 4;
> }
--
Paul Pluzhnikov