Closed Bug 580679 Opened 14 years ago Closed 8 years ago

Build NSS with the TLS zlib compression code and add the security.ssl.enable_compression preference to enable it.

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE
Future

People

(Reporter: wtc, Assigned: wtc)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [approved-patches-landed][not-ready-for-cedar])

Attachments

(7 files, 2 obsolete files)

This is the remaning work of NSS bug 275744.

Because Mozilla's zlib.h renames the zlib functions,
it is tricky to compile NSS against Mozilla's zlib.h
and link with the system zlib library.  This forces
Mozilla to build NSS with NSS_ENABLE_ZLIB unset.

There are multiple ways to fix this problem.  The
best solution is for Mozilla and NSS to use Mozilla's
zlib as a shared library.

I came up with a less ideal solution that's easier
to implement -- when Mozilla builds NSS, it should
not specify a -I <directory> flag where <directory>
contains Mozilla's headers.  I will attach my patch
when I have tested it on Linux and Windows.

After I fix this build issue, I will then write a
patch to actually enable TLS compression in Mozilla.
I may do that work in a separate bug.
The idea of my solution is to remove the unnecessary -I flags
for building NSS.

This patch is the NSS part of the fix.
Attachment #459186 - Flags: superreview?(kaie)
Attachment #459186 - Flags: review?(ted.mielczarek)
Attached patch PSM patchSplinter Review
This patch is the PSM part of the fix.

In a Mozilla build, NSPR headers are published to both
dist/include/nspr and dist/include.

The problematic zlib.h header from Mozilla is in dist/include.
The sqlite3.h header is also in dist/include.

The idea of the fix is for NSS to
- get the NSPR headers from dist/include/nspr
- do not use dist/include, except when compiling nss/lib/softoken,
  which needs sqlite3.h, but not zlib.h, fortunately.

I added the SQLITE_INCLUDE_DIR variable to specify an alternative
directory to get sqlite3.h from.  If you don't like that, I can
simply change -I$(DIST)/include/sqlite3 to -I$(DIST)/include.
Attachment #459188 - Flags: superreview?(kaie)
Attachment #459188 - Flags: review?(ted.mielczarek)
Comment on attachment 459186 [details] [diff] [review]
NSS patch (checked in)

Christophe, please review this patch.  Thanks.

1. Remove unnecessary -I flags from the NSS makefiles.
Mozilla is no longer using the MOZILLA_INCLUDES variable:
http://mxr.mozilla.org/mozilla-central/search?string=MOZILLA_INCLUDES
http://mxr.mozilla.org/mozilla1.9.2/search?string=MOZILLA_INCLUDES

2. Add SQLITE_INCLUDE_DIR for specifying the location of
the sqlite3.h header, and use this only in the directory
that needs sqlite3.h, lib/softoken.

3. sdb.h does not need to include sqlite3.h.
Attachment #459186 - Flags: review?(ted.mielczarek) → review?(christophe.ravel.bugs)
Comment on attachment 459186 [details] [diff] [review]
NSS patch (checked in)

r+ Christophe
Attachment #459186 - Flags: review?(christophe.ravel.bugs) → review+
Comment on attachment 459186 [details] [diff] [review]
NSS patch (checked in)

I checked in this patch on the NSS trunk (NSS 3.13) and
NSS_3_12_BRANCH (NSS 3.12.8).

Checking in coreconf/headers.mk;
/cvsroot/mozilla/security/coreconf/headers.mk,v  <--  headers.mk
new revision: 1.10; previous revision: 1.9
done
Checking in nss/lib/softoken/manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/manifest.mn,v  <--  manifest.mn
new revision: 1.37; previous revision: 1.36
done
Checking in nss/lib/softoken/sdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.h,v  <--  sdb.h
new revision: 1.5; previous revision: 1.4
done
Checking in nss/lib/softoken/legacydb/manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/manifest.mn,v  <--  manifest.mn
new revision: 1.7; previous revision: 1.6
done

Checking in coreconf/headers.mk;
/cvsroot/mozilla/security/coreconf/headers.mk,v  <--  headers.mk
new revision: 1.9.20.1; previous revision: 1.9
done
Checking in nss/lib/softoken/manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/manifest.mn,v  <--  manifest.mn
new revision: 1.36.22.1; previous revision: 1.36
done
Checking in nss/lib/softoken/sdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.h,v  <--  sdb.h
new revision: 1.4.22.1; previous revision: 1.4
done
Checking in nss/lib/softoken/legacydb/manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/manifest.mn,v  <--  manifest.mn
new revision: 1.5.68.1; previous revision: 1.5
done
Attachment #459186 - Attachment description: NSS patch → NSS patch (checked in)
Attachment #459186 - Flags: superreview?(kaie)
Attachment #459188 - Flags: superreview?(kaie) → superreview?(me)
Comment on attachment 459188 [details] [diff] [review]
PSM patch

I'm not an sr, but I assume you're just using that to ask for two reviews.  r=me.
Attachment #459188 - Flags: superreview?(me) → superreview+
Comment on attachment 459188 [details] [diff] [review]
PSM patch

I'm requesting approval2.0 for this patch.

The benefit of the patch is that Mozilla will build
NSS the same way the NSS team builds NSS, benefitting
fully from the QA done by the NSS team.

The compatibility/regression risk is zero since this
patch does NOT enable TLS compression.  It merely
builds the code.
Attachment #459188 - Flags: approval2.0?
To review this patch, compare it with a similar patch:
http://hg.mozilla.org/mozilla-central/rev/5a649ed4ab65

The default value of the security.ssl.enable_compression
preference is false (disabled) because we encountered a
few servers with broken implementations of TLS
compression.  Until I implement the workaround (detect
SSL errors that signal a broken server and retry with
TLS compression disabled), the default should be false.

The need to set clearSessionCache to true when the
security.ssl.enable_compression preference changes
comes from TLS 1.0 RFC 2246, Section 7.4.1.2
"Client hello", page 36:
   compression_methods
       This is a list of the compression methods supported by the
       client, sorted by client preference. If the session_id field is
       not empty (implying a session resumption request) it must include
       the compression_method from that session. ...

So compression method is part of the session state.

Note that this preference controls only the DEFLATE
(zlib) compression method.  We could name the preference
"enable_deflate_compression" to make it clear that
it only controls the DEFLATE compression method.  But
since DEFLATE is essentially the only TLS compression
method available, I don't want to over-engineer it.
(The only other TLS compression method is "LZS", see
the IANA registry at
http://www.iana.org/assignments/comp-meth-ids/comp-meth-ids.xhtml.)
I'll be happy to change the preference name to indicate
it's specific to the DEFLATE compression method, to be
future-proof.
Attachment #463156 - Flags: review?
Attachment #463156 - Flags: review? → review?(sayrer)
Attachment #459188 - Flags: review?(ted.mielczarek)
Comment on attachment 463156 [details] [diff] [review]
Add preference security.ssl.enable_compression, default to false

Please see comment 8 for a description of this patch.
Attachment #463156 - Flags: review?(kaie)
Comment on attachment 459188 [details] [diff] [review]
PSM patch

+DEFAULT_GMAKE_FLAGS += SOURCE_MDHEADERS_DIR=$(NSPR_INCLUDE_DIR)

Can you please say which code requires this?
I can't find this variable anywhere in NSS?


-# Turn off TLS compression support because NSS 3.12.6 can't be built
-# with Mozilla's zlib.h.  See bug 527659 comment 10.
-DEFAULT_GMAKE_FLAGS += NSS_ENABLE_ZLIB=


We disabled that code, because it broken the tinderbox.

I guess it would make sense to run a tryserver build with 3.12.8 and this patch before checking in?
Comment on attachment 463156 [details] [diff] [review]
Add preference security.ssl.enable_compression, default to false

I agree this preference makes sense.

r=kaie
Attachment #463156 - Flags: review?(kaie) → review+
Attachment #459188 - Flags: approval2.0? → approval2.0+
I did a tryserver build with patches 2 and 3 from this bug, it failed on the maemo (mobile firefox) platform, see http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281031100.1281034084.20012.gz
(In reply to comment #12)
> I did a tryserver build with patches 2 and 3 from this bug, it failed on the
> maemo (mobile firefox) platform

It indicates that libz isn't installed on that builder rather than that there's an issue with the patch.
On the Maemo tryserver slaves, we need to use the -rpath-link
linker flag.  Mozilla's .mozconfig file contains this line:
    export LDFLAGS='-Wl,-rpath-link,/usr/lib:/lib'

It is not possible to pass extra linker flags to NSS's build
system.  This is a known weakness of the NSS build system.

I'll implement a workaround, which is to explicitly specify
indirect library dependency -lz.
Attachment #463156 - Flags: review?(sayrer) → review+
Depends on: 585247
(In reply to comment #10)

SOURCE_MDHEADERS_DIR is used by NSS:
http://mxr.mozilla.org/security/search?string=SOURCE_MDHEADERS_DIR

SOURCE_MDHEADERS_DIR is the directory for
machine-dependent (MD) headers.  For example, on
Windows it is mozilla/dist/WIN954.0_DBG.OBJ/include.
It is only used by NSPR.  This is why I set
+DEFAULT_GMAKE_FLAGS += SOURCE_MDHEADERS_DIR=$(NSPR_INCLUDE_DIR)

In a Mozilla build, NSPR headers are published to
both mozilla/dist/include and mozilla/dist/include/nspr.
The former also has the patched zlib headers.  My patch
tells NSS to use mozilla/dist/include/nspr (NSPR_INCLUDE_DIR)
instead to avoid picking up the patched zlib headers.
Status: NEW → ASSIGNED
Attached patch Rollup patchSplinter Review
Requesting approval2.0.  This rollup patch combines two
patches in this bug (attachment 459188 [details] [diff] [review] and attachment 463156 [details] [diff] [review])
and one patch in bug 585247 (attachment 463750 [details] [diff] [review]).

The benefit of the patch is that Mozilla will build
NSS the same way the NSS team builds NSS, benefitting
fully from the QA done by the NSS team.  The new
security.ssl.enable_compression will allow interested
users to experiment with TLS compression.

The compatibility/regression risk is zero for most
users because TLS compression is disabled by default.
Only the users who enable the security.ssl.enable_compression
preference will run a small risk of website
incompatibility.
Attachment #464546 - Flags: approval2.0?
Attachment #464546 - Flags: approval2.0? → approval2.0+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Build NSS with the TLS zlib compression code. → Build NSS with the TLS zlib compression code and add the security.ssl.enable_compression preference to enable it.
Target Milestone: mozilla2.0b1 → mozilla2.0b4
This patch appears to have made Linux builds of mozilla-NSS depend on system zlib. Mozilla has made a conscious decision *not* to use system zlib. I think we should back out the last bit of this patch which actually sets NSS_ENABLE_ZLIB.
r=bsmedberg (carried from bug 587000)
Attachment #465742 - Flags: review+
partial backout pushed: http://hg.mozilla.org/mozilla-central/rev/0b20b89a66cd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #18)
> This patch appears to have made Linux builds of mozilla-NSS depend on system
> zlib. Mozilla has made a conscious decision *not* to use system zlib. I think
> we should back out the last bit of this patch which actually sets
> NSS_ENABLE_ZLIB.

Ben,

I can implement any of the following solutions to the NSS zlib build
issue.  Please let me know which one is acceptable to you.

1. (Simplest patch): link libssl3.so with NSS's own zlib static library
on all platforms.  NSS's zlib (mozilla/security/nss/lib/zlib) is
essentially the same as Mozilla's zlib.

2. Link libssl3.so with Mozilla's zlib static library on all platforms.

3. Build Mozilla's zlib as a shared library and share it between
Mozilla and NSS (libssl3.so).

Thank you.

Re: Mozilla's conscious decision *not* to use system zlib:

mozilla/modules/zlib/src contains zlib 1.2.3, which was released five
years ago in July 2005.  The system zlib in current versions of any OS
should be the same or newer than Mozilla's zlib.  Is there something
wrong with zlib 1.2.4 ro 1.2.5?
Ben, this patch implements solution #1 in comment 21.
Is this acceptable, or do you prefer another solution?
Thanks.
Attachment #467243 - Flags: review?(benjamin)
Comment on attachment 467243 [details] [diff] [review]
Link libssl3.so with NSS's own zlib static library on all platforms

I'm fine with the general approach, but we should probably honor the MOZ_NATIVE_ZLIB setting, http://mxr.mozilla.org/mozilla-central/source/config/autoconf.mk.in#232

So that the Linux distributions which do maintain their system libs in lockstep get the system zlib they expect.

That's probably something like:

ifdef MOZ_NATIVE_ZLIB
DEFAULT_GMAKE_FLAGS += USE_SYSTEM_ZLIB=1 ZLIB_LIBS=$(ZLIB_LIBS)
else
DEFAULT_GMAKE_FLAGS += USE_SYSTEM_ZLIB=
endif

Although that should probably be tested.
Attachment #467243 - Flags: review?(benjamin) → review-
Ben: thanks a lot for the suggestion.  What you wrote
should work as long as ZLIB_CFLAGS is empty, which is
true for all system zlibs I know of (zlib.h is installed
in /usr/include).

I also combined another zlib-related NSS makefile setting
(PROGRAMS=) in this patch.
Attachment #467243 - Attachment is obsolete: true
Attachment #467646 - Flags: review?(benjamin)
The only change is to quote $(ZLIB_LIBS) because it may contain
spaces.
Attachment #467646 - Attachment is obsolete: true
Attachment #467652 - Flags: review?(benjamin)
Attachment #467646 - Flags: review?(benjamin)
Attachment #467652 - Flags: review?(benjamin) → review+
Comment on attachment 467652 [details] [diff] [review]
Link libssl3.so with NSS's own zlib static library on all platforms, v2.1

Requesting approval2.0 of an improved version of a previously
approved patch.
Attachment #467652 - Flags: approval2.0?
Comment on attachment 467652 [details] [diff] [review]
Link libssl3.so with NSS's own zlib static library on all platforms, v2.1

We're past FF4 feature freeze, and since this is preffed off by default anyway, I don't think we really need or want to take it for FF4 now. Please renominate if I'm wrong, and let's get this landed after branching.
Attachment #467652 - Flags: approval2.0? → approval2.0-
Comment on attachment 467652 [details] [diff] [review]
Link libssl3.so with NSS's own zlib static library on all platforms, v2.1

bsmedberg: it's fine by me to not take this patch for
FF4.  In that case, I'd like to remove the
security.ssl.enable_compression preference from FF4 to
avoid confusion.  Please advise what you want me to do
in FF4.  Thanks.
Ben, please approve either this patch (to fully revert)
or attachment 467652 [details] [diff] [review] for mozilla 2.0.  Thanks.
Attachment #471374 - Flags: review?(benjamin)
Attachment #471374 - Flags: approval2.0?
Attachment #471374 - Flags: review?(benjamin)
Attachment #471374 - Flags: review+
Attachment #471374 - Flags: approval2.0?
Attachment #471374 - Flags: approval2.0+
Comment on attachment 471374 [details] [diff] [review]
Remove preference security.ssl.enable_compression from mozilla 2.0

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b5538db7d5eb
Target Milestone: mozilla2.0b4 → Future
Whiteboard: [approved-patches-landed]
Whiteboard: [approved-patches-landed] → [approved-patches-landed][not-ready-for-cedar]
(In reply to comment #28)
> Comment on attachment 467652 [details] [diff] [review] [diff] [details] [review]
> Link libssl3.so with NSS's own zlib static library on all platforms, v2.1
> 
> bsmedberg: it's fine by me to not take this patch for
> FF4.  In that case, I'd like to remove the
> security.ssl.enable_compression preference from FF4 to
> avoid confusion.  Please advise what you want me to do
> in FF4.  Thanks.

Can we land this, now?
(In reply to Mike Hommey [:glandium] from comment #31)
> Can we land this, now?

No, because of http://lists.randombit.net/pipermail/cryptography/2012-September/003191.html . It would be prudent to at least wait until the paper on CRIME is published to see what the damage is.
(In reply to David-Sarah Hopwood from comment #32)
> (In reply to Mike Hommey [:glandium] from comment #31)
> > Can we land this, now?
> 
> No, because of
> http://lists.randombit.net/pipermail/cryptography/2012-September/003191.html
> . It would be prudent to at least wait until the paper on CRIME is published
> to see what the damage is.

Note the question was about a patch that only changes how zlib is linked in nss.
(In reply to Mike Hommey [:glandium] from comment #33)
> (In reply to David-Sarah Hopwood from comment #32)
> > (In reply to Mike Hommey [:glandium] from comment #31)
> > > Can we land this, now?
> > 
> > No, because of
> > http://lists.randombit.net/pipermail/cryptography/2012-September/003191.html
> > . It would be prudent to at least wait until the paper on CRIME is published
> > to see what the damage is.
> 
> Note the question was about a patch that only changes how zlib is linked in
> nss.

Forget it, too early in the morning, the patch actually enables zlib :/
So, it looks like the CRIME attack is pretty fatal for the whole idea of TLS compression. Should this ticket be closed, or is there still some advantage in changing how NSS is linked even if zlib isn't going to be used?
HTTP/2 will ban TLS compression and TLS 1.3 removes it from the spec (as of the current draft). I assume people just forgot about this bug ticket. Should this just be WONTFIXed or is there some other work that needs doing? (or is there still a desire to do this for some reason?)
Flags: needinfo?(wtc)
Resolving as INCOMPLETE:
 - AFAICT, the upstream NSS patches linked to this bug did not get backed out.
 - There was a ZLIB_INTERNAL change that wasn't backed out.
 - Can't really say "fixed", since those changes were meant to facilitate enabling compression and none of the compression patches have stuck.

It seems considerably unlikely that TLS compression would be desired now due the issues mentioned in comment 35 and comment 36 anyways.
Status: REOPENED → RESOLVED
Closed: 14 years ago8 years ago
Flags: needinfo?(wtc)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: