Fix BZ #18660 -- overflow in getusershell

  On Mon, Aug 17, 2015 at 3:59 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> Since you're increasing an allocation size, don't you also need to adjust
> the check a few lines earlier for whether the allocation size calculation
> would overflow?

Thanks. Revised patch attached.

2015-08-15  Paul Pluzhnikov  <ppluzhnikov@google.com>
            Tobias Stoeckmann <tobias@stoeckmann.org>

        [BZ #18660]
        * misc/getusershell.c (initshells): Fix possible overflow.
  

On 18 Aug 2015 16:54, Paul Pluzhnikov wrote:
>  	while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) {
>  		while (*cp != '#' && *cp != '/' && *cp != '\0')
>  			cp++;
> -		if (*cp == '#' || *cp == '\0' || cp[1] == '\0')
> +		/* Reject non-absolute paths, or anything too short.  */
> +		if (cp[0] != '/' || cp[1] == '\0' || isspace(cp[1]))
>  			continue;
>  		*sp++ = cp;
>  		while (!isspace(*cp) && *cp != '#' && *cp != '\0')
>  			cp++;
> +		assert (cp < strings + flen);
>  		*cp++ = '\0';
>  	}

i'm having a hard time seeing how this works sanely (even before your change).
considering this file starts off with this comment:
/* NB: we do not initialize okshells here.  The initialization needs
   relocations.  These interfaces are used so rarely that this is not
   justified.  Instead explicitly initialize the array when it is
   used.  */

how can we justify this ugly code in the name of speed ?  wouldn't it be nicer
to gut it completely and avoid the stat/large malloc/etc... ?

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);
      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');
	  if (p)
	    *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;
}
  

Mike Frysinger <vapier@gentoo.org> writes:

> how can we justify this ugly code in the name of speed ?

This ugly code was lifted directly from BSD ...

Andreas.
  

On 19 Aug 2015 17:26, Andreas Schwab wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> > how can we justify this ugly code in the name of speed ?
> 
> This ugly code was lifted directly from BSD ...

i'm aware of its history.  that doesn't justify us keeping the ugly code,
just the API.
-mike
  

On Wed, Aug 19, 2015 at 12:19 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 19 Aug 2015 17:26, Andreas Schwab wrote:
>> Mike Frysinger <vapier@gentoo.org> writes:
>> > how can we justify this ugly code in the name of speed ?
>>
>> This ugly code was lifted directly from BSD ...
>
> i'm aware of its history.  that doesn't justify us keeping the ugly code,
> just the API.

FWIW I concur with Mike.

zw
  

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;


> }
  

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
  

Mike,

your proposal looks good to me (I read it and wrote a small test program).
In fact, I see it as a great improvement to the status quo.

Have you integrated your version and tried to add a test case as proposed earlier ?

Best regards
Tolga Dalman


On 08/19/2015 08:27 PM, Mike Frysinger wrote:
> 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
>
  

On 26 Aug 2015 23:07, Tolga Dalman wrote:
> your proposal looks good to me (I read it and wrote a small test program).
> In fact, I see it as a great improvement to the status quo.
> 
> Have you integrated your version and tried to add a test case as proposed earlier ?

all i've done is post this patch.  if people are happy with it and the
direction, i can add some tests.
-mike
  

> all i've done is post this patch.  if people are happy with it and the
> direction, i can add some tests.

I think the comment handling could be further adjusted.

After all, it IS possible to have a shell path containing '#' or ' '.
I won't argue that it's a sane thing to do, but the file system allows
it. And right now it's impossible to specify such a shell.

It is also specified that a valid shell must start with '/', i.e. it
must be an absolute path.

So, how about ignoring every line that does not define an absolute path?
That way, comments are supported, too.

Unfortunately I don't know if anyone writes comments next to the
absolute path in the file, like:

/bin/sh # korn shell