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

Status: RESOLVED INCOMPLETE

(Core :: Security: PSM, defect)

Description

[Wan-Teh Chang]

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.

Comment 1

[Wan-Teh Chang]

Attachment: NSS patch (checked in)

The idea of my solution is to remove the unnecessary -I flags
for building NSS.

This patch is the NSS part of the fix.

Comment 2

[Wan-Teh Chang]

Attachment: PSM patch

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.

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Christophe Ravel]

Comment on attachment 459186 [details] [diff] [review]
NSS patch (checked in)

r+ Christophe

Comment 5

[Wan-Teh Chang]

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

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 7

[Wan-Teh Chang]

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.

Comment 8

[Wan-Teh Chang]

Attachment: Add preference security.ssl.enable_compression, default to false

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.

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Kai Engert [: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 11

[Kai Engert [:KaiE:]]

Comment on attachment 463156 [details] [diff] [review]
Add preference security.ssl.enable_compression, default to false

I agree this preference makes sense.

r=kaie

Comment 12

[Kai Engert [:KaiE:]]

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

Comment 13

[Adam Langley]

(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.

Comment 14

[Wan-Teh Chang]

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.

Comment 15

[Wan-Teh Chang]

(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.

Comment 16

[Wan-Teh Chang]

Attachment: Rollup patch

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.

Comment 17

[Wan-Teh Chang]

Comment on attachment 464546 [details] [diff] [review]
Rollup patch

http://hg.mozilla.org/mozilla-central/rev/1117034e2762

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: revert ZLIB change

r=bsmedberg (carried from bug 587000)

Comment 20

[Matt Brubeck (:mbrubeck)]

partial backout pushed: http://hg.mozilla.org/mozilla-central/rev/0b20b89a66cd

Comment 21

[Wan-Teh Chang]

(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?

Comment 22

[Wan-Teh Chang]

Attachment: Link libssl3.so with NSS's own zlib static library on all platforms

Ben, this patch implements solution #1 in comment 21.
Is this acceptable, or do you prefer another solution?
Thanks.

Comment 23

[Benjamin Smedberg]

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.

Comment 24

[Wan-Teh Chang]

Attachment: Link libssl3.so with NSS's own zlib static library on all platforms, v2

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.

Comment 25

[Wan-Teh Chang]

Attachment: Link libssl3.so with NSS's own zlib static library on all platforms, v2.1

The only change is to quote $(ZLIB_LIBS) because it may contain
spaces.

Comment 26

[Wan-Teh Chang]

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.

Comment 27

[Benjamin Smedberg]

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.

Comment 28

[Wan-Teh Chang]

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.

Comment 29

[Wan-Teh Chang]

Attachment: Remove preference security.ssl.enable_compression from mozilla 2.0

Ben, please approve either this patch (to fully revert)
or attachment 467652 [details] [diff] [review] for mozilla 2.0.  Thanks.

Comment 30

[Wan-Teh Chang]

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

Comment 31

[Mike Hommey [:glandium]]

(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?

Comment 32

[Daira Hopwood]

(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.

Comment 33

[Mike Hommey [:glandium]]

(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.

Comment 34

[Mike Hommey [:glandium]]

(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 :/

Comment 35

[Daira Hopwood]

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?

Comment 36

[Dave Garrett]

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?)

Comment 37

[:Cykesiopka]

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.