There's still a possible overflow in getusershell.c. We could increase the buffer allocation yet again, but I have to agree with the glibc developers here: enough is enough. The code is ugly and has proven to be difficult to review.
The overflow has been spotted by Paul Pluzhnikov, after I submitted a bug report of our getusershell fixes to glibc, as seen here: https://sourceware.org/bugzilla/show_bug.cgi?id=18660 It led to a discussion of the glibc developers. Eventually, Mike Frysinger came up with a new implementation, which is not in glibc yet. I like it a lot, and Mike was kind enough to allow an MIT-licensed version for us. If in doubt, Mike is in CC. It's basically his implementation, plus KNF and using strcspn for shorter code. The new code differs from old behavior, but it should be sufficient. We support /etc/shells as seen in our system and as it is described in shells(5). But the wording of shells(5) is a bit odd. It makes me believe that relative paths would be okay, because: For each shell a single line should be present, consisting of the shell's path, relative to root. That is not true with the new code, and should be avoided IMHO. If this implementation encounters a line that does not start with a slash '/', it will be discarded. Maybe we should adjust the manual page to be more precise? If you want to test this code, chpass is the easiest way. Change your login shell to something that is not in /etc/shells and you should get a warning. If it's in /etc/shells, no warning shall be seen. Is it worth it, knowing that it's a deprecated BSD-specific function? Or maybe just increase buffer allocation (hopefully just) one more time? Tobias Index: lib/libc/gen/getusershell.c =================================================================== RCS file: /cvs/src/lib/libc/gen/getusershell.c,v retrieving revision 1.16 diff -u -p -u -p -r1.16 getusershell.c --- gen/getusershell.c 14 Sep 2015 16:09:13 -0000 1.16 +++ gen/getusershell.c 4 Dec 2015 21:39:01 -0000 @@ -1,131 +1,106 @@ -/* $OpenBSD: getusershell.c,v 1.16 2015/09/14 16:09:13 tedu Exp $ */ +/* $OpenBSD$ */ /* - * Copyright (c) 1985, 1993 - * The Regents of the University of California. All rights reserved. + * Copyright (c) 2015 Mike Frysinger <vap...@gentoo.org> * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions - * are met: - * 1. Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright - * notice, this list of conditions and the following disclaimer in the - * documentation and/or other materials provided with the distribution. - * 3. Neither the name of the University nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. * - * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS - * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY - * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include <sys/stat.h> - -#include <ctype.h> -#include <limits.h> +#include <assert.h> #include <paths.h> -#include <stdint.h> #include <stdio.h> #include <stdlib.h> +#include <string.h> #include <unistd.h> -/* - * Local shells should NOT be added here. They should be added in - * /etc/shells. - */ +/* 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; -static char *okshells[] = { _PATH_BSHELL, _PATH_CSHELL, _PATH_KSHELL, NULL }; -static char **curshell, **shells, *strings; -static char **initshells(void); - -/* - * Get a list of shells from _PATH_SHELLS, if it exists. - */ char * getusershell(void) { - char *ret; + /* + * 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: + okshell_idx++; + return (char *) _PATH_KSHELL; + case 4: + return NULL; + } + + /* Walk the /etc/shells file. */ + while (getline(&shellbuf, &buf_len, shellfp) != -1) { + /* Strip out any comments and trailing newline. */ + shellbuf[strcspn(shellbuf, "#\n")] = '\0'; + + /* + * Only accept valid shells + * (which we define as starting with a '/'). + */ + if (shellbuf[0] == '/') + return shellbuf; + } + + /* No more valid shells. */ + return NULL; +} - if (curshell == NULL) - curshell = initshells(); - ret = *curshell; - if (ret != NULL) - curshell++; - return (ret); +static void +usershell_reset(void) +{ + okshell_idx = 0; + if (shellfp != NULL) { + fclose(shellfp); + shellfp = NULL; + free(shellbuf); + shellbuf = NULL; + } } void endusershell(void) { - - free(shells); - shells = NULL; - free(strings); - strings = NULL; - curshell = NULL; + usershell_reset(); } 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. + */ + usershell_reset(); - curshell = initshells(); + if ((shellfp = fopen(_PATH_SHELLS, "rce")) == NULL) + okshell_idx = 1; } -static char ** -initshells(void) -{ - char **sp, *cp; - FILE *fp; - struct stat statb; - - free(shells); - shells = NULL; - free(strings); - strings = NULL; - if ((fp = fopen(_PATH_SHELLS, "re")) == NULL) - return (okshells); - if (fstat(fileno(fp), &statb) == -1) { - (void)fclose(fp); - return (okshells); - } - if (statb.st_size > SIZE_MAX) { - (void)fclose(fp); - return (okshells); - } - if ((strings = malloc((size_t)statb.st_size)) == NULL) { - (void)fclose(fp); - return (okshells); - } - shells = calloc((size_t)(statb.st_size / 3 + 2), sizeof (char *)); - if (shells == NULL) { - (void)fclose(fp); - free(strings); - strings = NULL; - return (okshells); - } - sp = shells; - cp = strings; - while (fgets(cp, PATH_MAX + 1, fp) != NULL) { - while (*cp != '#' && *cp != '/' && *cp != '\0') - cp++; - if (*cp == '#' || *cp == '\0') - continue; - *sp++ = cp; - while (!isspace((unsigned char)*cp) && *cp != '#' && *cp != '\0') - cp++; - *cp++ = '\0'; - } - *sp = NULL; - (void)fclose(fp); - return (shells); -}
