Fix BZ #18660 -- overflow in getusershell
Commit Message
Greetings,
Attached patch fixes BZ #18660 -- overflow in getusershell.
Note: the file does not follow GNU coding convention, should I make a
whitespace-only pass over it?
Note: Tobias proposed a different patch:
https://sourceware.org/bugzilla/attachment.cgi?id=8502, but I like my
patch better.
P.S. AFAICT, this is nearly impossible to test :-(
Thanks,
2015-08-15 Paul Pluzhnikov <ppluzhnikov@google.com>
Tobias Stoeckmann <tobias@stoeckmann.org>
[BZ #18660]
* misc/getusershell.c (initshells): Fix possible overflow.
Comments
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?
> On August 17, 2015 at 12:59 PM 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?
if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3)
goto init_okshells;
flen = statb.st_size + 3;
The check has to focus on flen's statb.st_size + 3 anyway.
It's larger than statb.st_size / 3.
On Mon, 17 Aug 2015, Tobias Stöckmann wrote:
> > On August 17, 2015 at 12:59 PM 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?
>
> if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3)
> goto init_okshells;
> flen = statb.st_size + 3;
>
> The check has to focus on flen's statb.st_size + 3 anyway.
> It's larger than statb.st_size / 3.
It's smaller than statb.st_size / 3 * sizeof (char *) (or the new adjusted
version thereof), since sizeof (char *) is 4 or 8 for configurations
supported by glibc.
> On August 17, 2015 at 5:48 PM Joseph Myers <joseph@codesourcery.com> wrote:
> It's smaller than statb.st_size / 3 * sizeof (char *) (or the new adjusted
> version thereof), since sizeof (char *) is 4 or 8 for configurations
> supported by glibc.
I wonder what makes me blind towards "sizeof (char *)" these days.
You are right, of course.
On 15 Aug 2015 11:31, Paul Pluzhnikov wrote:
> P.S. AFAICT, this is nearly impossible to test :-(
it is if we get clever/abusive. if you change misc/getusershell.c like so:
+#ifndef _LOCAL_PATH_SHELLS
+# define _LOCAL_PATH_SHELLS _PATH_SHELLS
+#endif
- if ((fp = fopen(_PATH_SHELLS, "rce")) == NULL)
+ if ((fp = fopen(_LOCAL_PATH_SHELLS, "rce")) == NULL)
then add -D_LOCAL_PATH_SHELLS='"$(srcdir)/tst-shells.conf"' to the test's
CPPFLAGS and list misc/getusershell.c directly in the test lines. that
way your test code will use those funcs directly with your custom shell
path.
-mike
@@ -119,7 +119,7 @@ initshells (void)
flen = statb.st_size + 3;
if ((strings = malloc(flen)) == NULL)
goto init_okshells;
- shells = malloc(statb.st_size / 3 * sizeof (char *));
+ shells = malloc(((statb.st_size / 3) + 2) * sizeof (char *));
if (shells == NULL) {
free(strings);
strings = NULL;
@@ -130,7 +130,8 @@ initshells (void)
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')