1320252 - early-data is empty when using TLS 1.3 over HTTP/1 (wo. ALPN) [h20/picotls]

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kazuho Oku]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161124030208

Steps to reproduce:

* use latest Nightly (53.0a1 (2016-11-24) (64-bit))
* set security.tls.enable_0rtt_data to true
* run cli command of [picotls](https://github.com/h2o/picotls) as server, with debugger attached
* connect to the server and send a hand-crafted response
* re-connect to the server

note: picotls does not support ALPN


Actual results:

An empty early-data is seen.

Nightly sends an early-data inidication, but the message that follows the ClientHello is the end-of-early-data alert.

The request arrives after the handshake completes.


Expected results:

The request should be included in early-data.

Comment 1

[Eric Rescorla (:ekr)]

:dragana, any idea what's going on here?

Comment 2

[Dragana Damjanovic [:dragana]]

Looking at this. Thanks.

Comment 3

[Dragana Damjanovic [:dragana]]

This is not a necko bug. Necko does not do early-data because the alpn is not negotiated. But I see in ssl debug log that keys for early data are derived. 
Is there an error in tls13_MaybeDo0RTTHandshake? maybe I am wrong. in that function ss->sec.ci.sid->u.ssl3.alpnSelection.len is 0 as expected since alpn is not present, but still rv = tls13_DeriveSecret(ss, ss->ssl3.hs.currentSecret,
                            kHkdfLabelClient,
                            kHkdfLabelEarlyTrafficSecret,
                            NULL,
                            &ss->ssl3.hs.clientEarlyTrafficSecret);
is called.

Maybe I am wrong.

Anyway necko is not calling PR_Write only DriveHandshake.

Comment 4

[Eric Rescorla (:ekr)]

The conditions for doing early data in TLS are found in:
  https://tlswg.github.io/tls13-spec/#early-data-indication
and state:
  The selected ALPN [RFC7301] protocol, if any.
Note also that these are server-side conditions. The client is required to match ALPN, which in this case means no ALPN.

On the Necko side, Firefox should be willing to do early data even when the server does not negotiate ALPN, because that is the same semantic as negotiating h1. Is there a reason why you are not allowing this case? Is there some limitation in the interface that is requiring it?

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Eric Rescorla (:ekr) from comment #4)
> The conditions for doing early data in TLS are found in:
>   https://tlswg.github.io/tls13-spec/#early-data-indication
> and state:
>   The selected ALPN [RFC7301] protocol, if any.
> Note also that these are server-side conditions. The client is required to
> match ALPN, which in this case means no ALPN.
> 
> On the Necko side, Firefox should be willing to do early data even when the
> server does not negotiate ALPN, because that is the same semantic as
> negotiating h1. Is there a reason why you are not allowing this case? Is
> there some limitation in the interface that is requiring it?

This has changed? I do not remember why, I think when I implemented it, nss was setting alpnErlySelection (it was call something like that) to not available or something if alpn was not present and it was not doing early data without alpn. It was sometime ago. Anyway it is an easy fix. Maybe need a chance in psm, but that is easy as well.

Thanks.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1320252.patch

Comment 7

[Patrick McManus [:mcmanus]]

thanks for the endpoint to increase interop. huge help.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8814512 [details] [diff] [review]
bug_1320252.patch

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

r- for now since I can't locate one of the functions this patch calls. Also, how difficult would it be to add a testcase for this?

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +321,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>    }
> +
> +  PRBool zeroRttActive;
> +  SECStatus rv = SSL_GetZeroRttActive(mFd, &zeroRttActive);

I'm not finding this function anywhere.

@@ +323,5 @@
> +
> +  PRBool zeroRttActive;
> +  SECStatus rv = SSL_GetZeroRttActive(mFd, &zeroRttActive);
> +  if (rv != SECSuccess || !zeroRttActive) {
> +      return NS_ERROR_NOT_AVAILABLE;

nit: just two spaces of indentation to be consistent here

@@ +337,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  if (alpnState == SSL_NEXT_PROTO_EARLY_VALUE) {
> +      aAlpnSelected.Assign(BitwiseCast<char*, unsigned char*>(chosenAlpn),

nit: just two spaces of indentation here as well

Comment 9

[Dragana Damjanovic [:dragana]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> Comment on attachment 8814512 [details] [diff] [review]
> bug_1320252.patch
> 
> Review of attachment 8814512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for now since I can't locate one of the functions this patch calls. Also,
> how difficult would it be to add a testcase for this?
> 

SSL_GetZeroRttActive should be added in bug 1320398, anyway maybe bug 1320398 will go in a bit different direction so the function name will change. I will update this patch when other bug is resolved.

I am not familiar with PSM tests, but i will take a look.

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_1320252_v1.patch

Comment 11

[Dragana Damjanovic [:dragana]]

Comment on attachment 8820333 [details] [diff] [review]
bug_1320252_v1.patch

This patch depends on nss 3.29

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8820333 [details] [diff] [review]
bug_1320252_v1.patch

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

LGTM. Still would be nice to have a test (or modify existing tests to cover this case?)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d328849f97c2
Send early-data even without alpn. r=keeler

Comment 14

[Iris Hsiao [:ihsiao]]

sorry had to back out for bustage, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=66565299&repo=mozilla-inbound&lineNumber=22971

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e614c01b60db
Backed out changeset d328849f97c2 for bustage

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: bug_1320252_v1.patch

Forgot to add SSL_GetPreliminaryChannelInfo to nss.symbols

Comment 17

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2dba317813ba1669c07e0f1b31b26c149b27c23

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5460aadc5761
Send early-data even without alpn. r=keeler

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5460aadc5761