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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: getusershell.c
Type: text/x-c
Size: 2847 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150819/2956d44a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150819/2956d44a/attachment.sig>