1328955 - 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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dragana Damjanovic [:dragana]]

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

Comment 1

[Masatoshi Kimura [:emk]]

If the server sends a handshake_failure alert, PSM should have already fallbacked. Doesn't it?

Comment 2

[Masatoshi Kimura [:emk]]

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

Comment 3

[Dragana Damjanovic [:dragana]]

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

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1328955.patch

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1328955.patch

Comment 6

[Patrick McManus [: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.

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1328955.patch

Comment 8

[Patrick McManus [: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 :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8865358a4a
Quickly reconnect if tls1.3 handshake with early-data is downgraded. r=mcmanus

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3f8865358a4a