[PATCH] Possible buffer overflow

[PATCH] Possible buffer overflow

[Matthias Koenig]

Hi,

There seems to be a possible buffer overflow in modules/mount_afs.c.
strncat(dest, src, n) uses at most n chars from src. n is not the
size of dest. Patch below.

Regards,
Matthias

--- modules/mount_afs.c
+++ modules/mount_afs.c
@@ -36,8 +36,8 @@
 	char dest[PATH_MAX * 2];
 
 	strcpy(dest, root);	/* Convert the name to a mount point. */
-	strncat(dest, "/", sizeof(dest));
-	strncat(dest, name, sizeof(dest));
+	strncat(dest, "/", sizeof(dest)-strlen(dest)-1);
+	strncat(dest, name, sizeof(dest)-strlen(dest)-1);
 
 	/* remove trailing slash (http://bugs.debian.org/141775) */
 	if (dest[strlen(dest)-1] == '/')

Re: [PATCH] Possible buffer overflow

[Jeff Moyer]

==> On Thu, 30 Nov 2006 18:06:43 +0100, Matthias Koenig <mkoenig@suse.de> said:

Matthias> Hi,
Matthias> There seems to be a possible buffer overflow in modules/mount_afs.c.
Matthias> strncat(dest, src, n) uses at most n chars from src. n is not the
Matthias> size of dest. Patch below.

The fix seems correct to me.  Have you actually seen a problem in the
wild?  I wonder why the dest string is twice the maximum path length;
that doesn't make a whole lot of sense.  Perhaps we should fix that
while we're in here.

-Jeff

> --- modules/mount_afs.c
> +++ modules/mount_afs.c
> @@ -36,8 +36,8 @@
>  	char dest[PATH_MAX * 2];

>  	strcpy(dest, root);	/* Convert the name to a mount point. */
> -	strncat(dest, "/", sizeof(dest));
> -	strncat(dest, name, sizeof(dest));
> +	strncat(dest, "/", sizeof(dest)-strlen(dest)-1);
> +	strncat(dest, name, sizeof(dest)-strlen(dest)-1);

>  	/* remove trailing slash (http://bugs.debian.org/141775) */
>  	if (dest[strlen(dest)-1] == '/')

Re: [PATCH] Possible buffer overflow

[Ian Kent]

On Thu, 2006-11-30 at 14:35 -0500, Jeff Moyer wrote:
> ==> On Thu, 30 Nov 2006 18:06:43 +0100, Matthias Koenig <mkoenig@suse.de> said:
> 
> Matthias> Hi,
> Matthias> There seems to be a possible buffer overflow in modules/mount_afs.c.
> Matthias> strncat(dest, src, n) uses at most n chars from src. n is not the
> Matthias> size of dest. Patch below.
> 
> The fix seems correct to me.  Have you actually seen a problem in the
> wild?  I wonder why the dest string is twice the maximum path length;
> that doesn't make a whole lot of sense.  Perhaps we should fix that
> while we're in here.

Yes, that array does seem to be excessive.
I was thinking that it would be better to calculate the length, check it
and then do the copy.

> 
> -Jeff
> 
> > --- modules/mount_afs.c
> > +++ modules/mount_afs.c
> > @@ -36,8 +36,8 @@
> >  	char dest[PATH_MAX * 2];
> 
> >  	strcpy(dest, root);	/* Convert the name to a mount point. */
> > -	strncat(dest, "/", sizeof(dest));
> > -	strncat(dest, name, sizeof(dest));
> > +	strncat(dest, "/", sizeof(dest)-strlen(dest)-1);
> > +	strncat(dest, name, sizeof(dest)-strlen(dest)-1);
> 
> >  	/* remove trailing slash (http://bugs.debian.org/141775) */
> >  	if (dest[strlen(dest)-1] == '/')
> 
> _______________________________________________
> autofs mailing list
> autofs@linux.kernel.org
> http://linux.kernel.org/mailman/listinfo/autofs