Closed Bug 583908 Opened 11 years ago Closed 11 years ago

Enable TLS false start in Mozilla.

Categories

(Core :: Security: PSM, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b3

People

(Reporter: wtc, Assigned: wtc)

References

(Depends on 1 open bug, )

Details

Attachments

(4 files, 1 obsolete file)

Attached patch Proposed patchSplinter Review
The attached patch enables TLS false start in Mozilla.

It adds a new preference, security.ssl.enable_false_start,
with a default value of true.

To review this patch, look at this MXR query for a similar
preference security.enable_tls_session_tickets:
http://mxr.mozilla.org/mozilla-central/search?string=security.enable_tls_session_tickets

This patch similar changes for security.ssl.enable_false_start.

Note: there are servers that are incompatible with TLS
false start.  So this patch has a low but nonzero
compatibility risk.  We can eliminate the risk completely
by changing the default value of security.ssl.enable_false_start
to false, but then very few users will be testing this
feature, making it harder to discover and fix such
incompatible servers.
Attachment #462252 - Flags: superreview?(sayrer)
Attachment #462252 - Flags: review?(kaie)
Attachment #462252 - Flags: approval2.0?
Attachment #462252 - Flags: superreview?(sayrer)
Attachment #462252 - Flags: review?(sayrer)
Attachment #462252 - Flags: approval2.0?
Attachment #462252 - Flags: approval2.0+
Comment on attachment 462252 [details] [diff] [review]
Proposed patch

this doesn't need SR. Looks good as long as kaie is ok with it.
Attachment #462252 - Flags: review?(sayrer) → review+
Comment on attachment 462252 [details] [diff] [review]
Proposed patch

I pushed the patch to mozilla-central in changeset 5a649ed4ab65:
http://hg.mozilla.org/mozilla-central/rev/5a649ed4ab65
TLS false start broke a Mozilla mochitest named test_bug329869.html.
So I have to change the default to false (disabled) until I find
out what's wrong.
Attachment #462332 - Flags: review?
Attachment #462332 - Flags: review? → review?(gavin.sharp)
Attachment #462332 - Flags: review?(gavin.sharp) → review+
The test failed with these messages:

314 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_bug329869.html | FAILURE: , expected secure got broken
315 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_bug329869.html | FAILURE: for 'secure' expected flags [0,0,0],
Comment on attachment 462332 [details] [diff] [review]
Change the default to false

This patch was pushed to mozilla-central in changeset b28c5262fd9e:
http://hg.mozilla.org/mozilla-central/rev/b28c5262fd9e
I verified both inside a debugger and with the NSS ssltap tool
that Firefox is sending application data early when TLS false
start is enabled.

Here is a sample ssltap trace:

<-- [
[server_hello and certificate handshake messages omitted for brevity]
(1063 bytes of 4)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 16 03 01 00  04                                     | .....
   type    = 22 (handshake)
   version = { 3,1 }
   length  = 4 (0x4)
   handshake {
   0: 0e 00 00 00                                         | ....
      type = 14 (server_hello_done)
      length = 0 (0x000000)
   }
}
]
--> [
(314 bytes of 262, with 47 left over)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 16 03 01 01  06                                     | .....
   type    = 22 (handshake)
   version = { 3,1 }
   length  = 262 (0x106)
   handshake {
   0: 10 00 01 02                                         | ....
      type = 16 (client_key_exchange)
      length = 258 (0x000102)
         ClientKeyExchange {
            message = {...}
         }
   }
}
(314 bytes of 1, with 41 left over)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 14 03 01 00  01                                     | .....
   type    = 20 (change_cipher_spec)
   version = { 3,1 }
   length  = 1 (0x1)
   0: 01                                                  | .
}
(314 bytes of 36)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 16 03 01 00  24                                     | ....$
   type    = 22 (handshake)
   version = { 3,1 }
   length  = 36 (0x24)
            < encrypted >
}
]
--> [
(378 bytes of 373)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 17 03 01 01  75                                     | ....u
   type    = 23 (application_data)
   version = { 3,1 }
   length  = 373 (0x175)
            < encrypted >
}
]
<-- [
(47 bytes of 1, with 41 left over)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 14 03 01 00  01                                     | .....
   type    = 20 (change_cipher_spec)
   version = { 3,1 }
   length  = 1 (0x1)
   0: 01                                                  | .
}
(47 bytes of 36)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 16 03 01 00  24                                     | ....$
   type    = 22 (handshake)
   version = { 3,1 }
   length  = 36 (0x24)
            < encrypted >
}
]
<-- [
(503 bytes of 498)
SSLRecord { [Tue Aug  3 12:57:23 2010]
   0: 17 03 01 01  f2                                     | .....
   type    = 23 (application_data)
   version = { 3,1 }
   length  = 498 (0x1f2)
            < encrypted >
}
]
This patch changes the default of security.ssl.enable_false_start
back to true (TLS false start enabled).  The patch includes the
necessary NSS changes, reviewed in bug 525092 comment 28.

The benefit of this patch is to reduce the connection setup
latency due to SSL full handshakes, and to test this SSL
protocol change as early as possible in the Firefox 4.0 beta
cycle.

See the Note at the end of comment 0 for the compatibility
risk of this patch.  The regression risk of this patch is
now lower than before after fixing the bug uncovered by
the test_bug329869.html mochitest failure.
Attachment #462666 - Flags: review?(sayrer)
Attachment #462666 - Flags: approval2.0?
Attachment #462666 - Flags: review?(sayrer)
Attachment #462666 - Flags: review+
Attachment #462666 - Flags: approval2.0?
Attachment #462666 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/79aa28daf1f4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 462252 [details] [diff] [review]
Proposed patch

r=kaie
Attachment #462252 - Flags: review?(kaie) → review+
Depends on: 585061
In bug 585061 comment 13 Wan-Teh said he wants to address that bug in this bug, so I'm reopening this one.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I leave the security.ssl.enable_false_start preference
alone.  It is harmless to have a hidden preference that
does nothing.
Attachment #463652 - Flags: review?(kaie)
I attached the wrong patch.  Sorry about the confusion.
Attachment #463652 - Attachment is obsolete: true
Attachment #463653 - Flags: review?(kaie)
Attachment #463652 - Flags: review?(kaie)
Comment on attachment 463653 [details] [diff] [review]
Test the SSL_ENABLE_FALSE_START macro (correct patch)

r=kaie
Attachment #463653 - Flags: review?(kaie) → review+
the latest patch fixes the compilation time problem.

but what happens if:
- mozilla is compiled against 3.12.8
- user has multiple copies of NSS installed on the system
- for some reason at runtime an older version of NSS gets chosen?
Comment on attachment 463653 [details] [diff] [review]
Test the SSL_ENABLE_FALSE_START macro (correct patch)

Requesting approval2.0.

The benefit of the patch is to allow building
mozilla-central against a released version of
NSS.  (TLS false start is only in NSS 3.12.8
Beta.)

The patch has no compatibility or regression
risk.
Attachment #463653 - Flags: approval2.0?
(In reply to comment #14)
The SSL_OptionSetDefault calls will do nothing and return
SECFailure.  Since the code ignores the return value of
SSL_OptionSetDefault, it does no harm.
Attachment #463653 - Flags: approval2.0? → approval2.0+
(In reply to comment #16)
> it does no harm.

good, thanks for verifying
Comment on attachment 463653 [details] [diff] [review]
Test the SSL_ENABLE_FALSE_START macro (correct patch)

http://hg.mozilla.org/mozilla-central/rev/1907ad5a3f9a
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Blocks: 591523
No longer blocks: 591523
Depends on: 591523
You need to log in before you can comment on or make changes to this bug.