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

[Mike Frysinger <vapier at gentoo dot org>]

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

Attachment: signature.asc
Description: Digital signature