Closed Bug 1250568 Opened 8 years ago Closed 8 years ago

Allow enabling TLS 1.3

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: mt, Unassigned)

References

Details

Attachments

(5 files, 10 obsolete files)

MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler
58 bytes, text/x-review-board-request
keeler
: review+
Details
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r=ekr
58 bytes, text/x-review-board-request
ekr
: review+
Details
MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r=keeler
58 bytes, text/x-review-board-request
keeler
: review+
Details
MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r=keeler
58 bytes, text/x-review-board-request
keeler
: review+
Details
broken_sites_tls_1_3.txt
1.67 KB, text/plain
Details 
This will eventually need a bit more infrastructure, but for the moment, expanding the allowed range of values for SSL/TLS versions should allow the brave to enable draft versions of TLS 1.3.  It should also let us get a head-start on compatibility testing.
Is NSS_ENABLE_TLS_1_3 defined by default? Do we have to handle the error gracefully in case the system NSS does not set NSS_ENABLE_TLS_1_3?
The code detects what version of TLS is supported by NSS.  But apparently, despite what I've been told, we don't set NSS_ENABLE_TLS_1_3, so nothing has changed here.
https://reviewboard.mozilla.org/r/36367/#review32977

FWIW, it looks like you accidentally typo'ed bsmedberg as bsmegberg in your r? syntax, so he might not have noticed the review.
Attachment #8723124 - Flags: review?(benjamin)
Cykesiopka, thanks.
Comment on attachment 8723124 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg?keeler

https://reviewboard.mozilla.org/r/36367/#review32999

data-review=me with that change

::: toolkit/components/telemetry/Histograms.json:1334
(Diff revision 1)
>    "SSL_HANDSHAKE_VERSION": {

This histogram as well as the new ones below need the following data which is now required:

"bug_numbers"
"alert_emails"
Attachment #8723124 - Flags: review?(benjamin) → review+
Comment on attachment 8723124 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36367/diff/1-2/
Attachment #8723124 - Attachment description: MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r?bsmegberg,keeler → MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg?keeler
Attachment #8723124 - Flags: review?(dkeeler)
Comment on attachment 8723125 [details]
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r?ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36369/diff/1-2/
Comment on attachment 8723124 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg?keeler

I don't know why Mozreview in its infinite wisdom decided to drop the review request.  It still stands.  BTW, I've added alert_emails entries for all the histograms that looked security-relevant.  I hope that the alerts aren't too spammy.
Attachment #8723124 - Flags: review?(dkeeler)
Comment on attachment 8723124 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg?keeler

https://reviewboard.mozilla.org/r/36367/#review33041

LGTM.

::: security/manager/ssl/nsNSSIOLayer.cpp:1676
(Diff revision 2)
> -                               SSL_LIBRARY_VERSION_TLS_1_2 };
> +                               SSL_LIBRARY_VERSION_TLS_1_3 };

I don't think this should change, actually. If 'security.tls.version.fallback-limit' is set to 4 and NSS is compiled with TLS 1.3 support, this should already do what we want.

::: toolkit/components/telemetry/Histograms.json:1340
(Diff revision 2)
> -    "description": "SSL Version (1=tls1, 2=tls1.1, 3=tls1.2)"
> +    "description": "SSL Version (1=tls1, 2=tls1.1, 3=tls1.2, 4=tls1.3)"

Might be nice to update the comment at https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/security/manager/ssl/nsNSSCallbacks.cpp#1149 as well.

::: toolkit/components/telemetry/Histograms.json:8014
(Diff revision 2)
>    "SSL_AUTH_DSA_KEY_SIZE_FULL": {

We don't enable these cipher suites any more, so this can be removed (or we can do it in a follow-up).
Attachment #8723124 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #11)
> I don't think this should change, actually. If
> 'security.tls.version.fallback-limit' is set to 4 and NSS is compiled with
> TLS 1.3 support, this should already do what we want.

Then we don't fallback to TLS 1.2? I don't think it's possible, given that ~1% servers are TLS 1.3 intolerant.
Luckily, TLS 1.3 has a built-in anti-downgrade mechanism, so we don't have to worry about the downgrade attack when we fallback to 1.2.
The desired behavior is that we offer TLS 1.3 and then fall back to TLS 1.2 if it fails.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #11)
> ::: security/manager/ssl/nsNSSIOLayer.cpp:1676
> (Diff revision 2)
> > -                               SSL_LIBRARY_VERSION_TLS_1_2 };
> > +                               SSL_LIBRARY_VERSION_TLS_1_3 };

Ah, but this should not be changed. David is right. (MozReview should really show more context by default...)
https://reviewboard.mozilla.org/r/36367/#review33041

> I don't think this should change, actually. If 'security.tls.version.fallback-limit' is set to 4 and NSS is compiled with TLS 1.3 support, this should already do what we want.

Quite right.  We want this to fall back to 1.2 by default.

> We don't enable these cipher suites any more, so this can be removed (or we can do it in a follow-up).

bug 1251133
Comment on attachment 8723124 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36367/diff/2-3/
Comment on attachment 8723125 [details]
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r?ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36369/diff/2-3/
Depends on: 1254306
No longer depends on: 1254306
Review commit: https://reviewboard.mozilla.org/r/44005/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44005/
Attachment #8737554 - Flags: review?(dkeeler)
Attachment #8737554 - Flags: review?(benjamin)
Attachment #8737555 - Flags: review?(ekr)
Attachment #8737556 - Flags: review?(dkeeler)
Attachment #8737557 - Flags: review?(dkeeler)
Attachment #8723124 - Attachment is obsolete: true
Attachment #8723125 - Attachment is obsolete: true
Attachment #8723125 - Flags: review?(ekr)
Comment on attachment 8737554 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r?bsmedberg,keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44005/diff/1-2/
Comment on attachment 8737555 [details]
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r?ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44007/diff/1-2/
Comment on attachment 8737556 [details]
MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44009/diff/1-2/
Comment on attachment 8737557 [details]
MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44011/diff/1-2/
Blocks: 1253525
Review commit: https://reviewboard.mozilla.org/r/44025/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44025/
Attachment #8737595 - Flags: review?(dkeeler)
Attachment #8737595 - Flags: review?(benjamin)
Attachment #8737596 - Flags: review?(ekr)
Attachment #8737597 - Flags: review?(dkeeler)
Attachment #8737598 - Flags: review?(dkeeler)
Attachment #8737554 - Attachment is obsolete: true
Attachment #8737554 - Flags: review?(dkeeler)
Attachment #8737554 - Flags: review?(benjamin)
Attachment #8737555 - Attachment is obsolete: true
Attachment #8737555 - Flags: review?(ekr)
Attachment #8737556 - Attachment is obsolete: true
Attachment #8737556 - Flags: review?(dkeeler)
Attachment #8737557 - Attachment is obsolete: true
Attachment #8737557 - Flags: review?(dkeeler)
Comment on attachment 8737595 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r?bsmedberg,keeler

https://reviewboard.mozilla.org/r/44025/#review40745
Attachment #8737595 - Flags: review?(dkeeler) → review+
Comment on attachment 8737597 [details]
MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r?keeler

https://reviewboard.mozilla.org/r/44029/#review40749
Attachment #8737597 - Flags: review?(dkeeler) → review+
Comment on attachment 8737598 [details]
MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r?keeler

https://reviewboard.mozilla.org/r/44031/#review40753

LGTM.

::: devtools/shared/webconsole/network-helper.js:529
(Diff revision 1)
>     *                    * "secure": the connection was properly secured.
>     *          If state == broken:
>     *            - errorMessage: full error message from
>     *                            nsITransportSecurityInfo.
>     *          If state == secure:
>     *            - protocolVersion: one of TLSv1, TLSv1.1, TLSv1.2.

Let's add TLSv1.3 here.

::: devtools/shared/webconsole/network-helper.js:713
(Diff revision 1)
>     * description.
>     *
>     * @param Number version
>     *        One of nsISSLStatus version constants.
>     * @return string
>     *         One of TLSv1, TLSv1.1, TLSv1.2 if @param version is valid,

nit: update documentation

::: devtools/shared/webconsole/network-helper.js:724
(Diff revision 1)
>          return "TLSv1";
>        case Ci.nsISSLStatus.TLS_VERSION_1_1:
>          return "TLSv1.1";
>        case Ci.nsISSLStatus.TLS_VERSION_1_2:
>          return "TLSv1.2";
> +      case Ci.nsISSLStatus.TLS_VERSION_1_3:

I guess there's a test for this function we should probably update: test_security-info-protocol-version.js

::: security/manager/ssl/nsISSLStatus.idl:23
(Diff revision 1)
>  
>    const short SSL_VERSION_3   = 0;
>    const short TLS_VERSION_1   = 1;
>    const short TLS_VERSION_1_1 = 2;
>    const short TLS_VERSION_1_2 = 3;
> +  const short TLS_VERSION_1_3 = 4;

As a heads-up, it looks like both nsITLSServerSocket.idl and nsISSLSocketControl.idl also define TLS_VERSION_1\* - it might be relevant to add 1_3 to those as well (then again, they really only seem to use 1_2).
Attachment #8737598 - Flags: review?(dkeeler) → review+
Attachment #8737595 - Attachment is obsolete: true
Attachment #8737595 - Flags: review?(benjamin)
Attachment #8737596 - Attachment is obsolete: true
Attachment #8737596 - Flags: review?(ekr)
Attachment #8737597 - Attachment is obsolete: true
Attachment #8737598 - Attachment is obsolete: true
Comment on attachment 8737907 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler

https://reviewboard.mozilla.org/r/44171/#review41021

::: toolkit/components/telemetry/Histograms.json:8059
(Diff revision 1)
>      "description": "The number of errors using the HomeProvider due to a locked DB. *** No longer needed (bug 1156565). Delete histogram and accumulation code! ***"
>    },
> +  "SSL_TLS13_INTOLERANCE_REASON_PRE": {
> +    "alert_emails": ["seceng-telemetry@mozilla.com"],
> +    "bug_numbers": [1250568],
> +    "expires_in_version": "never",

Why is this "never"? If this is an operational metric, should this be opt-out instead of opt-in? Also how will you be monitoring this? Current automated alerting doesn't work with enumerated histograms in a useful way.

::: toolkit/components/telemetry/Histograms.json:8062
(Diff revision 1)
> +    "alert_emails": ["seceng-telemetry@mozilla.com"],
> +    "bug_numbers": [1250568],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 64,
> +    "description": "detected symptom of TLS 1.3 intolerance, before considering historical info"

This needs more detail. What do these 64 values mean? You either need to list them or reference some other enumeration value in the code.
Attachment #8737907 - Flags: review?(benjamin)
Attachment #8737907 - Flags: feedback-
Comment on attachment 8737907 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler

https://reviewboard.mozilla.org/r/44171/#review41071

Did this change since the last review? Why isn't there an interdiff? If it hasn't changed, r=me carries over.
Attachment #8737907 - Flags: review?(dkeeler) → review+
Comment on attachment 8737909 [details]
MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r=keeler

https://reviewboard.mozilla.org/r/44175/#review41073
Attachment #8737909 - Flags: review?(dkeeler) → review+
Comment on attachment 8737910 [details]
MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r=keeler

https://reviewboard.mozilla.org/r/44177/#review41077

LGTM.
Attachment #8737910 - Flags: review?(dkeeler) → review+
So, I get an r+, address the nits, mozreview forgets the r+ and asks for review again, then I end up with f-.  Great.

I will address the concerns, but that might take some time.
Attachment #8737908 - Flags: review?(ekr) → review+
Comment on attachment 8737908 [details]
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r=ekr

https://reviewboard.mozilla.org/r/44173/#review43515

LGTM
https://reviewboard.mozilla.org/r/44171/#review41021

> Why is this "never"? If this is an operational metric, should this be opt-out instead of opt-in? Also how will you be monitoring this? Current automated alerting doesn't work with enumerated histograms in a useful way.

Actually, I just copied the existing ones on the assumption that whoever put them there knew what they were doing.  However, I will say that this is something we look at whenever we make changes to the TLS implementation, and while some of the existing metrics are now less useful to the point they are probably due to be removed, they have been necessary to drive decisions about how we manage upgrades to the stack.  Since we are looking to ship TLS 1.3 some time this year, this will be critical in doing that.  And it will be useful for some time after that in determining whether we need all the nasty fallback logic we carry.

never: I don't believe that we need real-time alerting for this, it's primarily of strategic value.

opt-out: I don't have a ready rule-of-thumb for making the privacy trade-off here.  In terms of utility, we currently get only a low volume of measurements on the equivalent opt-out metrics, we could certainly use more measurements if you think the cost acceptable.  In terms of privacy cost, this metric is predominantly about sites rather than users.

expires_in_version: I can't give you an end date for that experiment because I currently can't give you a start date.  I hope that we'll be done before Firefox 60, but can't be certain.

> This needs more detail. What do these 64 values mean? You either need to list them or reference some other enumeration value in the code.

I've modified this, and all the existing values to include a better pointer.
Comment on attachment 8737907 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44171/diff/1-2/
Attachment #8737907 - Flags: feedback- → review?(benjamin)
Comment on attachment 8737908 [details]
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r=ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44173/diff/1-2/
Comment on attachment 8737909 [details]
MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44175/diff/1-2/
Comment on attachment 8737910 [details]
MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44177/diff/1-2/
Comment on attachment 8737907 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler

data-review=me

I'm a little uncomfortable with the "never", so expect me to ping you every 6 months to make sure that you're actually using this data.
Attachment #8737907 - Flags: review?(benjamin) → feedback+
Challenge accepted.  Honestly, I want to remove the whole lot of these.  The data we get from this one should hopefully put us in a place to delete a whole bunch of fragile and complicated code that is doing nasty things in the name of interoperability over security (and performance).  If we get a year of this and it allows us to remove all 8 telemetry probes and all that code, I think that this will have paid off.
Depends on: 1258375
Comment on attachment 8737907 [details]
MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44171/diff/2-3/
Attachment #8737907 - Attachment description: MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r?bsmedberg,keeler → MozReview Request: Bug 1250568 - Add support for TLS1.3 in prefs and telemetry, r=bsmedberg,keeler
Attachment #8737908 - Attachment description: MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r?ekr → MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r=ekr
Attachment #8737909 - Attachment description: MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r?keeler → MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r=keeler
Attachment #8737910 - Attachment description: MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r?keeler → MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r=keeler
Attachment #8737907 - Flags: feedback+
Comment on attachment 8737908 [details]
MozReview Request: Bug 1250568 - Enable building of TLS 1.3, r=ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44173/diff/2-3/
Comment on attachment 8737909 [details]
MozReview Request: Bug 1250568 - Adding ECDHE_PSK suites, r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44175/diff/2-3/
Comment on attachment 8737910 [details]
MozReview Request: Bug 1250568 - Adding TLS 1.3 to nsISSLStatus, r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44177/diff/2-3/
Depends on: 1274718
With TLS 1.3 turned on in Nightly, we appear to have 27 broken sites.

Two of them appear superficially as certificate errors (interscope.com, vzlom-igr.com) but that's because this client change prevented a connection to their primary servers, and forced them to connect to alternate servers with bad configs. 

To review the report:

* Go to this URL: https://tlscanary.mozilla.org/runs/2016-05-23-09-13-21/

* Click on the heading for the column "error.message" to sort

* Click on the error "ssl_error_bad_cert_domain" - choose "remove all" to eliminate these from view (these are caused by the BR workaround changes in bug 1245280)

List of broken sites attached.
Honestly, I'm surprised that this is so few.  Given the small number of sites, maybe we could ask someone to reach out to them.

I see the same problem regarding primary servers on some of those that you filtered out.  For example, feelinwow.com responds with a *.shophippo.com certificate.  https://bdnog.org/ fails in Nightly for me as well.
I'm curious why the insecure fallback to TLS 1.2 didn't work on (the most of) those servers. no_cypher_overlap is a fallback reason.
Hmm, if I load ampli.fr from the command line with an NSPR log, the page loads fine. I could reproduce the no_cypher_overlap error without the log...
My bad. I launched a different binary from the command line.
Filing a follow-up bug to fix the issue.
Depends on: 1275252
(In reply to Martin Thomson [:mt:] from comment #57)
> Honestly, I'm surprised that this is so few.  Given the small number of
> sites, maybe we could ask someone to reach out to them.
> 
> I see the same problem regarding primary servers on some of those that you
> filtered out.  For example, feelinwow.com responds with a *.shophippo.com
> certificate.  https://bdnog.org/ fails in Nightly for me as well.


I just kicked off a canary run on a list of sites taken from the Google CT list - over 2.6 million. That's 10x as big as the sample I used before. I should have results in a few days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: