65.64 KB, text/plain
26.47 KB, text/html
163.95 KB, text/plain
648 bytes, patch
Nelson Bolyard (seldom reads bugmail): review+
|Details | Diff | Splinter Review|
2.99 KB, patch
Nelson Bolyard (seldom reads bugmail): review+
|Details | Diff | Splinter Review|
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.
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
Target Milestone: --- → Future
Actually, we should consider doing this in NSS 3.6.
due to the security bug in zlib < 1.1.4 zlib should be upgraded!
*** Bug 185479 has been marked as a duplicate of this bug. ***
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
firstname.lastname@example.org 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.
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.
Priority: -- → P2
Target Milestone: Future → 3.8
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.
Created attachment 109579 [details] 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 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?
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
Created attachment 109589 [details] differences between trunk and zlib 1.0.4 Here are the context diffs.
Attachment #109579 - Attachment is obsolete: true
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
Created attachment 113896 [details] 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.
Created attachment 113897 [details] 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)
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
Zlib version 1.2.1 was released a couple of weeks ago. It is supposed to run significantly faster than 1.1.4.
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>.
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?
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.10
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
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>.)
Created attachment 164666 [details] [diff] [review] 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).
Created attachment 164667 [details] [diff] [review] 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 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.
Attachment #164667 - Flags: review?(nelson)
Julien, please do a verification build of the NSS tip on OS/2.
Wan-Teh, The OS/2 build on the NSS tip worked fine with gcc 3.2.2.
Attachment #164666 - Flags: review?(nelson) → review+
Attachment #164667 - Flags: review?(nelson) → review+
You need to log in before you can comment on or make changes to this bug.