Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The serverhello.random trick is ugly and might interact with ticket handling... #399

Closed
sftcd opened this issue Mar 10, 2021 · 6 comments · Fixed by #420
Closed

The serverhello.random trick is ugly and might interact with ticket handling... #399

sftcd opened this issue Mar 10, 2021 · 6 comments · Fixed by #420
Assignees

Comments

@sftcd
Copy link
Collaborator

@sftcd sftcd commented Mar 10, 2021

Encoding the "ECH worked" signal in the SH.random lower octets is, at best, ugly. It also arguably causes violation of internal API designs in at least the OpenSSL code. And I'm not sure if there might be some interaction between that and ticket handling (just from the look of some code I've yet to understand really.)

Do we really need such a hacky solution? Given the ECH sticks out in the CH, we could just define a non-encrypted extension in the SH to include the confirmation value maybe (though even that is pretty hacky).

@ekr
Copy link
Collaborator

@ekr ekr commented Mar 10, 2021

I have some sympathy for this, but the point of this signal design was to make it difficult to distinguish GREASE from non-GREASE ECH.

@davidben
Copy link
Collaborator

@davidben davidben commented Mar 10, 2021

Could you elaborate on the concern with ticket handling? I'm not sure I see how tickets would relate to this.

@sftcd
Copy link
Collaborator Author

@sftcd sftcd commented Mar 17, 2021

@davidben
Copy link
Collaborator

@davidben davidben commented Mar 30, 2021

Just to synchronize the discussion across list/GitHub a bit, this was discussed a bit in https://mailarchive.ietf.org/arch/msg/tls/pBjQDWT4ZvkATFKZ04rKZQ6Zvi4/. I agree with @sftcd now that there's some fiddly interactions from two ClientHellos and ServerHello.

I sketched out a possible change sketched out in https://mailarchive.ietf.org/arch/msg/tls/gpVj4FSmHQ2XQTKpQY07IDcD40I/. We could compute ClientHelloInner...ServerHelloECHConf as in the draft right now. But then rather than feed that into the handshake secret computation, just rely on that transcript containing a secret ClientHello.random and computing F(transcript). (Hash it down, KDF it with zero secret, whatever.)

One observation is that, while this relies oddly on a secret ClientHello.random, the existing construction does too. An attacker constructing a ServerHello gets to pick ServerHello.key_share and, with knowledge of ClientHello.key_share, compute the resulting shared secret.

@martinthomson
Copy link
Contributor

@martinthomson martinthomson commented Apr 1, 2021

One further observation here is that if the client uses the same shares for CHInner and CHOuter, the double key derivation piece doesn't really need to apply.

@cjpatton
Copy link
Contributor

@cjpatton cjpatton commented Apr 1, 2021

For my own reference, I just wanted to flag something that we've discussed on the list: the current acceptance signal interacts with PSK in a way that could easily cause a bug in an implementation that hasn't carefully vetted this interaction.

@chris-wood chris-wood self-assigned this Apr 13, 2021
chris-wood added a commit that referenced this issue Apr 14, 2021
We previously derived the acceptance signal from the handshake secret.
This meant that clients which used the wrong ECHConfig might need to
process ServerHello extensions twice before computing the sigal, which
can be problematic for some libraries. Given that the signal's secrecy
is entirely dependent on ClientHelloInner.random, we can relax the signal
computation and base it on the transcript alone, which includes
ClientHelloInner.random, rather than a secret derived from that transcript.

Closes #399.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants