Closed Bug 1320252 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kazuhooku, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

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.
:dragana, any idea what's going on here?
Flags: needinfo?(dd.mozilla)
Looking at this. Thanks.
Assignee: nobody → dd.mozilla
Flags: needinfo?(dd.mozilla)
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.
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?
(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.
Depends on: 1320398
Attached patch bug_1320252.patch (obsolete) — Splinter Review
Attachment #8814512 - Flags: review?(dkeeler)
Component: Untriaged → Networking
Product: Firefox → Core
Whiteboard: [necko-active]
thanks for the endpoint to increase interop. huge help.
Summary: early-data is empty when using TLS 1.3 over HTTP/1 (wo. ALPN) → early-data is empty when using TLS 1.3 over HTTP/1 (wo. ALPN) [h20/picotls]
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
Attachment #8814512 - Flags: review?(dkeeler) → review-
(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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug_1320252_v1.patch (obsolete) — Splinter Review
Attachment #8814512 - Attachment is obsolete: true
Comment on attachment 8820333 [details] [diff] [review]
bug_1320252_v1.patch

This patch depends on nss 3.29
Attachment #8820333 - Flags: review?(dkeeler)
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?)
Attachment #8820333 - Flags: review?(dkeeler) → review+
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d328849f97c2
Send early-data even without alpn. r=keeler
Keywords: checkin-needed
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e614c01b60db
Backed out changeset d328849f97c2 for bustage
Forgot to add SSL_GetPreliminaryChannelInfo to nss.symbols
Attachment #8820333 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8824519 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5460aadc5761
Send early-data even without alpn. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5460aadc5761
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1329469
You need to log in before you can comment on or make changes to this bug.