Re: [patch] Fix BZ #18660 -- overflow in getusershell

[Mike Frysinger <vapier at gentoo dot org>]

On 19 Aug 2015 09:28, Paul Pluzhnikov wrote:
> On Wed, Aug 19, 2015 at 7:55 AM, Mike Frysinger wrote:
> > /* 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?

hmm, looks like freebsd, openbsd, darwin, and gnulib don't require this.
i'll drop that.

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

the old logic is even worse than that.  it skips all leading bytes until it
finds a slash and then returns things after that, while splitting on space.
so a line like:
	space/foobar cow
will return "/foobar".

the docs i've seen (darwin, freebsd, linux) leave it fairly vague and say
"valid user shell".  but no one validates things, and really only document
comments and blank lines.  i think we should just go with that.

> >       /* Only accept valid shells (which we define as starting with a '/').  */
> >       if (shellbuf[0] == '/')
> 
> Do we want to return "/" as a valid shell here?

the current already does :).  since we aren't checking the path in the file 
(i.e. running stat and seeing if it's a +x file), i don't think we want to
treat / specially.  afterall, "/bogus" or "/bin" or "/sbin/" will still get
returned.

> > 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;

nope ... current code is correct.  see how the value of 0 is treated (which is 
also the default since it's a bss var).
-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:
      if (shellfp == NULL)
	{
	  setusershell ();
	  return getusershell ();
	}
      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 (1)
    {
      ssize_t len;
      char *p;

      len = getline (&shellbuf, &buf_len, shellfp);
      if (len <= 0)
	break;

      /* Strip out any comment lines.  */
      p = strchr (shellbuf, '#');
      if (p)
	*p = '\0';
      else
	{
	  /* Chop the trailing newline.  */
	  p = shellbuf + len - 1;
	  if (*p == '\n')
	    *p = '\0';
	}

      /* Only accept valid shells (which we define as starting with a '/').  */
      if (shellbuf[0] == '/')
	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;
}

Attachment: signature.asc
Description: Digital signature