275744 - Support for TLS compression RFC 3749

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Chris Newman]

When encryption is used, it defeats all compression facilities subsequent to the
encryption; thus all link-level encryption (e.g. PPP) and other facilities
become useless.  So compression needs to occur prior to encryption and this
standard is ideal for that purpose.

A way to implement this without creating a hard dependency on zlib would be to
have the caller of NSS pass in function pointers to the necessary zlib entry points.

Comment 1

[Matt Bernstein]

I'd love to see this--it'd really help IMAP header retrievals on large
mailboxes, especially over thin links like wireless.

Surely zlib already exists somewhere in Mozilla, given that it's possible to
grab HTTP Content-Transfer-Encoding: application/gzip ?

Comment 2

[Josh Birnbaum]

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

Comment 3

[Matt Bernstein]

Don't think this is specific to NSS 3.9.3. I do think that the talk of dropping
SSLv2 should help this bug; I believe it's possible to do an SSLv3/TLSv1
handshake and ask for compression iff the server supports it, while you can't do
this with an SSLv2/SSLv3/TLSv1 handshake.

I like this to be included in 1.9, assuming it's too late for 1.8..

Comment 4

[Wan-Teh Chang]

We don't plan to implement TLS compression in NSS 3.11
or NSS 3.12 because it is less important than the other
work we have planned. If someone contributes a patch, we
will be happy to review it and check it in.

Comment 5

[Chris Newman]

FYI, the there is now a proposal in the IETF to add compression to IMAP at the layer above TLS:

  https://datatracker.ietf.org/public/idindex.cgi?command=id_detail&id=14490

Here's the rationale:

    - COMPRESS can be implemented easily by IMAP servers and clients.
      At present, TLS compression is not widely implemented.  In the
      LEMONADE WG, the general consent is that libraries implementing
      TLS compression will not be available soon enough for LEMONADE.

I observe that failure to deploy TLS compression is resulting in additional protocol complexity.  However, the "LEMONADE" WG is focusing on mobile email extensions to Internet email, so it's actually the TLS stacks in cell phones and whatnot that are particularly problematic.

If I hear about someone implementing COMPRESS in an NSS+IMAP product, I'll encourage them to contribute patches to NSS at the same time.

FYI, an implementation which only provided TLS compression if /usr/lib/libz.so was present on the OS would be acceptable to me.

Comment 6

[Adam Langley]

Attachment: patch

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Adam,  Please provide your patch as a CVS diff against the CVS trunk.
Be aware that Mozilla's Hg source repository is a downstream repository
which is generally NOT up to date with the latest CVS upstream repository.
We only accept patches that apply cleanly to the upstream CVS repository.

Comment 8

[Adam Langley]

Attachment: patch v2

The previous patch applied cleanly against the CVS head, but have used cvs diff -u8p this time. I hope that's more correct.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405184 [details] [diff] [review]
patch v2

Thanks.  You can ask me to review any libSSL patches.
I'll review this one.  

At first glance, it looks to me like your patch will only build on Linux and maybe Unix, but not Windows. (Not sure about Mac OSX).  
If I'm right, that will be a problem.  But let me do more review before you revise your patch any.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405184 [details] [diff] [review]
patch v2

This doesn't compile on Windows.  This patch assumes C++/C99 style variable declarations are allowed, but not all platforms supported by NSS have C99 compilers, so you cannot declare variables in the middle of basic blocks.  That problem is so pervasive in this patch, that there's really not much point in me reviewing it until all those are fixed.  Fortunately, this is all very easy to fix.  But there are a lot of places where the fix is needed.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: errors from attempt to build this patch on Windows

This is the build log for Windows.  
All the fatal build errors were in ssl3con.c.  
When these are all fixed, I imagine the next rev of the patch will get much
farther.  
For the next patch, you can just use cvs diff -Npu .  No need to specify 8
lines of context.  Bugzilla's patch review tool will let me see all the 
context I want for CVS diffs (and only for those).

Comment 12

[Adam Langley]

Attachment: patch v3

I tried building ssl3con.c with -ansi -pedantic -Wall and addressed the three places where I was declaring variables within a block. Sorry about that. Patch updated.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405307 [details] [diff] [review]
patch v3

Unique patch descriptions help.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405307 [details] [diff] [review]
patch v3

12345678901234567890123456789012345678901234567890123456789012345678901234567890
The good news is that this patch builds on windows, PROVIDED that 
USE_ZLIB is NOT defined.  It does not build when USE_ZLIB is defined,
because it doesn't link with any zlib library on that platform. 
The patch will not be able to be committed until the USE_ZLIB feature
builds and works on all supported platforms.  So, that's effectively
an r-, but I'm not going to give this patch a review result (r+, r-) 
until I've given it a more thorough review.  

Have you tested this patch with NSS's ssl.sh test script?  Does it pass?

Here are a few other preliminary review comments.

>+#ifdef NSS_ENABLE_ZLIB
>+#include <zlib.h>

that should be #include "zlib.h", because zlib.h is an NSS header file,
at least on platforms that do not have a platform native zlib.

> static const /*SSL3CompressionMethod*/ uint8 compressions [] = {
>-    compression_null
>+    compression_null,
>+#ifdef NSS_ENABLE_ZLIB
>+    compression_deflate,
>+#endif
> };

I need to check and make sure that the client doesn't advertise deflate 
compression when it's been compiled time enabled but not run time enabled.


>+#ifdef NSS_ENABLE_ZLIB
>+/*
>+ * The DEFALTE algorithm can result in an expansion of 0.1% + 12 bytes. For a

DEFALTE?  


>@@ -8225,7 +8420,6 @@ const ssl3BulkCipherDef *cipher_def;
> 
>     ssl_GetSpecReadLock(ss); /******************************************/
> 
>-    crSpec = ss->ssl3.crSpec;
>     cipher_def = crSpec->cipher_def;

In function ssl3_HandleRecord, you moved a line of code to a place much
earlier in the function.  That line is:
    crSpec = ss->ssl3.crSpec;

the specs (and even the pointers to the specs) must only be accessed while
holding the appropriate spec lock.  The place from which that line was moved
was immediately following an acquisition of the relevant spec read lock.
Now, that spec is being accessed while unlocked.  That must be fixed.

The patch adds a few lines whose length is longer than 80 columns. 
Please wrap those long lines.  Our coding standard disallows them.
Here are some (a few) of them.

>+    /* possibly decompress the record. If we aren't using compression then
>+     * plaintext == databuf and so the uncompressed data is already in 
databuf. */
>+    if (crSpec->decompress) {
>+	if (databuf->space < plaintext->len + SSL3_COMPRESSION_MAX_EXPANSION) {
>+	    rv = sslBuffer_Grow(databuf, plaintext->len + 
SSL3_COMPRESSION_MAX_EXPANSION);
>+	    if (rv != SECSuccess) {
>+		SSL_DBG(("%d: SSL3[%d]: HandleRecord, tried to get %d bytes",
>+			 SSL_GETPID(), ss->fd, plaintext->len + 
SSL3_COMPRESSION_MAX_EXPANSION));
>+		/* sslBuffer_Grow has set a memory error code. */
>+		/* Perhaps we should send an alert. (but we have no memory!) */

More comments in a full review later.  
I'll be on vacation for 4 days starting tomorrow (Friday).

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Oh, one more preliminary review comment that I meant to include...

> static const /*SSL3CompressionMethod*/ uint8 compressions [] = {
>-    compression_null
>+    compression_null,
>+#ifdef NSS_ENABLE_ZLIB
>+    compression_deflate,
>+#endif
> };

If NSS_ENABLE_ZLIB is not defined, the trailing comma on compression_null
will produce a compilation error on some platforms (e.g. HPUX).

Comment 16

[Adam Langley]

Attachment: Patch v4, Updated to address some of the comments from the preliminary review.

I'm going to ask wtc about building with zlib on Windows and about adding the tests. Otherwise, I hope that I've addressed your preliminary comments with this update.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406498 [details] [diff] [review]
Patch v4, Updated to address some of the comments from the preliminary review.

I'm back from vacation now and will review this new revision of the patch in more depth.  
I'll ignore the Windows-specific issues for now.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406498 [details] [diff] [review]
Patch v4, Updated to address some of the comments from the preliminary review.

I've completed my review.

1. This patch ensures that, when sending the client hello, we only
advertise the zlib compression method if it is run-time enabled.  
However, it does not do either of the following:

a) When processing a received client hello in the server, check to 
ensure that the zlib compression method is run-time enabled before 
accepting and using that method, or

b) When processing the Server hello, checking to ensure that the
zlib compression is run time enabled before using it if the server
chooses it.  

This means that 
a) if the client asks to use zlib, and it has been compiled into 
the server, it will be used in the server whether it has been 
enabled or disabled by the server (using the socket option), an d

b) even if the client does not advertise the zlib compression method,
if it has been compiled in, and the server tells the client to use 
it, the client will use it.  This is a vulnerability.

2. Please get rid of this comment in the existing code, which is 
obsoleted by your new code:

1843 /*
1844  * null compression is easy to do
1845 PORT_Memcpy(wrBuf->buf + SSL3_RECORD_HEADER_LENGTH, pIn, contentLen);
1846  */

3. memset is too inefficient here.

>+    /* If we will be decompressing the buffer we need to decrypt somewhere
>+     * other than into databuf */
>+    if (crSpec->decompress) {
>+	memset(&temp_buf, 0, sizeof(temp_buf));
>+	plaintext = &temp_buf;
>+    } else {
>+	plaintext = databuf;
>+    }
>+
>+    plaintext->len = 0; /* filled in by decode call below. */

temp_buf is a struct with just 3 members, one of which will be set to zero 
below.  memset is VERY inefficient for use with structures/buffers below a 
certain size.  Please just explicitly initialize the other two members.

4. Some of the new lines appear to exceed 80 columns in length.
Here are some examples:

> 	         padding_length != 
>-	                 *(databuf->buf + databuf->len - (padding_length + 1)))
>+	                 *(plaintext->buf + plaintext->len - (padding_length + 1)))
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

>-	PORT_Memcmp(databuf->buf + databuf->len, hash, crSpec->mac_size) != 0) {
>+	PORT_Memcmp(plaintext->buf + plaintext->len, hash, crSpec->mac_size) != 0) {
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Please adjust them to fit within 80 columns.

5. Some of the new functions don't follow the NSS indentation style.
ssl3_DestroyCompressContext
ssl3_DestroyDecompressContext

6. I think we should invent a new SSL error code for compression/decompression
failure, rather than reusing SSL_ERROR_DECRYPTION_FAILURE for this purpose.
But I'm not adamant about it.  

These issues, plus the zlib on Windows issue, are the only remaining issues.

Comment 19

[Adam Langley]

Attachment: Patch v5, Updated to address comments and to add tests

Updating patch with tests and fixes for review comments. Not marking r? at this point because I still have to get Windows working somehow.

wtc suggested that the -z flag to the test programs should take an argument in case additional compression options are added to TLS. However, I haven't actually done this because -z is universally taken to be 'zlib' and it never takes an argument anywhere else. However, it's easy to do if anyone feels strongly.

Comment 20

[Wan-Teh Chang]

Attachment: Makefile changes if nss/cmd/zlib is moved to nss/lib/zlib (checked in)

This patch contains the necessary makefile changes if we
move nss/cmd/zlib to nss/lib/zlib.

Since we build nss/lib before nss/cmd, we must move our
zlib from nss/cmd to nss/lib if nss/lib/ssl is to use
our zlib.

Once this is done, it just takes some more changes to
the makefiles in nss/lib/ssl to link libssl3.so/ssl3.dll
with our zlib as a static library.

I just inspected a Firefox 3.5.3 installation on Windows.
It doesn't contain zlib as a DLL.  So more work will be
required if we want Firefox and NSS to share zlib as a
DLL.

Nelson, do you want to proceed with the plan of moving
our zlib from nss/cmd to nss/lib?  Do you want to do a
straightforward "cvs remove" plus "cvs add", or do you
want to file a ticket to have the directory moved in
the CVS repository with CVS history preserved?

I suggest that we first check in a version of Adam's
patch that supports deflate compression only on platforms
that have system zlib, while we sort out what we should
do about the other platforms.

Other options to deal with zlib dependency:
1. have the caller of NSS pass in function pointers to the
necessary zlib entry, as Chris Newman suggested in comment 0.
2. have NSS load libz.so/zlib.dll and look up the function
pointers at run time.

Comment 21

[Wan-Teh Chang]

Comment on attachment 408972 [details] [diff] [review]
Makefile changes if nss/cmd/zlib is moved to nss/lib/zlib (checked in)

Since our zlib is simply the upstream zlib plus our makefiles,
it should be OK to move it without preserving CVS history.
I'm willing to file a ticket if that's the right thing though.

Comment 22

[Wan-Teh Chang]

Attachment: nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib

Assuming our zlib is moved to nss/lib, we can change
the nss/lib/ssl makefiles to use either the system zlib
or nss/lib/zlib, depending on whether USE_SYSTEM_ZLIB
is defined in mozilla/security/coreconf for the platform.

When using nss/lib/zlib, this patch links libssl3.so/ssl3.dll
with zlib as a static library.  This is not ideal but
is easier to implement.

Comment 23

[Wan-Teh Chang]

Attachment: nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v2

The previous version of this patch doesn't work on Linux.

Comment 24

[Wan-Teh Chang]

Attachment: Patch for ssltap to pretty-print compression methods (checked in)

This patch also cleans up the code for pretty-printing
cipher suites, so it's larger than absolutely necessary.

Comment 25

[Wan-Teh Chang]

Comment on attachment 409463 [details] [diff] [review]
Patch for ssltap to pretty-print compression methods (checked in)

With my patch, ssltap prints the compression methods as follows:

In client hello:

            compression[2] = {
                (00) null
                (01) DEFLATE
            }

In server hello:

            compression method = (00) null
or
            compression method = (01) DEFLATE

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Test servers for compression.  Up occasionally.  Certs may be bogus.
https://doc.ic.ac.uk/csg and https://www.zooko.com
Thanks to Adam for these.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Adam's Patch v5 part 1, comparable to Adam's Patch v4

Bugzilla's patch reviewing tools have the ability to review diffs between 
two patches, but ONLY if those two patches patch exactly the same set of 
files, and all the files being patched in both patches are of the same CVS
version numbers.  In this case, Patch v5 patches 4 more files than patch v4,
so I split it into two patches, part 1 (attached here) which bugzilla should
be able to compare with patch v4, and part 2 (to be attached next) which 
obviously will not be.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Adam's Patch v5 part 2

Here's part 2, for completeness.  Sadly, bugzilla still will not compare 
part 1 with patch v4, claiming that the version numbers of some files do 
not match.  :(

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Adam's patch v4 brought forward to tip of trunk for comparison to v5 part 1

Call this patch v4 redux

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 408972 [details] [diff] [review]
Makefile changes if nss/cmd/zlib is moved to nss/lib/zlib (checked in)

Yes, Let's do this in any case.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 409463 [details] [diff] [review]
Patch for ssltap to pretty-print compression methods (checked in)

r=nelson
Thanks for the additional cleanup.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 408139 [details] [diff] [review]
Patch v5, Updated to address comments and to add tests

Ok, this is extremely close.  I have a few nits, two issues,
one of which is a new one, and other comments.

>+"-z mean enable compression.\n"

How nice that 'z' was available.  It's perfect for this feature!

>-    optstate = PL_CreateOptState(argc, argv, "23BC:DNP:TUW:c:d:f:in:op:qst:uvw:");
>+    optstate = PL_CreateOptState(argc, argv, "23BC:DNP:TUW:c:d:f:in:op:qst:uvzw:");

This line was already too long and needed to be wrapped, but now even 
more so.  Also, please keep the option letters in alphabetical order.
Make that end with "uvw:z" rather than as you proposed.  If this had 
been the only issue, I would just have silently fixed in at check in.

>-    optstate = PL_CreateOptState(argc, argv, "23BTSfc:h:p:d:m:n:oqr:suvw:xW:");
>+    optstate = PL_CreateOptState(argc, argv, "23BTSfc:h:p:d:m:n:oqr:suvw:xzW:");

This line now just fits in 80 columns, so it's OK length wise. You added the 
z in the right place.  This string obviously had a bunch of alphabetical order
problems.  Since you're changing it, let me ask you to fix them, making the 
string be "23BSTW:c:d:fh:m:n:op:qr:suvw:xz"


>Index: mozilla/security/nss/lib/ssl/Makefile

>+ifdef USE_ZLIB
>+DEFINES += -DNSS_ENABLE_ZLIB
>+EXTRA_SHARED_LIBS += -lz
>+endif

This is one of the two issues.  I think we want to have the option to be 
able to link with the system zlib on systems that have it, and to link
with NSS's static zlib on systems that don't have one.  I think we're 
almost there.  Wan-Teh's patch shows that we can easily link with static
zlib on Windows, and the patch above shows we can easily do it with 
system zlib on Linux, so it's just a little more makefile work. 

To avoid complexity, I'd suggest having the default always be to use 
the static zlib on all platforms, and let the user define an environment
variable USE_SYSTEM_LIBZ if he wants to do that instead.

Wan-Teh, Thanks for figuring out how to make that work on Windows.
I'm glad it was less work than I had imagined.

>+/* compressionEnabled returns true iff the compression algorithm with index i
>+ * is enabled for the given SSL socket. */
>+static char compressionEnabled(int i, sslSocket *ss) {

in libSSL, all functions that take a sslSocket* argument always take it as 
the first argument.  So, please switch the order of these arguments here
and in the callers.  



>Index: mozilla/security/nss/lib/ssl/ssl3prot.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v
>retrieving revision 1.13
>diff -p -u -r1.13 ssl3prot.h
>--- mozilla/security/nss/lib/ssl/ssl3prot.h	6 Mar 2008 20:16:22 -0000	1.13
>+++ mozilla/security/nss/lib/ssl/ssl3prot.h	23 Oct 2009 23:27:31 -0000
>@@ -173,7 +173,10 @@ typedef struct {
>     uint8 length;
> } SSL3SessionID;
>      
>-typedef enum { compression_null = 0 } SSL3CompressionMethod;
>+typedef enum {
>+    compression_null = 0,
>+    compression_deflate = 1
>+} SSL3CompressionMethod;
>      

I think it will be necessary to name the enum (in addition to the 
typedef, e.g. 

>+typedef enum SSL3CompressionMethodEnum {
>+    compression_null = 0,
>+    compression_deflate = 1
>+} SSL3CompressionMethod;

libssl defines an extensible structure SSLChannelInfo and a function 
which copies it (as much as will fit) into the caller's buffer
SSL_GetChannelInfo().  This is how the application gets the info about
all the variables negotiated by the handshake on the connection.  
At the present time, this structure does not include the compression
info, because until now, it really wasn't negotiable. Now it is, so 
now we need to add a member onto the end of that structure, and 
populate it with the correct info in that function.  

I took a crack at implementing that, and wrote a tiny patch which I'll 
attach shortly for Wan-Teh's review.  But the SSLChannelInfo structure 
in defined in a header file that is #included by ssl3prot.h, so it 
cannot use typedefs defined in ssl3prot.h. But it can use named enums,
even if they're defined later.  So, I ask you to change your declaration
of that enum above as shown.  

With those changes, I think we'll be ready to commit.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Supplemental patch - add compression to SSLChannelInfo

This patch depends on the change to the previous patch that I described 
in the previous comment. 

Wan-Teh, please review.

Comment 34

[Wan-Teh Chang]

Comment on attachment 409533 [details] [diff] [review]
Supplemental patch - add compression to SSLChannelInfo

r=wtc.

In sslt.h:

>+    enum SSL3CompressionMethodEnum compression;

This enum looks strange.  Can we move the enum definition or
the typedef to sslt.h, so that we can use SSL3CompressionMethod
here?

In sslinfo.c:

>-	    /* XXX  These should come from crSpec */
>+	    ssl_GetSpecReadLock(ss);
> 	    inf.cipherSuite      = ss->ssl3.hs.cipher_suite;
>+	    inf.compression      = ss->ssl3.crSpec->compression_method;
>+	    ssl_ReleaseSpecReadLock(ss);

Should we keep the XXX comment, since cipher_suite is
not yet a member of ss->ssl3.crSpec?

Comment 35

[Wan-Teh Chang]

Comment on attachment 409463 [details] [diff] [review]
Patch for ssltap to pretty-print compression methods (checked in)

I checked in the ssltap patch on the NSS trunk (NSS 3.12.5).
It doesn't require Adam's patch.

Checking in ssltap.c;
/cvsroot/mozilla/security/nss/cmd/ssltap/ssltap.c,v  <--  ssltap.c
new revision: 1.15; previous revision: 1.14
done

Comment 36

[Wan-Teh Chang]

(In reply to comment #32)
>
> >Index: mozilla/security/nss/lib/ssl/Makefile
> 
> >+ifdef USE_ZLIB
> >+DEFINES += -DNSS_ENABLE_ZLIB
> >+EXTRA_SHARED_LIBS += -lz
> >+endif
> 
> This is one of the two issues.  I think we want to have the option to be 
> able to link with the system zlib on systems that have it, and to link
> with NSS's static zlib on systems that don't have one. [...snipped...] 
> 
> To avoid complexity, I'd suggest having the default always be to use 
> the static zlib on all platforms, and let the user define an environment
> variable USE_SYSTEM_LIBZ if he wants to do that instead.

The USE_SYSTEM_ZLIB variable already exists.  It is supposed to be
defined in mozilla/security/coreconf/<OS>.mk file.  Right now it's
only defined for Linux and OpenBSD.  Can we just use the existing
USE_SYSTEM_ZLIB varible?
http://mxr.mozilla.org/security/search?string=USE_SYSTEM_ZLIB

My patch (attachment 409001 [details] [diff] [review]) already does that.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Yes, I didn't mean to suggest yet another variable.

Comment 38

[Wan-Teh Chang]

Attachment: Adam's patch v6 (checked in, except sslstress.txt)

I updated Adam's patch to address Nelson's comment 32.
I also made a few cosmetic changes related to Nelson's
suggested changes.

However, I can't make two of Nelson's suggested changes.

1. In nss/lib/ssl/Makefile, I merely changed USE_ZLIB
to USE_SYSTEM_ZLIB.  I will make the rest of the change
that Nelson suggested (to link with zlib statically by
default) when Mozilla copies our zlib from nss/cmd/zlib
to nss/lib/zlib.  I already have the patch (attachment 409001 [details] [diff] [review])
to do that.

2. Regarding the SSL3CompressionMethod enum type, it is
defined in the private header ssl3prot.h.  So we will need
to move it to a public header (sslt.h) for Nelson's
SSLChannelInfo patch (attachment 409533 [details] [diff] [review]).  Merely giving
the enum a name won't work, because the two enum constants
(compression_null and compression_deflate) also need to
be defined in a public header.  I suggest that we take care
of that as part of the SSLChannelInfo patch.  I also suggest
we rename the enum SSLCompressionMethod (removing the "3"
from "SSL3") and add the ssl_ prefix to the enum constants
(ssl_compression_null, ssl_compression_deflate) when we make
them public.

Nelson, I suggest that we check in this patch first to make
subsequent code reviews easier.

Comment 39

[Wan-Teh Chang]

Comment on attachment 409984 [details] [diff] [review]
Adam's patch v6 (checked in, except sslstress.txt)

Nelson, please review the changes to sslstress.txt.  I think
some of them are wrong.

>+  noECC     0      -z        -2_-c_1000_-C_c_-z            Stress TLS  RC4 128 with MD5 (compression)
>+  noECC     0      -z_-u     -2_-c_1000_-C_c_-z            Stress TLS  RC4 128 with MD5 (session ticket, compression)

In the second one, I think we should also add -u to the
client params so that session ticket is enabled on both
sides.

>+  noECC     0      -r_-r_-z  -2_-c_100_-C_c_-n_TestUser_-z Stress TLS RC4 128 with MD5 (compression, client auth)
>+  noECC     0   -u_-r_-r_-u  -2_-c_100_-C_c_-n_TestUser_-z Stress TLS RC4 128 with MD5 (compression, client auth, session ticket)

The first one should also say "no reuse" in parentheses.

The second one has two issues: the server params has two -u's.
It should be -u -z.  The client params is missing -u.

We should figure out a way to verify that DEFLATE compression
is actually used in the tests.  Perhaps it requires your
SSLChannelInfo patch.  Perhaps we need an SSL option to disable
compression_null so that compression_deflate is the only
supported compression method.  What do you think?

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 409984 [details] [diff] [review]
Adam's patch v6 (checked in, except sslstress.txt)

r=nelson for all parts of this patch EXCEPT sslstress.txt.
More comments to follow.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #39)

> In the second one, I think we should also add -u to the client params so 
> that session ticket is enabled on both sides.

OK

> The first one should also say "no reuse" in parentheses.

It should?  I think "no reuse" generally goes with the -N option, which 
defeats all session reuse.  I don't see that option in use there.
Did I miss it?

> The second one has two issues: the server params has two -u's.
> It should be -u -z.  The client params is missing -u.

Agreed.
 
> We should figure out a way to verify that DEFLATE compression is actually 
> used in the tests.  Perhaps it requires your SSLChannelInfo patch.  

I think I prefer that approach.

> Perhaps we need an SSL option to disable compression_null so that 
> compression_deflate is the only supported compression method.  
> What do you think?

The SSL3 ID and TLS RFCs require that NULL compression be supported always.

Comment 42

[Wan-Teh Chang]

Comment on attachment 409984 [details] [diff] [review]
Adam's patch v6 (checked in, except sslstress.txt)

I checked in this patch (except sslstress.txt) on the NSS trunk
(NSS 3.12.5).

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.90; previous revision: 1.89
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.63; previous revision: 1.62
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.58; previous revision: 1.57
done
Checking in lib/ssl/Makefile;
/cvsroot/mozilla/security/nss/lib/ssl/Makefile,v  <--  Makefile
new revision: 1.8; previous revision: 1.7
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.29; previous revision: 1.28
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.118; previous revision: 1.117
done
Checking in lib/ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.14; previous revision: 1.13
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.7; previous revision: 1.6
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.67; previous revision: 1.66
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.58; previous revision: 1.57
done

Comment 43

[Wan-Teh Chang]

Attachment: Patch for sslstress.txt (checked in)

Nelson, you're right, we should not add "no reuse" because
the -N option is not used.  Please review this patch.  I'm
not sure if we need to test the -u -z combination because
the processing of session tickets and compression methods
seems to be mutually independent.  Should we just test -z?
I don't want to add unnecessary long tests to our test suite.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Well, we have to test -z together with at least one combination of features. 
Session tickets can be a part of that combination, or not, as you choose.
I don't think it's necessary to test all possible combinations with -z.

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 410270 [details] [diff] [review]
Patch for sslstress.txt (checked in)

This seems sensible.  r=nelson

Comment 46

[Wan-Teh Chang]

Nelson, I will attach a new SSLChannelInfo patch that will
also fix some bugs in Adam's patch v6.  Please don't work on
the SSLChannelInfo patch.

Comment 47

[Wan-Teh Chang]

Attachment: Supplemental patch v2 - fix bugs, add compressionAlgorithm to SSLChannelInfo

Adding the compression algorithm to SSLChannelInfo allowed
me to see that strsclnt could not negotiate DEFLATE compression
with selfserv because strsclnt sent {null, DEFLATE} in client
hello and selfserv picks the first supported compression method
on the list.  This patch changes NSS to list DEFLATE before null
in client hello (see ssl3con.c).  It also removes an extra comma
that breaks the concatenation of two string literals in strsclnt.c.

Adam, you just need to review the fixes for these two bugs in
ssl3con.c and strsclnt.c.

Nelson, I'd like you to review some design decisions first.

1. The new field of SSLChannelInfo is named compressionAlgorithm,
which is the name used in the SecurityParameters structure in
TLS.  An alternative name is compressionMethod, which is the name
used in the ClientHello and ServerHello structures in TLS.
Which name do you prefer?  (I'm now actually inclined towards
compressionMethod.)

2. I also added a const char * field to SSLChannelInfo, for the
name of the compression algorithm, to match the similar fields
in SSLCipherSuiteInfo.

3. Do you prefer "null" or "NULL" as the string representation
of CompressionMethod.null?

4. I removed the "3" from "SSL3CompressionMethod" and added
the "ssl_" prefix to "compression_null" and "compression_deflate",
to match the style of similar enums defined in sslt.h.

I also have two questions for you.

1. In SSL_GetChannelInfo, why do you use
ss->ssl3.crSpec->compression_method instead of
ss->ssl3.hs.compression?  Adam's patch sets
ss->ssl3.hs.compression, which is why
ss->ssl3.hs.compression seems better to me.
(I don't know what ss->ssl3.crSpec is.)

2. I found what might be a bug when using strsclnt
without the -2 option.  Without the -2 option,
strsclnt sends SSLv2-compatible client hello,
which cannot specify compression methods.  So the
initial full handshake negotiates the default
null compression.  However, strsclnt uses SSLv3
client hello for session resumption handshakes,
in which it specifies {DEFLATE, null}, and
selfserv selects the first supported compression
method as usual (DEFLATE).  Is this wrong?
Should selfserv select the compression method of
the resumed session (null) when it resumes a
session?

Comment 48

[Wan-Teh Chang]

Comment on attachment 410270 [details] [diff] [review]
Patch for sslstress.txt (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.5).

Checking in sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.15; previous revision: 1.14
done

Comment 49

[Adam Langley]

>> method as usual (DEFLATE).  Is this wrong?
>> Should selfserv select the compression method of
>> the resumed session (null) when it resumes a
>> session?

Yea, this is another bug in my code :(

RFC 4346: "For resumed sessions this field is the value from the
resumed session state."

I didn't think about this. I can probably correct it tomorrow. Thanks
for bringint it up.

Comment 50

[Adam Langley]

LGTM -> Supplemental patch v2 - fix bugs, add compressionAlgorithm to SSLChannelInfo (25.13 KB, patch)

Comment 51

[Wan-Teh Chang]

Comment on attachment 410425 [details] [diff] [review]
Supplemental patch v2 - fix bugs, add compressionAlgorithm to SSLChannelInfo

Bob, please see comment 47 for a description of this patch, and
the design decisions/questions originally intended for Nelson
(now for you).  Thanks!

Comment 52

[Robert Relyea]

> 1. The new field of SSLChannelInfo is named compressionAlgorithm,
> which is the name used in the SecurityParameters structure in
> TLS.  An alternative name is compressionMethod, which is the name
> used in the ClientHello and ServerHello structures in TLS.
> Which name do you prefer?  (I'm now actually inclined towards
> compressionMethod.)

I'm confused. it looks like it's called 'compression'to me (maybe I'm looking at the wrong patch). It looks like it's declared as an enum when all other arguments or PRUIntXX and PRIntXX. The former is probably just paranoia, but it's probably best to stay with the old style unless Nelson is trying to purposefully break us out of that old style.

> 3. Do you prefer "null" or "NULL" as the string representation
> of CompressionMethod.null?

ciphers are all upper case, including NULL. DEFLATE is upper case, so it would seem reasonable for NULL to be too.

> 1. In SSL_GetChannelInfo, why do you use
> ss->ssl3.crSpec->compression_method instead of
> ss->ssl3.hs.compression? 

I suspect because the original patch used the crSpec definition everywhere. I think hs is handshake and crSpec is cipher spec. The latter is used at the low level, the former at the higher. It seems like the hs. is more correct, both fields have identical data (crSpec->compression_method is set at lin 1269 of ssl3con.c "ssl3_SetupPendingCipherSpecs" to hc.compression).

> 2. ....  Is this wrong?
> Should selfserv select the compression method of
> the resumed session (null) when it resumes a
> session?

I believe the answer is yes, as adam indicated.

Comment 53

[Robert Relyea]

Comment on attachment 410425 [details] [diff] [review]
Supplemental patch v2 - fix bugs, add compressionAlgorithm to SSLChannelInfo

r+ if you fix the following.

Make compressionAlgorithm a PRUInt (probably at least 16).
You can rename it to compressionMethod if you like, or just compression.

You can change ss->ssl3.crSpec->compression_method to the appropriate ss->ssl3.hs.compression if you want.

change return "null" to return "NULL in ssl_getcompressionAlgorithmName.

I'm not so sure about the char * in the channel info. For ciphers we have a separate call that returns cipher specific info (like the name).

Comment 54

[Robert Relyea]

> I'm confused. 

I'm unconfused now, I found the appropriate patch (as seen my my review.:)

bob

Comment 55

[Nelson Bolyard (seldom reads bugmail)]

I wonder if maybe I should switch roles with bob, and go to being reviewer
instead of writer.  There's a lot of detail about libSSL that I know and 
evidently other's don't.  crSpec is one of 4 spec pointers, IIRC, which are
crSpec   current read cipher spec
cwSpec   current write cipher spec
prSpec   pending read cipher spec
pwSpec   pending write cipher spec

The "current" specs are the specs in use, actually being used to encrypt 
(write) and decrypt (read) the records going over the wire now. 
The "pending" specs are the specs being negotiated during the current 
handshake (while a handshake is in progress, up until the second "change
cipher spec" record is sent).  

I admit that it's not entirely clear when to use the .hs. variables vs 
when to use the "pending" cipher spec variables, but neither of those should
ever be mixed up with the "current" cipher spec variables.  The "current"
cipher spec variables should always be treated as "read only" and should be
accessed only only while holding the appropriate lock.  The "pending" 
variables can be regarded as read/write, and also should be accessed only 
while holding the appropirate lock. 

Even though there are 4 spec pointers, there are actually only two spec structures.  Normally (most of the time) the two "current" pointers point
to one, and the two "pending" structures point to the other.  When you send
a "change cipher spec" message, you switch the current and pending write spec pointers.  When you receive a "change cipher spec" message, you switch the 
current and pending read spec pointers.  So, in the brief time when one side
(reader or writer) has switched nd the other side (writer or reader) has not
one side's pending is the other side's current, and vice versa.

Comment 56

[Robert Relyea]

From your description, it sounds like get channelInfo should return the value of hs.compresssion_method then, since it's not getting a lock.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 410425 [details] [diff] [review]
Supplemental patch v2 - fix bugs, add compressionAlgorithm to SSLChannelInfo

Sorry SR-
This patch is wrong, because it doesn't use a lock to access the cipher spec.  
My original patch DID use a lock.  Why was that changed?
More comments to follow.

Comment 58

[Adam Langley]

Attachment: Preliminary patch to make sure that the compression of resumed sessions is correct. Needs wtc's patch first.

I wrote a patch which appears to fix the bug that wtc pointed out in #47

% ./selfserv -z -C 10 -d sql:/home/agl/.pki/nssdb -n key -p 8000
% ./strsclnt -c 2 -z -d sql:/home/agl/.pki/nssdb -p 8000 agl.example.com

This performs an SSLv2 handshake, establishes a connection with NULL compression and then resumes a second connection, with NULL compression, despite advertising DEFLATE in the, now TLS, ClientHello.

Adding -2 to strsclnt causes a TLS handshake to be performed initially. The server picks DEFLATE compression. The resumed connection also has DEFLATE compression.

Comment 59

[Robert Relyea]

> From your description, it sounds like get channelInfo should return the value
> of hs.compresssion_method then, since it's not getting a lock.

Actually I'm wrong about that. The original code doesn't get a lock, but the current code does.

I would be interested to learn why we want the current read cipher spec here and not the handshake value, and if the current read cipher spec is right for compression, why isn't it right for encryption?

It seems such an obvious asymmetry (if it's required) should have a pretty extensive comment.

bob

Comment 60

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 410425 [details] [diff] [review]
Supplemental patch v2 - fix bugs, add compressionAlgorithm to SSLChannelInfo

I think I was really tired, and probably stressed out, when I previously reviewed this patch.  Sorry.  SR++

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

> if the current read cipher spec is right for compression, why isn't it 
> right for encryption?

It should be right for both.  Sounds like you found a bug somewhere.

Comment 62

[Wan-Teh Chang]

Comment on attachment 408972 [details] [diff] [review]
Makefile changes if nss/cmd/zlib is moved to nss/lib/zlib (checked in)

nss/cmd/zlib has just been copied to nss/lib/zlib.  We can now
switch over to the new location of zlib.

I checked in this patch on the NSS trunk (NSS 3.12.5).

Checking in cmd/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/manifest.mn,v  <--  manifest.mn
new revision: 1.29; previous revision: 1.28
done
Checking in cmd/Makefile;
/cvsroot/mozilla/security/nss/cmd/Makefile,v  <--  Makefile
new revision: 1.8; previous revision: 1.7
done
Checking in lib/manifest.mn;
/cvsroot/mozilla/security/nss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.20; previous revision: 1.19
done
Checking in lib/Makefile;
/cvsroot/mozilla/security/nss/lib/Makefile,v  <--  Makefile
new revision: 1.13; previous revision: 1.12
done

Comment 63

[Wan-Teh Chang]

Attachment: CVS remove nss/cmd/zlib (output log)

I CVS removed nss/cmd/zlib on the NSS trunk (NSS 3.12.5).

Comment 64

[Wan-Teh Chang]

Attachment: Supplemental patch v3 - fix bugs, add compressionAlgorithm to SSLChannelInfo (checked in)

I made some cosmetic changes before I checked this in on the
NSS trunk (NSS 3.12.5).

1. changed compression method name string "null" to "NULL".
2. renamed the |compressionAlgorithm| field of SSLChannelInfo
   to |compressionMethod|.
3. tweaked the output format of SSLChannelInfo in selfserv,
   strsclnt, and tstclnt.

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.91; previous revision: 1.90
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.64; previous revision: 1.63
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.59; previous revision: 1.58
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.120; previous revision: 1.119
done
Checking in lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.5; previous revision: 1.4
done
Checking in lib/ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.15; previous revision: 1.14
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.69; previous revision: 1.68
done
Checking in lib/ssl/sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.19; previous revision: 1.18
done
Checking in lib/ssl/sslsnce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsnce.c,v  <--  sslsnce.c
new revision: 1.51; previous revision: 1.50
done
Checking in lib/ssl/sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.13; previous revision: 1.12
done

Comment 65

[Wan-Teh Chang]

Attachment: Supplemental patch for ssltap (checked in)

I also changed ssltap to print the null compression method
as "NULL" instead of "null", and to print all the compression
method identifiers listed at IANA:
http://www.iana.org/assignments/comp-meth-ids/comp-meth-ids.xhtml

Checking in ssltap.c;
/cvsroot/mozilla/security/nss/cmd/ssltap/ssltap.c,v  <--  ssltap.c
new revision: 1.16; previous revision: 1.15
done

Comment 66

[Wan-Teh Chang]

Comment on attachment 410989 [details] [diff] [review]
Supplemental patch v3 - fix bugs, add compressionAlgorithm to SSLChannelInfo (checked in)

Re: the issue of crSpec vs. hs (see comment 61)

> 	} else if (ss->ssl3.initialized) { 	/* SSL3 and TLS */
>-
>-	    /* XXX  These should come from crSpec */
>-	    inf.cipherSuite      = ss->ssl3.hs.cipher_suite;
>+	    ssl_GetSpecReadLock(ss);
>+	    inf.cipherSuite       = ss->ssl3.hs.cipher_suite;
>+	    inf.compressionMethod = ss->ssl3.crSpec->compression_method;
>+	    inf.compressionMethodName =
>+		ssl_GetCompressionMethodName(inf.compressionMethod);
>+	    ssl_ReleaseSpecReadLock(ss);
> 	}

I found that crSpec has only ssl3BulkCipherDef and ssl3MACDef,
but not the cipher suite value.  So ss->ssl3.hs.cipher_suite
seems to be the only way to get the cipher suite.

If it'd be better for crSpec to contain the cipher suite value,
should we resurrect the "XXX  These should come from crSpec"
comment?

Comment 67

[Wan-Teh Chang]

Attachment: Supplemental patch for SSL_GetChannelInfo (checked in)

SSL_GetChannelInfo needs to set the new compression method
fields for SSL2, otherwise compressionMethodName is a NULL
pointer.

I checked in this patch on the NSS trunk (NSS 3.12.5) to
fix Tinderbox test crashes.  (Tinderboxes seem to enable
verbose mode of tstclnt, which crashes when trying to print
a NULL compressionMethodName for SSL2 tests.)

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.20; previous revision: 1.19
done

Comment 68

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 411005 [details] [diff] [review]
Supplemental patch for SSL_GetChannelInfo (checked in)

Perhaps, for SSL v2, it should say something like "N/A" (not applicable), rather than "NULL", which is a valid SSL3 compression method.

Comment 69

[Nelson Bolyard (seldom reads bugmail)]

I'd LOVE to do a clean-up of libSSL, but it has never seemed to be a higher
priority than any of 100 other bugs and RFEs against NSS, so it never gets 
done.  Now, with the greatly reduced NSS staff, and the doubt about Sun's
NSS team's future, I doubt I'll ever get to it.  I don't have much hope that
the staffing level of NSS will ever rise to the point that I can go back to 
being "MisterSSL" (meaning: I really focus on SSL) again.  At this point,
I'll be happy if I can stay employed long enough to finish this bug.

I'd really like to eliminate all duplication between "hs" and the specs. 
I think that "hs" should ONLY be used before and during a handshake.  
After the handshake is done (after the change cipher specs have occured)
all references should be to the crSpec and cwSpec.  "hs" should not be 
relevant, because it could contain values for the NEXT handshake being done.
So, at best, the "hs" values overlap the "prSpec" and "pwSpec" values.  

Perhaps it should be said that the "hs" values are the locally desired 
values at the beginning of the handshake negotiation, and the p[rw]Spec 
values are the negotiated values.  That's probably the right vision.

Yes, I think the cipher suite values probably should (also) be in the specs
and GetChannelInfo should get it from there rather than from the "hs". 
That's basically a bug, and should be fixed, with considerable care.

Comment 70

[Wan-Teh Chang]

(In reply to comment #68)
>
> Perhaps, for SSL v2, it should say something like "N/A" (not applicable),
> rather than "NULL", which is a valid SSL3 compression method.

I agree that it's worth considering to return "N/A" for
SSL v2, but it seems to require adding an enum constant
ssl_compression_invalid = -1 to the SSLCompressionMethod
type.  It doesn't seem worth complicating the code for a
protocol that's going away.  Or are you proposing that we
return compressionMethod=ssl_compression_null and
compressionMethodName="N/A" for SSL v2?

Returning "NULL" is reasonable though because the null
compression method is a no-op (identity operation) and
accurately maps a notion that doesn't exist in SSL v2.

Comment 71

[Nelson Bolyard (seldom reads bugmail)]

> Or are you proposing that we return compressionMethod=ssl_compression_null 
> and compressionMethodName="N/A" for SSL v2?

Yes.  But this is not high priority.

Comment 72

[Wan-Teh Chang]

Attachment: Return compressionMethodName = "N/A" for SSL v2 (checked in)

Note that we still return compression = ssl_compression_null for
SSL v2.  This avoids the need to add a new ssl_compression_na/invalid
enum constant.

This patch ignores whitespace to facilitate code review.  The actual
patch aligns the '=' signs in assignment statements nicely.

Comment 73

[Wan-Teh Chang]

Comment on attachment 411245 [details] [diff] [review]
Return compressionMethodName = "N/A" for SSL v2 (checked in)

While testing this patch, I found that the following code prints

  selfserv: SSL version 0.2 using 128-bit RC4 with 128-bit MD5 MAC

for SSL v2:

            FPRINTF(stderr,
            "selfserv: SSL version %d.%d using %d-bit %s with %d-bit %s MAC\n",
               channel.protocolVersion >> 8, channel.protocolVersion & 0xff,
               suite.effectiveKeyBits, suite.symCipherName,
               suite.macBits, suite.macAlgorithmName);

Is that correct?

Comment 74

[Wan-Teh Chang]

Attachment: Adam's patch to make sure that the compression of resumed sessions is correct. v2 (checked in)

Adam, thanks for the patch.  I made the following changes to your
patch.

1. I perform the two checks on the cached compression method in the
same order we perform the two checks on the cached cipher suite.

2. I remove the #ifdef PARANOIA from the check that the cached
compression method is still enabled, because the equivalent check
for cached cipher suite is not inside that ifdef.

3. I set ss->ssl3.hs.compression = sid->u.ssl3.compression and
jump to compression_found instead of suite_found.  This
eliminates the code at the suite_found label in your patch.

Please review this version.

Comment 75

[Wan-Teh Chang]

Comment on attachment 411245 [details] [diff] [review]
Return compressionMethodName = "N/A" for SSL v2 (checked in)

I checked in the patch on the NSS trunk (NSS 3.12.5).

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.21; previous revision: 1.20
done

Comment 76

[Wan-Teh Chang]

Comment on attachment 411250 [details] [diff] [review]
Adam's patch to make sure that the compression of resumed sessions is correct. v2 (checked in)

Nelson, the review or checkin of this patch can wait until after NSS 3.12.5
based on your discretion.

Comment 77

[Wan-Teh Chang]

Attachment: nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v3

Bob,

This is the same as v2 of this patch.  I merely updated the
patch to the current NSS trunk.

The review or checkin of this patch can wait until after
NSS 3.12.5, based on your or Nelson's discretion.

Comment 78

[Nelson Bolyard (seldom reads bugmail)]

> the following code prints
> selfserv: SSL version 0.2 using 128-bit RC4 with 128-bit MD5 MAC
> for SSL v2:

That's correct.  The 16-bit version number space shared by all versions of
SSL was originally intended to be a simple integer with values to 64k. 
SSL v2 has the value 0x0002.  There was a version 0x0003 and it appears in 
the (in)famous SSL patent, but AFAIK was never in any live browser. 
When the SSL internet draft was written for "SSL 3.0", the 16 bit space
was conceptually divided into two 8-bit spaces (as you know) commonly 
displayed separated by a decimal point.  Seen that way, the original SSL 2
is version 0.2.  To this day, one can still find specifications for two 
separate versions of SSL claiming to be SSL 3, one of which is 3.0.  Any
search for SSL in the USPTO will always find the other one.  That is one 
reason why I often try to be very explicit about which SSL 3 I mean 
(e.g. SSL30).

Comment 79

[Wan-Teh Chang]

Attachment: Undefine compress as a macro (checked in, then reverted)

Nelson, this patch is necessary for NSS 3.12.5 to be built as
part of Mozilla on Linux with DEFLATE compression support.

Comment 80

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 411877 [details] [diff] [review]
Undefine compress as a macro (checked in, then reverted)

OK.  For 3.12.6, we should probably rename "compress" to something else. 
Also, shouldn't the #undef be in the header file where the structure member is defined, to ensure that all files that include that header don't have this problem?  
But we can do that in 3.12.6, or something.

Comment 81

[Wan-Teh Chang]

Attachment: Rename compress=>compressor decompress=>decompressor (checked in)

I actually prefer renaming our compress to avoid the conflict,
even though this patch is larger.  I also renamed decompress
for consistency.

Comment 82

[Wan-Teh Chang]

Comment on attachment 411877 [details] [diff] [review]
Undefine compress as a macro (checked in, then reverted)

I checked in this patch on the NSS trunk (NSS 3.12.5).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.121; previous revision: 1.120
done

Comment 83

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 411880 [details] [diff] [review]
Rename compress=>compressor decompress=>decompressor (checked in)

r=nelson for checkin when libSSL is unfrozen for 3.12.6 checkins.

Comment 84

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 411250 [details] [diff] [review]
Adam's patch to make sure that the compression of resumed sessions is correct. v2 (checked in)

r=nelson for checkin to NSS 3.12.6 to trunk after libSSL is unfrozen for 3.12.5

Comment 85

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 411279 [details] [diff] [review]
nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v3

r=nelson for checkin for 3.12.6 after libSSL is unfrozen for NSS 3.12.5 .
I do think Bob should still review this.

Comment 86

[Wan-Teh Chang]

Comment on attachment 411250 [details] [diff] [review]
Adam's patch to make sure that the compression of resumed sessions is correct. v2 (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.122; previous revision: 1.121
done

Comment 87

[Wan-Teh Chang]

Comment on attachment 411877 [details] [diff] [review]
Undefine compress as a macro (checked in, then reverted)

I backed out this patch on the NSS trunk (NSS 3.12.6).
I will use the better solution in attachment 411880 [details] [diff] [review].

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.123; previous revision: 1.122
done

Comment 88

[Wan-Teh Chang]

Comment on attachment 411880 [details] [diff] [review]
Rename compress=>compressor decompress=>decompressor (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.123; previous revision: 1.122
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.70; previous revision: 1.69
done

Comment 89

[Wan-Teh Chang]

Attachment: Declare variables at the beginning of a block (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6) to fix a
compilation error on Windows, introduced by attachment 411250 [details] [diff] [review].

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.124; previous revision: 1.123
done

Comment 90

[Nelson Bolyard (seldom reads bugmail)]

Is this bug now resolved for 3.12.6 ?

Comment 91

[Georg v. Zezschwitz]

Sorry for this late response, but I had to manage setting up a build environment.

This patch *might* work for Linux/OpenBSD, but has no effect for Windows / Mac or all other platforms with USE_SYSTEM_ZLIB!=1.

If manually enforcing compression by editing lib/ssl/Makefile to enforce -DNSS_ENABLE_ZLIB, building on my Mac generates

...
Undefined symbols:
  "_MOZ_Z_deflateEnd", referenced from:
      _ssl3_DestroyCompressContext in ssl3con.o
[some more zlib-functions]

Any simple way to either use the external zlib or Mozillas internal one?

Comment 92

[Georg v. Zezschwitz]

Although I got it working now for MacOS (don't know if my approach to link with libmozz is "legal", patch later after testing with Linux as well), there might be problems with multiple concurrent SSL connections.
Traces to https://services.mozilla.org (a server that supports SSL compression) show SSL errors:
 "SSLv3 Alert (Level: Fatal, Description: Bad Record MAC)"
for 2nd connections and further ones.

PCAP file attached

Comment 93

[Georg v. Zezschwitz]

Attachment: pcap file with SSL compression (faulty)

pcap trace "NSS 3.12.6 with SSL compression" to https://services.mozilla.org - browser reports connection aborts

E.g.:
 48   3.565694 192.168.18.106 -> 63.245.209.70 SSLv3 Alert (Level: Fatal, Description: Bad Record MAC)

Comment 94

[Adam Langley]

Some observations:

The first failing connection is starting at frame 26. The failures are the client rejecting the server's Finished message with a Bad MAC alert. The handshake is SSLv3 and decompression failures result in a Bad MAC alert in SSLv3. The successful connection is a full handshake. The failing connections are resumptions.

Chrome Linux has been running this patch for a while now with no reported trouble. However, it's possible that we aren't triggering quite the same code paths for some reason. I'll try to write a client to specifically test this and see if I can reproduce it.

Comment 95

[Adam Langley]

I can reproduce this by resuming a connection with SSLv3. Probably we haven't seen this problem with Chrome Linux because we're always using TLSv1.

The error is coming from zlib when we try to decompress the server's Finished message when resuming. If I set a breakpoint in the code, I see the following:

First compressed block, 52 bytes (this is from the initial, non-resume connection):

0x6a54c0:       0x78    0x9c    0x00    0x28    0x00    0xd7    0xff    0x14
0x6a54c8:       0x00    0x00    0x24    0x7b    0xf6    0x0e    0x3f    0x66
0x6a54d0:       0xf8    0x75    0xe7    0xa2    0xa2    0x50    0x54    0x8b
0x6a54d8:       0xf9    0xf3    0x95    0x44    0xb8    0x8b    0x84    0xbf
0x6a54e0:       0xcf    0xbb    0xc0    0x65    0xf4    0x35    0xd4    0xd3
0x6a54e8:       0x28    0xa9    0xb5    0xf9    0xb1    0xd9    0xbc    0x00
0x6a54f0:       0x00    0x00    0xff    0xff

Results in 40 bytes, uncompressed:

0x6362e0:       0x14    0x00    0x00    0x24    0x7b    0xf6    0x0e    0x3f
0x6362e8:       0x66    0xf8    0x75    0xe7    0xa2    0xa2    0x50    0x54
0x6362f0:       0x8b    0xf9    0xf3    0x95    0x44    0xb8    0x8b    0x84
0x6362f8:       0xbf    0xcf    0xbb    0xc0    0x65    0xf4    0x35    0xd4
0x636300:       0xd3    0x28    0xa9    0xb5    0xf9    0xb1    0xd9    0xbc

The second time the breakpoint is hit is when decompressing the server's Finished message from the second connection (which was resumed):

Second 'compressed' block, 40 bytes:

0x69d6d0:       0x14    0x00    0x00    0x24    0x2c    0x08    0x73    0x37
0x69d6d8:       0xe3    0x7d    0xf1    0xb6    0x73    0xef    0xa0    0x5a
0x69d6e0:       0x47    0xf5    0x00    0x04    0xd5    0x7e    0xa8    0x20
0x69d6e8:       0x08    0x05    0x84    0xe9    0x6c    0x72    0x8f    0xe6
0x69d6f0:       0x30    0x44    0xf6    0x9f    0xc4    0xa4    0x04    0x6f

That looks rather like an uncompressed Finished message! The type and length bytes are there and it's the correct length. Looking at Georg's trace: the encrypted handshake messages for the failing connections are 64 bytes long which the successful ones are 80 bytes long. (It's AES 256 SHA, so there's block cipher padding in those numbers).

I suspect that this isn't our bug: the server isn't remembering the compression when resuming connections with SSLv3. By my reading of the SSLv3 spec, it should be.

I can also repo this against www.doc.ic.ac.uk (Apache with OpenSSL).

I cannot repo against `openssl s_server -accept 8080 -id_prefix foo` (0.9.8k, nor 0.9.7h).

Here's an interesting thread saying the OpenSSL didn't persist the compression method when serialising sessions: http://rt.openssl.org/Ticket/Display.html?id=1960&user=guest&pass=guest . However, I don't see how that can account for the observed behaviour since we're advertising DEFLATE in the ClientHello on the resumed sessions.

Its difficult to know what to do about this. Even if we enable TLSv1, a server might come back with SSLv3 and have this bug. It seems pretty horrible, but we might want to catch the decompression failure, see if the record looks to be uncompressed and, if so, switch off compression.

Comment 96

[Adam Langley]

I cannot repo with Apache2 (SVN) + mod_ssl + 0.9.8k. But I *can* repo with Apache 2.2.8 + mod_ssl + 0.9.8g.

Either way, it appears that there's a large number of buggy (?) servers out there.

Comment 97

[Adam Langley]

Attachment: patch to disable compression when we get a record that appears to not be compressed

Georg: can you try this?

Comment 98

[Reed Loden [:reed]]

Comment on attachment 419029 [details] [diff] [review]
patch to disable compression when we get a record that appears to not be compressed

>+	    /* There appears to be a bug with (at least) Apache + OpenSSL where
>+	     * resumed SSLv3 connections don't actually use compression. See
>+	     * comment 93..95 of https://bugzilla.mozilla.org/process_bug.cgi

You mean https://bugzilla.mozilla.org/show_bug.cgi?id=275744 here, but you could just shorten it to "bug 275744", imho.

Comment 99

[Georg v. Zezschwitz]

Yes! Works fine! Tests were:
- TLS enable and TLS disabled (SSLv3)
- MacOS with built-in ZLIB
- www.doc.ic.ac.uk, services.mozilla.com

Comment 100

[Nelson Bolyard (seldom reads bugmail)]

My 13+ years of experience with NSS and SSL tell me that every time you add
code to enable non-standards-compliant interoperation with a non-standards-
compliant peer, you can NEVER remove that.  In effect, you extend the 
definition of the protocol so that, forever thereafter, that non-standard behavior becomes part of the de-facto standard definition.   You can never 
undo it.  This is ESPECIALLY TRUE in browsers, where the first browser to 
remove such a feature always runs a risk of losing market share (after all
the browsers have done it).  

If you doubt this even a little bit, look at the fact that browsers have 
been doing "TLS intolerant fallback" which utterly defeats TLS's built-in 
protection against version roll-back attacks, for 11 years now.  Recently, attempts to standardize on a new form of TLS renegotiation, to fix the TLS
renegotiation vulnerability (which is publicly known and exploits exist in
the wild) was delayed for MONTHS while people argued that browsers MUST NOT
give up this "TLS intolerant fallback" behavior, not even 11 years after 
the broken servers should have all been fixed.  Based on their response, 
I gather that Firefox developers were unwilling to even consider dropping 
TLS intolerant fallback.  

So, it's much better to resist the urge to immediately cave in and do any and
every thing in your power to interoperate with non-compliant servers, and instead to force those servers to get fixed, than to immediately give in.  
This is DOUBLY TRUE in security protocols.  

Remember, there's a name for business people who never say no to their users,
and that name isn't "security providers".

Comment 101

[Chris Newman]

Nelson makes many important points.  For commercial software, I often refuse to provide a workaround until the customer admits the other party is at fault and we're doing a favor helping them (refusing to workaround a problem when a workaround is feasible and straightforward can lose customers, so this is the best available compromise in a commercial context).

In an open source context, standing on principle is more feasible but it still comes down to a question of how widely deployed the broken software happens to be.  If the broken compression support is in OpenSSL 0.9.8g to 0.9.8j, then there's a good chance the bug is too widely deployed to ignore.  If NSS refuses to workaround it but others do, then users will assume NSS is broken.  So what to do when a workaround is a bad idea (for the reasons Nelson correctly stated) but you also want to protect the reputation of the correct software?

My preferred way to deal with this is to have an off-by-default-option with a very "expressive" name.  It's a bit riskier in an embedded library, but if the option looks something like SSL_OptionSet(s, SSL_KLUDGE_TO_WORKAROUND_BROKEN_TLS_COMPRESSION, 1) hopefully callers of the library will think twice before turning it on by default and the broken software will eventually get fixed.  When an end-user turns on an off-by-default option like this, the end user is forced to recognize which software is actually broken and which software is being "nice" providing a workaround.  This protects the reputation of the correct software.  If NSS had an optional syslog-style logging callback, I'd also suggest a LOG_WARNING callback to indicate compression was requested but had to be turned off due to a bug in the peer's implementation (useful information for a server operational log or a client debug log).

I know this is more coding work and there is some drawback to carrying that workaround code forward, but off-by-default does address many of the concerns Nelson raises, in particular getting the security risk level closer to that of strict compliance code, and helping to reduce harmful drift away from the standard.

I haven't done the deployment analysis to be confident an off-by-default-option is the correct choice in this case (as opposed to a clear error message explaining which peer is broken), but wanted to suggest it as a possible solution if this is a widely deployed problem.

Comment 102

[Adam Langley]

Comment on attachment 419029 [details] [diff] [review]
patch to disable compression when we get a record that appears to not be compressed

Nelson, you're right. This patch is a mistake. I would suggest that it's probably still worth detecting this and returning a specific error code (i.e. SERVER_MISTAKENLY_SENT_UNCOMPRESSED_RECORD) so save people debugging time in the future.

We could argue for a long time about exactly what to do, so either yourself or wtc should probably just choose something. (I can still code it up.)

Firefox might end up not enabling compression for this reason, but I'm willing to break a few users with Chrome for the greater good. (We'll have to watch the bug tracker and see how bad it is. Since it's SSLv3 only, I suspect it'll be ok.)

Comment 103

[Georg v. Zezschwitz]

Attachment: Patch to enable SSL compression for non-system-zlib-Systems

I am very prepared to receive a rejection. This patch change the Makefile and zlib.h of nss so that not only Linux/BSD get SSL support. It uses the NSS zlib library instead of the external system one. Ugly thing is, that - at least with my building process, compiling Firefox+NSS at once, the definition from modules/zlib/src/mozzconf.h (intended for modules/zlib - Version) are included e.g. in security/nss/lib/ssl/ssl3con.c. This makes the references to zlib functions like "inflate" using the name of the module zlib-Version (e.g. _MOZ_Z_deflateEnd). However, linking against NSSs zlib library should be preferable.

Any more elegant solution would be appreciated, but I am not that deep in the building system :-(.
However, this change increases the "target-group" from 5% to hopefully 100%. Tested with MacOS 10.6, library used in Firefox and Thunderbird.

Comment 104

[Georg v. Zezschwitz]

Some test results:
My impression is, that the described SSLv3-Resume-Bug applies to all OpenSSL versions up to OpenSSL 1.0.0beta2 and is fixed with OpenSSL 1.0.0beta4. I observed the faulty SSLv3 resume compiling Apache 2.2.14 (and other versions) in its native version and with the native OpenSSL versions.

So most likely most Apache / mod_ssl / OpenSSL - combinations are affected.

I did some quick tests:

Firefox:
SSL compression saved about 7% loading my GoogleMail start page.
Biggest gains are - however - with 

Thunderbird:
My Debian Lenny mail server (Exim with StartTLS, CourierIMAP SSL) is "ready by default" for SSL compression.
Savings for the cycle
 - Sending a mail to myself via SMTP / TLS
 - Writing to Sent-IMAP-Folder
 - Retrieving it from INBOX-Folder
were:
- reduction to 82% of size for a 505 kB JPEG data (Base64-encoding compress gains)
- reduction to 38% of size for a 220 kB Word document (Base64 savings plus "Silly user saving potential")

So, the much more delighted future users of SSL compression are certainly the Thunderbird ones.

Comment 105

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 102,
Adam, I'd be quite happy to have a new error code, such as 
SSL_ERROR_COMPRESSED_RECORD_EXPECTED
but I wouldn't be surprised if Mozilla products report it to the user as
"unknown reason" instead of a meaningful error message.

Comment 106

[Adam Langley]

Attachment: patch to add a new error code when we detect missing compression

Comment 107

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 419320 [details] [diff] [review]
patch to add a new error code when we detect missing compression

This patch is fine, as far as it goes, but is incomplete.
Any time an error code is added, a string for it must also be 
added to the appropriate file of error strings.  For SSL errors
the relevant file is presently security/nss/cmd/lib/SSLerrs.h

Comment 108

[Adam Langley]

Attachment: patch to add a new error code when we detect missing compression (v2)

> Any time an error code is added, a string for it must also be
> added to the appropriate file of error strings.

Opps, thanks Nelson.

Comment 109

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 419431 [details] [diff] [review]
patch to add a new error code when we detect missing compression (v2)

r=nelson,  Thanks.

Comment 110

[Nelson Bolyard (seldom reads bugmail)]

In comment 90, I wondered what remained to be done for this bug. 
I see now that the remaining work is to enable this compression code to 
be built and linked on all platforms.  

In comment 85, I gave r+ to attachment 411279 [details] [diff] [review], a makefile patch 
that (I believed) enables it to build on all platforms.  I asked that Bob 
Relyea should super-review it because I suspected that he would want to 
tweak it to use the "system" zlib library by default on Linux.  But I don't
think we should delay the TLS compression feature on all platforms while 
waiting for that optimization.  

Georg, please test the patch in attachment 411279 [details] [diff] [review].  
If it suites your purposes, please let us know here, and I will commit it.

Comment 111

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 419232 [details] [diff] [review]
Patch to enable SSL compression for non-system-zlib-Systems

Georg,
I believe the makefile portion of this patch will not work with MSVC on Windows.
Please let us know if the previous patch in attachment 411279 [details] [diff] [review] will work for you.

Please explain to me the circumstances under which all the additional #undefs
you added are necessary.  I believe you added them to counteract the large set
of #defines in 
http://mxr.mozilla.org/mozilla-central/source/modules/zlib/src/mozzconf.h#48
but it is completely unclear to me how that set of #defines could ever affect
NSS, since (AFAICT) NSS never #includes the mozzconf.h header file in its build.
(does it?).

Comment 112

[Georg v. Zezschwitz]

Patch 411279 does not anymore apply cleanly to lib/ssl/config.mk, as config.mk
was changed in 1.30.

However, it seems to include all changes done to config.mk in 1.30, so I applied it to rev 1.29 of config.mk

This patch does not work for my "full building" (same errors as in comment 91):
I followed the building instructions for Firefox with MacOS. Then, I linked the nss subdirectory of the Mercurial "mozilla-central" tree sources to the nss folder of the NSS CVS checkout:

ls -l mozilla/security/nss
lrwxr-xr-x  1 gvz  gvz  48 23 Dez 21:48 mozilla/security/nss -> /Users/gvz/Projects/moz.cvs/mozilla/security/nss

for a full built of Firefox including the NSS working dir.

"cd mozilla/obj-ff-dbg ; make"

This makes e.g. ssl3con.c being compiled this way:

gcc-4.2 -arch i386 -o /Users/gvz/Projects/mozilla/obj-ff-dbg/nss/ssl/ssl3con.o -c -g -fPIC -Di386 -Wmost -fpascal-strings -fno-common -pipe -DDARWIN -DHAVE_STRERROR -DHAVE_BSD_FLOCK -isysroot /Developer/SDKs/MacOSX10.5.sdk -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_gvz -DNSS_ENABLE_ECC -DUSE_UTIL_DIRECTLY -DNSS_ENABLE_ZLIB -I/Users/gvz/Projects/mozilla/obj-ff-dbg/dist/include/nspr -I/Users/gvz/Projects/mozilla/obj-ff-dbg/security/manager/../../dist/include -I/Users/gvz/Projects/mozilla/obj-ff-dbg/dist/public/nss -I/Users/gvz/Projects/mozilla/obj-ff-dbg/dist/private/nss -I/Users/gvz/Projects/mozilla/obj-ff-dbg/dist/include   ssl3con.c

The reason for the inclusion of the defines is that the line:

#ifdef NSS_ENABLE_ZLIB
#include "zlib.h"
#endif

in e.g. ssl3con.c finds the zlib.h from the modules/zlib-directory.

The second -I option (-I/Users/gvz/Projects/mozilla/obj-ff-dbg/security/manager/../../dist/include) points to a directory with zlib.h, and this zlib.h resolves to the modules/zlib-version:

ls -l dist/include/zlib.h
lrwxr-xr-x  1 gvz  gvz  51 30 Dez 11:00 dist/include/zlib.h -> /Users/gvz/Projects/mozilla/modules/zlib/src/zlib.h

This was the point where I gave up to and decide to go the ugly undef-way.

Comment 113

[Nelson Bolyard (seldom reads bugmail)]

Comment 112 documents a bug in the way that Firefox builds NSS.  
The correct fix, IMO, is not to add all those #undefs.  
It is to correct the set and order of the -I directories in the CC 
command line, when NSS is being built by Firefox.  

There should probably be a new and separate bug filed about that, 
filed against firefox, component BUILD, or something like that.

Comment 114

[Georg v. Zezschwitz]

Nelson, the "ugly #undefs" are probably not needed. I rebuilt and it was - on my Mac - sufficient to include:

  CFLAGS := -I../zlib $(CFLAGS)

to mozilla/security/nss/lib/ssl/Makefile (as in my attachment 419232 [details] [diff] [review]). It might be even sufficient to change the ordering of the CFLAGS from 

-I(...)/obj-ff-dbg/security/manager/../../dist/include
-I(...)/obj-ff-dbg/dist/public/nss
-I(...)/mozilla/obj-ff-dbg/dist/private/nss

to

-I(...)/obj-ff-dbg/dist/public/nss
-I(...)/mozilla/obj-ff-dbg/dist/private/nss
-I(...)/obj-ff-dbg/security/manager/../../dist/include

However, if you think that this is not in the scope of this bug or NSS at all, I can open a bug as suggested. Anyway, as soon as you release the new NSS, people will notice that building fails.
So, shall we - with patch 411279 to be included - close this bug at here?

Comment 115

[Robert Relyea]

Comment on attachment 411279 [details] [diff] [review]
nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v3

r+

This looks like it should still work when USE_SYSTEM_ZLIB is set (the previous code required $(ZLIB_LIBS) to be set as well).

The code also makes it clear the freebl is always linked, even if BYPASS isn't set. This was true of the previous code, it just wasn't clear.

bob

Comment 116

[Wan-Teh Chang]

Comment on attachment 419431 [details] [diff] [review]
patch to add a new error code when we detect missing compression (v2)

>+ER3(SSL_ERROR_COMPRESSED_RECORD_EXPECTED,      (SSL_ERROR_BASE + 113),
>+"Server mistakenly sent an uncompressed record.")

Nelson, following the naming convention of existing SSL error codes,
I think this error code should be named SSL_ERROR_RX_UNEXPECTED_UNCOMPRESSED_RECORD.
What do you think?

Comment 117

[Wan-Teh Chang]

Attachment: patch to add a new error code when we detect missing compression, v3 (checked in)

I updated Adam's patch and checked it in on the NSS trunk (NSS 3.12.6).

I renamed the new error code SSL_ERROR_RX_UNEXPECTED_UNCOMPRESSED_RECORD.
It now has the value SSL_ERROR_BASE + 114.

Checking in cmd/lib/SSLerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.10; previous revision: 1.9
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.134; previous revision: 1.133
done
Checking in lib/ssl/sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.10; previous revision: 1.9
done

Comment 118

[Wan-Teh Chang]

Attachment: nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v4 (checked in)

I updated this patch to the current NSS trunk.  I also added a
variable NSS_ENABLE_ZLIB, set to 1 by default, that allows Mozilla
to turn off TLS deflate compression to work around the build issue
that Georg reported.

I'm requesting a new review because of these changes.

Georg, are you interested in using TLS deflate compression, or do
you just want to solve the Mozilla build issue?  Your hack
  CFLAGS := -I../zlib $(CFLAGS)
should work, and reordering the -I flags as you described in comment
114 should also work.  The best solution is to ensure that there is
only one zlib.h (or none when using system zlib) in the -I directories.
This requires coordination between Mozilla's and NSS's build systems.

Comment 119

[Wan-Teh Chang]

Although I think my patch attachment 425150 [details] [diff] [review] has a low risk (tested
on Linux, Mac, and Windows), I am fine with addressing the build
issues after NSS 3.12.6.

Comment 120

[Robert Relyea]

Comment on attachment 425150 [details] [diff] [review]
nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v4 (checked in)

r+ rrelyea

Comment 121

[Wan-Teh Chang]

Comment on attachment 425150 [details] [diff] [review]
nss/lib/ssl makefile changes to use either system zlib or nss/lib/zlib, v4 (checked in)

I checked in the patch on the NSS trunk (NSS 3.12.6).

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/ssl/Makefile,v  <--  Makefile
new revision: 1.9; previous revision: 1.8
done
Checking in config.mk;
/cvsroot/mozilla/security/nss/lib/ssl/config.mk,v  <--  config.mk
new revision: 1.31; previous revision: 1.30
done

Comment 122

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 419232 [details] [diff] [review]
Patch to enable SSL compression for non-system-zlib-Systems

This patch is no longer needed.

Comment 123

[Wan-Teh Chang]

I filed bug 580679 to fix the build issue in Mozilla (see comment 112
and comment 113).

Comment 124

[Nelson Bolyard (seldom reads bugmail)]

Adam & Wan-Teh,
When you added the compression code, you added a compression context pointer
to the "spec" structures.  That pointer itself is clearly protected by the 
locks that protect the spec structures.  

What lock did you intend to protect the compression context?  
Was is the spec lock?  
Or was it the respective "buf lock" (e.g. xmitBufLock or RecvBufLock) ??  

Some patches have been submitted (I was reviewing them today) that seem to 
assume that it is the spec lock, and hence a read-lock on the spec is not 
sufficient to hold while compression or decompression is taking place.   
The intent of the "spec" structure was (at one time) that it would not be
changed except during a handshake, and hence reader locks would suffice for
processing ordinary application_data records.  

Thoughts?

Comment 125

[Adam Langley]

(In reply to comment #124)
> What lock did you intend to protect the compression context?  
> Was is the spec lock?  
> Or was it the respective "buf lock" (e.g. xmitBufLock or RecvBufLock) ??  

I believe that it should be the respective buf lock. Otherwise the contention on the spec lock would be excessive.

If I screwed that up anywhere, please let me know. A few AssertHeld's may be in order.

AGL