diff mbox

Fix BZ #18660 -- overflow in getusershell

Message ID CAPC3xaqdOk4EWQJEiBLidfVxSx1iH5F9k_DTZDamkjQR1xZ3Gw@mail.gmail.com
State New, archived
Headers show

Commit Message

Paul Pluzhnikov Aug. 15, 2015, 6:31 p.m. UTC
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

Joseph Myers Aug. 17, 2015, 10:59 a.m. UTC | #1
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?
Tobias Stöckmann Aug. 17, 2015, 3:33 p.m. UTC | #2
> 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.
Joseph Myers Aug. 17, 2015, 3:48 p.m. UTC | #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.
Tobias Stöckmann Aug. 17, 2015, 3:53 p.m. UTC | #4
> 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.
Mike Frysinger Aug. 19, 2015, 2:02 p.m. UTC | #5
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
diff mbox

Patch

diff --git a/misc/getusershell.c b/misc/getusershell.c
index fc2c43b..44143dc 100644
--- a/misc/getusershell.c
+++ b/misc/getusershell.c
@@ -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')