Re: gzip bug w/ patch..

[Wojtek Pilorz <wpilorz () bdk pl>]

Greg,

strncpy in the patch you have attached might not include 0 terminator,
which seems dangerous.

I think it would be preferred to follow
strncpy(ifname, iname, sizeof(ifname) - 1);
with
ifname[sizeof(ifname) - 1] = 0;

Whether too long names should be quietly truncated and then accepted
is another problem - I guess they should not (esp. since the path might be
later modified, e.g. by appending .gz)
In any case, the first part of the patch does not seem to go far enough,
since there are other strcpy/strcat calls below line 1009 in get_istat
function.

On the other hand, a quick look at gzip-1.2.4a/gzip.c, lines 1683 and
following shows that the second part of patch is not needed, since 
sizeof(nbuf) is MAX_PATH_LEN and the if condition at line 1685
would prevent strcpy from being performed if 'dir' is too long.
 
Thank you for drawing our attention to this gzip problem.

Best regards,

Wojtek
--------------------
Wojtek Pilorz
Wojtek.Pilorz () bdk pl


On Sun, 30 Dec 2001, greg wrote:

> Date: Sun, 30 Dec 2001 14:26:10 -0000
> From: greg <gregn () dekode org>
> To: bugtraq () securityfocus com
> Subject: gzip bug w/ patch..
>
> Earlier, Goobles had pointed out a bug in Gzip pertaining to this code in
> (gzip.c):
>
>  if (len + NLENGTH(dp) + 1 < MAX_PATH_LEN - 1) {
>             strcpy(nbuf,dir);
>             if (len != 0 /* dir = "" means current dir on Amiga */
> #ifdef PATH_SEP2
>
>
> while looking through I have found that the real problem lied here in
> (gzip.c):
>
> line 1009:
>
>     strcpy(ifname, iname);
>
>
> well anyway, there is an attached patch, bye.
>
>
>
>
>
> --- gzip.c    Thu Aug 19 09:39:43 1993
> +++ gzip-fix.c        Sun Dec 30 13:57:44 2001
> @@ -1006,7 +1006,7 @@
>      char *dot; /* pointer to ifname extension, or NULL */
>  #endif
>  
> -    strcpy(ifname, iname);
> +    strncpy(ifname, iname, sizeof(ifname) - 1);
>  
>      /* If input file exists, return OK. */
>      if (do_stat(ifname, sbuf) == 0) return OK;
> @@ -1683,7 +1683,7 @@
>       }
>       len = strlen(dir);
>       if (len + NLENGTH(dp) + 1 < MAX_PATH_LEN - 1) {
> -         strcpy(nbuf,dir);
> +         strncpy(nbuf, dir, sizeof(nbuf) - 1);
>           if (len != 0 /* dir = "" means current dir on Amiga */
>  #ifdef PATH_SEP2
>               && dir[len-1] != PATH_SEP2