Re: gzip bug w/ patch..

From: Wojtek Pilorz (wpilorz@bdk.pl)
Date: 12/31/01


Date: Mon, 31 Dec 2001 10:40:33 +0100 (CET)
From: Wojtek Pilorz <wpilorz@bdk.pl>
To: greg <gregn@dekode.org>

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



Relevant Pages

  • Re: Ten percent test
    ... current patch - but we hoped that his smaller change would be sufficient ... AFAIAC, gzip can take its turn in the queue, ... As it is, the IRQ's are being serviced, so no keystrokes are being lost, ... assembling an executable with a separate process printing the listing of ...
    (Linux-Kernel)
  • Re: initdiskless patch
    ... > either my patch to move gzip into /bin or something similar. ... > Another patch I have locally is to replace GNU gzip with the BSD ... but after a gzip binary is moved to the root needs to go away. ... you may want use a mfs based rootfs. ...
    (freebsd-current)
  • Re: initdiskless patch
    ... This patch would need either my patch to ... Another patch I have locally is to replace GNU gzip with the BSD licensed ... Moving this to the root is somewhat more of a challenge though ... This is completely unnecessary on diskless. ...
    (freebsd-current)
  • Re: gzip security update
    ... The October2 patch was declared with a higher version than this security ... The GNU data compression program. ... The gzip package contains the popular GNU gzip data compression ...
    (Fedora)