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

[Paul Pluzhnikov <ppluzhnikov at gmail dot com>]

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