131826 - upgrade zlib version in cmd/zlib

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Jamie Nicolson]

The zlib in cmd/zlib is version 1.0.4. There is a security hole in zlib versions
1.0.9 - 1.1.3. This hole does not affect our version, but there may be other
problems with 1.0.4 (see http://www.gzip.org/zlib/advisory-2002-03-11.txt).

We should update our zlib to the latest version.

Comment 1

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 2

[Wan-Teh Chang]

Target Future.

Comment 3

[Wan-Teh Chang]

Actually, we should consider doing this in NSS 3.6.

Comment 4

[Henrik Gemal]

due to the security bug in zlib < 1.1.4 zlib should be upgraded!

Comment 5

[Wan-Teh Chang]

*** Bug 185479 has been marked as a duplicate of this bug. ***

Comment 6

[Glenn Randers-Pehrson]

It's not quite a duplicate because this one says zlib-1.0.4 is OK, but now
we know it isn't.  Bug 185479 increases the urgency of fixing this
bug, since it is now claimed that there is an exploit for the vulnerability
in zlib-1.0.4.

Glenn

Comment 7

[Wan-Teh Chang]

[e-mail] pointed out that zlib 1.0.4 has a
heap corruption vulnerability.  See bug 185466
for more information.

The zlib in NSS is only used by the NSS command
line tools signtool and modutil.

Comment 8

[Wan-Teh Chang]

Jamie, I just created the NSS_ZLIB_1_1_4_BRANCH for the
mozilla/security/nss/cmd/zlib directory.

I've checked in the latest README and .c and .h files
from zlib 1.1.4 on that branch.

I've also removed stubs.c and zip_nodl.c, which I believe
are Netscape's own additions and are obsolete now.  Please
take a look at these two files and let me know if you agree.

I can compile the new zlib using our current makefiles on
Solaris and Windows.  Our test suite passed.

We should review the Netscape modifications to zlib 1.0.4
and see any of them need to be carried over to the new zlib.

Comment 9

[Jamie Nicolson]

I agree that stubs.c and zip_nodl.c are not needed anymore.

I compared the current tip of the trunk against the zlib 1.0.4 sources. Most of
the differences were build changes: declaring functions with PR_PUBLIC_API, lots
of #ifdef MOZILLA_CLIENT, etc. I found two files that had functional differences.

In infblock.c, we have slightly different ways of freeing memory. Most, but not
all, of these changes were added by wtc in March 2002, in response to the
denial-of-service bug report.

In inftrees.c, there are some algorithmic changes. They seem to have already
been incorporated into the 1.1.4 sources, so they may have been bug fixes that
we copied into our source tree.

I will go ahead and check in the diff file as an attachment to the bug. I think
we should go ahead with the 1.1.4 sources.

Comment 10

[Jamie Nicolson]

Attachment: differences between trunk and zlib 1.0.4

This is list of differences between the current tip of the trunk and the
zlib-1.0.4 sources.

Comment 11

[Wan-Teh Chang]

Comment on attachment 109579 [details]
differences between trunk and zlib 1.0.4

Thanks for looking into this, Jamie.

Could you regenerate the diffs with the -u or -c
flag?

Comment 12

[Glenn Randers-Pehrson]

The change at line 48 of inflate_sync() (changes two array values
from 192 to 112) is in fact the fix for the heap corruption vulnerability.

So this bug is not so urgent...

Glenn

Comment 13

[Jamie Nicolson]

Attachment: differences between trunk and zlib 1.0.4

Here are the context diffs.

Comment 14

[Glenn Randers-Pehrson]

I meant line 48 of inftrees.c.  This is the crucial update:

=====================> inftrees.c <=====================
47c48
<  3, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 0, 192, 192}; /* 192==invalid */
>  3, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 0, 112, 112}; /* 112==invalid */

Glenn

Comment 15

[Wan-Teh Chang]

Attachment: Test results with zlib 1.1.4

I plugged in zlib 1.1.4.  The good news is that it compiles
with no changes on all platforms.  The bad news is that two
signtool tests ("Listing signed files in jar (signtool -v)"
and "Show who signed jar (signtool -w)") fail intermittently.
This is a results.html where the two signtool tests failed.
I will attach its output.log next.

Comment 16

[Wan-Teh Chang]

Attachment: Test output log with zlib 1.1.4

The output of the two signtool tests that failed is listed
below:

tools.sh: Listing signed files in jar ----------------------
signtool -v nojs.jar -d ../alicedir -p nss -k objsigner
using certificate directory: ../alicedir

NOTE -- "nojs.jar" archive DID NOT PASS crypto verification.
  (reported reason: General JAR file error)

entries shown below will have their digests checked only.

	  status   path
    ------------   -------------------
	verified   sign.html
	verified   signjs.html

NOTE -- "nojs.jar" archive DID NOT PASS crypto verification.
tools.sh: Show who signed jar ------------------------------
signtool -w nojs.jar -d ../alicedir
using certificate directory: ../alicedir
NOTE -- "nojs.jar" archive DID NOT PASS crypto verification.
  (reported reason: General JAR file error)

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 18

[Glenn Randers-Pehrson]

Zlib version 1.2.1 was released a couple of weeks ago.  It is supposed
to run significantly faster than 1.1.4.

Comment 19

[Wan-Teh Chang]

Comment on attachment 109589 [details]
differences between trunk and zlib 1.0.4

Here is a summary of the changes in this file.
Note that we are compiling our zlib sources
with -DMOZILLA_CLIENT.

1. 27 functions are defined in .c files with
PR_PUBLIC_API, and 24 functions are declared
in zlib.h with PR_PUBLIC_API.

PR_PUBLIC_API is the same as PR_IMPLEMENT.
On some platforms it makes the function
exported from the shared library/DLL.

Since we do not build our zlib as a shared
library/DLL, these PR_PUBLIC_API's are not
necessary.

2. In deflate.c, the macro around the
declaration and definition of check_match
is changed from DEBUG to DEBUG_NEVER.

A fprintf(stderr,...) statement inside
check_match is commented out.

Since we do not compile with -DDEBUG_NEVER,
check_match expands to nothing in our debug
and optimized builds.

check_match is used as an assertion.  If
the match is found to be invalid, it causes
the program to exit.

3. infblock.c contains changes (some new
ZFREE calls, one new inflate_trees_free call)
that look like fixes for double free or memory
leaks.

4. inftrees.c contains changes that look
like bug fixes.

5. zconf.h contains a porting change for XP_OS2
(to cause the STDC macro to be defined).
Since OS2 is handled in the zconf.h in zlib 1.1.4
and 1.2.1, this porting change should not be
necessary when we upgrade zlib.  Need to build
on OS/2 to verify.

zconf.h is modified to include prtypes.h in
order to get the definition of PR_PUBLIC_API.
This change is not necessary (see #1).

6. zutil.h contains a __WIN32_WCE porting
change to handle errno.  (Windows CE does
not have errno.)

In zutil.h, the macro around 'verbose', z_error,
Assert, and Trace* is changed from DEBUG to
DEBUG_ZLIB, and inclusion of <stdio.h>, definition
of 'verbose', and declaration of z_error are added
to the #else block.

Since we do not compile with -DDEBUG_ZLIB, we
use the #else block in our debug and
optimized builds.  So Assert and Trace* all
expand to nothing.  We may want to carry over
this change when we upgrade zlib because we
don't want trace messages in our debug build.

7. In trees.c, the macro around the definition
of the send_code macro is changed from DEBUG
to DEBUG_NEVER.  Since we do not compile with
-DDEBUG_NEVER, send_code expands to simply
send_bits, without the if (verbose>2) fprintf(stderr,...)
statement, in our debug and optimized builds.

This change is not necessary because we do
not define 'verbose', so zutil.h defines
'verbose' to be 0.

8. In zutil.c, we define z_error to call
PR_ASSERT(0) instead of printing the message
'm' and calling exit(1).

This change is not necessary because R_ASSERT(0)
also terminates the program. 

9. In zutil.h, if HAVE_STRERROR is defined, the
declaration of strerror is commented out.  I
believe this is to avoid conflicting with the
declaration of strerror on Windows, whose <string.h>
declares strerror with _declspec(dllimport).
(strerror is a Standard C library function
declared in <string.h>.)

This change is not necessary as long as we
do not define HAVE_STRERROR on Windows.

I think zlib should not manually declare
strerror if HAVE_STRERROR is defined.  It
should just rely on the declaration in
<string.h>.

Comment 20

[Glenn Randers-Pehrson]

The confidential flag has just been removed from bug #185466 which gives details
about the zlib-1.0.4 vulnerability.  That's not a problem here because NSS zlib
has been patched already to defend against this vulnerability (see comment #12).
 But perhaps we should go ahead and update security/nss/src/zlib to a current
version, either 1.2.1 (patched, like the other zlib in the tree,
modules/zlib/src), or 1.2.2 which is the latest release from Mark Adler.  Why
not keep both zlibs in sync if they cannot be merged into one instance in the tree?
Do the signtool tests work with zlib-1.2.2?

Comment 21

[Wan-Teh Chang]

I just upgraded mozilla/security/nss/cmd/zlib on the
trunk to zlib 1.2.2.

Checking in README;
/cvsroot/mozilla/security/nss/cmd/zlib/README,v  <--  README
new revision: 1.4; previous revision: 1.3
done
Checking in adler32.c;
/cvsroot/mozilla/security/nss/cmd/zlib/adler32.c,v  <--  adler32.c
new revision: 1.4; previous revision: 1.3
done
Checking in compress.c;
/cvsroot/mozilla/security/nss/cmd/zlib/compress.c,v  <--  compress.c
new revision: 1.4; previous revision: 1.3
done
Checking in config.mk;
/cvsroot/mozilla/security/nss/cmd/zlib/config.mk,v  <--  config.mk
new revision: 1.6; previous revision: 1.5
done
Checking in crc32.c;
/cvsroot/mozilla/security/nss/cmd/zlib/crc32.c,v  <--  crc32.c
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/security/nss/cmd/zlib/crc32.h,v
done
Checking in crc32.h;
/cvsroot/mozilla/security/nss/cmd/zlib/crc32.h,v  <--  crc32.h
initial revision: 1.1
done
Checking in deflate.c;
/cvsroot/mozilla/security/nss/cmd/zlib/deflate.c,v  <--  deflate.c
new revision: 1.4; previous revision: 1.3
done
Checking in deflate.h;
/cvsroot/mozilla/security/nss/cmd/zlib/deflate.h,v  <--  deflate.h
new revision: 1.4; previous revision: 1.3
done
Checking in example.c;
/cvsroot/mozilla/security/nss/cmd/zlib/example.c,v  <--  example.c
new revision: 1.4; previous revision: 1.3
done
Checking in gzio.c;
/cvsroot/mozilla/security/nss/cmd/zlib/gzio.c,v  <--  gzio.c
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/security/nss/cmd/zlib/infback.c,v
done
Checking in infback.c;
/cvsroot/mozilla/security/nss/cmd/zlib/infback.c,v  <--  infback.c
initial revision: 1.1
done
Removing infblock.c;
/cvsroot/mozilla/security/nss/cmd/zlib/infblock.c,v  <--  infblock.c
new revision: delete; previous revision: 1.5
done
Removing infblock.h;
/cvsroot/mozilla/security/nss/cmd/zlib/infblock.h,v  <--  infblock.h
new revision: delete; previous revision: 1.3
done
Removing infcodes.c;
/cvsroot/mozilla/security/nss/cmd/zlib/infcodes.c,v  <--  infcodes.c
new revision: delete; previous revision: 1.3
done
Removing infcodes.h;
/cvsroot/mozilla/security/nss/cmd/zlib/infcodes.h,v  <--  infcodes.h
new revision: delete; previous revision: 1.3
done
Checking in inffast.c;
/cvsroot/mozilla/security/nss/cmd/zlib/inffast.c,v  <--  inffast.c
new revision: 1.4; previous revision: 1.3
done
Checking in inffast.h;
/cvsroot/mozilla/security/nss/cmd/zlib/inffast.h,v  <--  inffast.h
new revision: 1.4; previous revision: 1.3
done
Checking in inffixed.h;
/cvsroot/mozilla/security/nss/cmd/zlib/inffixed.h,v  <--  inffixed.h
new revision: 1.3; previous revision: 1.2
done
Checking in inflate.c;
/cvsroot/mozilla/security/nss/cmd/zlib/inflate.c,v  <--  inflate.c
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/security/nss/cmd/zlib/inflate.h,v
done
Checking in inflate.h;
/cvsroot/mozilla/security/nss/cmd/zlib/inflate.h,v  <--  inflate.h
initial revision: 1.1
done
Checking in inftrees.c;
/cvsroot/mozilla/security/nss/cmd/zlib/inftrees.c,v  <--  inftrees.c
new revision: 1.4; previous revision: 1.3
done
Checking in inftrees.h;
/cvsroot/mozilla/security/nss/cmd/zlib/inftrees.h,v  <--  inftrees.h
new revision: 1.4; previous revision: 1.3
done
Removing infutil.c;
/cvsroot/mozilla/security/nss/cmd/zlib/infutil.c,v  <--  infutil.c
new revision: delete; previous revision: 1.3
done
Removing infutil.h;
/cvsroot/mozilla/security/nss/cmd/zlib/infutil.h,v  <--  infutil.h
new revision: delete; previous revision: 1.3
done
Removing makefile.win;
/cvsroot/mozilla/security/nss/cmd/zlib/makefile.win,v  <--  makefile.win
new revision: delete; previous revision: 1.1
done
Removing maketree.c;
/cvsroot/mozilla/security/nss/cmd/zlib/maketree.c,v  <--  maketree.c
new revision: delete; previous revision: 1.2
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/cmd/zlib/manifest.mn,v  <--  manifest.mn
new revision: 1.7; previous revision: 1.6
done
Checking in minigzip.c;
/cvsroot/mozilla/security/nss/cmd/zlib/minigzip.c,v  <--  minigzip.c
new revision: 1.4; previous revision: 1.3
done
Removing netscape_mods.doc;
/cvsroot/mozilla/security/nss/cmd/zlib/netscape_mods.doc,v  <--  netscape_mods.d
oc
new revision: delete; previous revision: 1.1
done
Checking in trees.c;
/cvsroot/mozilla/security/nss/cmd/zlib/trees.c,v  <--  trees.c
new revision: 1.4; previous revision: 1.3
done
Checking in uncompr.c;
/cvsroot/mozilla/security/nss/cmd/zlib/uncompr.c,v  <--  uncompr.c
new revision: 1.4; previous revision: 1.3
done
Checking in zconf.h;
/cvsroot/mozilla/security/nss/cmd/zlib/zconf.h,v  <--  zconf.h
new revision: 1.5; previous revision: 1.4
done
Removing zip16.def;
/cvsroot/mozilla/security/nss/cmd/zlib/zip16.def,v  <--  zip16.def
new revision: delete; previous revision: 1.1
done
Checking in zlib.h;
/cvsroot/mozilla/security/nss/cmd/zlib/zlib.h,v  <--  zlib.h
new revision: 1.4; previous revision: 1.3
done
Checking in zutil.c;
/cvsroot/mozilla/security/nss/cmd/zlib/zutil.c,v  <--  zutil.c
new revision: 1.4; previous revision: 1.3
done
Checking in zutil.h;
/cvsroot/mozilla/security/nss/cmd/zlib/zutil.h,v  <--  zutil.h
new revision: 1.5; previous revision: 1.4
done

Comment 22

[Wan-Teh Chang]

I added makefile rules to build the test programs
'example' and 'minigzip' and the makefile target
'test' to run the tests.

The test programs are not built by default.

Checking in Makefile;
/cvsroot/mozilla/security/nss/cmd/zlib/Makefile,v  <--  Makefile
new revision: 1.6; previous revision: 1.5
done
Checking in config.mk;
/cvsroot/mozilla/security/nss/cmd/zlib/config.mk,v  <--  config.mk
new revision: 1.7; previous revision: 1.6
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/cmd/zlib/manifest.mn,v  <--  manifest.mn
new revision: 1.8; previous revision: 1.7
done

Comment 23

[Wan-Teh Chang]

I verified that the OS2 porting change I mentioned in
comment 19, item 5 (to define the STDC macro in zconf.h)
is in zlib 1.2.2.

The DEBUG_ZLIB change described in comment 19, item 6
is intended to turn off all trace messages in debug
builds.  I verified (with the zlib authors) that the
best way to do this in zlib 1.2.2 is to compile with
the 'verbose' macro defined to be -1.

I plan to report two problems to the zlib maintainers.

1. Their definition of 'errno' as a global variable
for Windows CE is not thread safe.  (Windows CE has
threads.)

2. Their manual declaration of strerror in zutil.h
may conflict with the declaration of strerror in
<string.h> on some platforms (see comment 19, item
9).  The best solution is to ifdef the manual
declaration for the few platforms that have strerror
but do not declare it in <string.h>.  (strerror is
a standard C library function declared in <string.h>.)

Comment 24

[Wan-Teh Chang]

Attachment: Our local patch for zlib 1.2.2

This patch contains the changes we made to
the .c and .h files in zlib 1.2.2.  We removed
the definition of 'errno' as a global variable
for Win CE because it is not thread-safe.  Our
own Win CE port defines 'errno' as thread local
variable (a field in the PRThread structure).

Comment 25

[Wan-Teh Chang]

Attachment: Patch for the makefiles in nss/cmd/zlib

This patch contains the makefile changes necessary
to upgrade to zlib 1.2.2.  I also added makefile
rules to build and run the zlib test programs 'example'
and 'minigzip'.

I removed the obsolete code from the makefiles.

I define the 'verbose' macro as -1 to turn off all
zlib trace messages in debug builds.  (We turned off
the trace messages before.)

Comment 26

[Wan-Teh Chang]

Comment on attachment 164667 [details] [diff] [review]
Patch for the makefiles in nss/cmd/zlib

I'm attaching these two patches after the fact.
In the interest of time, I've already checked in
these patches.	I'd still like your code reviews.

Here is how I upgraded to zlib 1.2.2.

1. I copied the toplevel .c and .h files in zlib
1.2.2 to nss/cmd/zlib.	I didn't copy any of the
files in the subdirectories under zlib 1.2.2.

2. I removed the obsolete .c and .h files from
nss/cmd/zlib.

3. I applied our local patch to the .c and .h
files.

4. I modified our makefiles so they build the
zlib 1.2.2 library and test programs.

I only attached the patches for steps 3 and 4.

Comment 27

[Wan-Teh Chang]

Julien, please do a verification build of the NSS tip
on OS/2.

Comment 28

[Julien Pierre]

Wan-Teh,

The OS/2 build on the NSS tip worked fine with gcc 3.2.2.