Closed Bug 1328955 Opened 3 years ago Closed 3 years ago

When a client tries TLS1.3 with Early Data and a server falls back to tls1.2, we should reconnect using tls 1.3 without Early Data

Categories

(Core :: Networking: HTTP, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-next])

Attachments

(1 file, 2 obsolete files)

- Client negotiates TLS 1.3 w/ 0-RTT
- Server reconfigures to be TLS 1.2 only
- Client sends 1.3 CH with Early Data
- Server reads CH, negotiates 1.2, sends SH, and then tries to read ClientKeyExchange but gets early application data instead. This causes a handshake failure.

nss will return a new error code for this situation and necko should reconnect using tls 1.2.
THe new code will be added in bug 1328952.
Whiteboard: [necko-next]
Depends on: 1328952
If the server sends a handshake_failure alert, PSM should have already fallbacked. Doesn't it?
(In reply to Masatoshi Kimura [:emk] from comment #1)
> If the server sends a handshake_failure alert, PSM should have already
> fallbacked. Doesn't it?

OK, probably the server will send an unexpected_message alert.

But, if we retry at all, we should use TLS *1.3* without 0RTT. It will not loosen security. 1.2-only servers can handle 1.3 CH unless 0RTT is used or they are TLS 1.3 intolerant (the latter case is already handled by insecure fallback).
(In reply to Masatoshi Kimura [:emk] from comment #2)
> (In reply to Masatoshi Kimura [:emk] from comment #1)
> > If the server sends a handshake_failure alert, PSM should have already
> > fallbacked. Doesn't it?
> 
> OK, probably the server will send an unexpected_message alert.
> 
> But, if we retry at all, we should use TLS *1.3* without 0RTT. It will not
> loosen security. 1.2-only servers can handle 1.3 CH unless 0RTT is used or
> they are TLS 1.3 intolerant (the latter case is already handled by insecure
> fallback).

I was attending to retry it with tls 1.3 without 0RTT, just did not have time to fix the title (I wrote it quickly, because I on vacation, and made a mistake).
Summary: When a client tries TLS1.3 with Early Data and a server falls back to tls1.2, we should reconnect using tls 1.2 → When a client tries TLS1.3 with Early Data and a server falls back to tls1.2, we should reconnect using tls 1.3 without Early Data
Attached patch bug_1328955.patch (obsolete) — Splinter Review
Attached patch bug_1328955.patch (obsolete) — Splinter Review
Attachment #8826596 - Attachment is obsolete: true
Attachment #8826616 - Flags: review?(mcmanus)
Comment on attachment 8826616 [details] [diff] [review]
bug_1328955.patch

Review of attachment 8826616 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +944,5 @@
>      // auth provider, beliving the cached credentials are wrong and asking for
>      // the password mistakenly again from the user.
> +    if (((reason == NS_ERROR_NET_RESET || reason == NS_OK) &&
> +        !(mCaps & NS_HTTP_STICKY_CONNECTION)) ||
> +        reason == NS_ERROR_DOWNGRADE_WITH_EARLY_DATA) {

I don't think we want to bypass the sticky rule here (though it doesn't make any sense that it would come into play).. so I think this should read

  if (((reason == NS_ERROR_NET_RESET ||
        reason == NS_OK ||
        reason == NS_ERROR_DOWNGRADE_WITH_EARLY_DATA) &&
        !(mCaps & NS_HTTP_STICKY_CONNECTION)) {

::: xpcom/base/ErrorList.h
@@ +718,5 @@
>    ERROR(NS_ERROR_CMS_VERIFY_NOT_YET_ATTEMPTED,     FAILURE(1039)),
>    ERROR(NS_ERROR_CMS_VERIFY_CERT_WITHOUT_ADDRESS,  FAILURE(1040)),
>    ERROR(NS_ERROR_CMS_ENCRYPT_NO_BULK_ALG,          FAILURE(1056)),
>    ERROR(NS_ERROR_CMS_ENCRYPT_INCOMPLETE,           FAILURE(1057)),
> +  ERROR(NS_ERROR_DOWNGRADE_WITH_EARLY_DATA,        FAILURE(12128)),

you don't need to do this if you don't need to localize strings at the xpcom layer - and since we dont want to ever return this from necko I think that applies. You can just include the nss header as dom already does
https://dxr.mozilla.org/mozilla-central/source/dom/network/TCPSocket.cpp#35

in the rare case where we won't restart, we should map the code to whatever it used to be so we don't start adding in new strings

and then we don't need to worry about constants getting out of sync.
Attachment #8826616 - Flags: review?(mcmanus)
Attachment #8826616 - Attachment is obsolete: true
Attachment #8828123 - Flags: review?(mcmanus)
Comment on attachment 8828123 [details] [diff] [review]
bug_1328955.patch

Review of attachment 8828123 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +946,5 @@
>      // auth provider, beliving the cached credentials are wrong and asking for
>      // the password mistakenly again from the user.
> +    if ((reason == NS_ERROR_NET_RESET ||
> +         reason == NS_OK ||
> +         reason == psm::GetXPCOMFromNSSError(SSL_ERROR_DOWNGRADE_WITH_EARLY_DATA)) &&

nice :)
Attachment #8828123 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8865358a4a
Quickly reconnect if tls1.3 handshake with early-data is downgraded. r=mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f8865358a4a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.