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

Caw tunnel version #2

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Caw tunnel version #2

wants to merge 27 commits into from

Conversation

ekr
Copy link
Owner

@ekr ekr commented Mar 7, 2020

No description provided.

ekr and others added 20 commits November 3, 2019 06:38
the client creates a ClientHelloInner value with the true SNI,
encrypts that, and puts it into an extension in ClientHelloOuter,
which is what is actually sent to the server.

There are a number of open issues here:

- This design protects against the HRR splicing attack by having
  the transcript include ClientHelloInner.nonce, which relies
  on transcript secrecy. An alternative design would be to
  explicitly include it in the key schedule. However, this
  is a bit odd in split mode.

- You have to use trial decryption on the client to determine whether
  the ClientHelloInner or ClientHelloOuter was accepted, and the client
  doesn't even know when it gets HRR whether it applies to the inner or
  outer CH. Should we consider having an extension indicate
  this? Obviously that leaks some information, however the primary thing
  that it leaks is whether the server knew the ESNI key, which can be
  independently determined by sending a new CH with the relevant config.

This whole dual transcript thing is definitely a weak spot that would
need some extensive analysis.
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
Comment on lines 585 to 588
actually be invalid for one or the other ClientHello. If the inner ClientHello
is unaffected by this retry, then the client only changes the outer ClientHello.
In contrast, if the outer ClientHello is unaffected by this retry, then the client
changes the inner ClientHello and recomputes any fields necessary in the outer
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an unwise optimization. What's the rationale here?

Also, won't it be impossible because you'll get a different echo_nonce?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By optimization do you mean it'd be simpler to just re-generate both? I think the intent was to stick with the HRR model and change as little as possible. But I'd be fine with regenerating both inner and outer CHs. That would definitely be simpler to implementer, IMO.


~~~
pkR = HPKE.KEM.Unmarshal(ECHOConfig.public_key)
enc, context = SetupPSKS(pkR, "tls13-echo-hrr", echo_hrr_key, "")
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comments about ctx rather than PSK. Maybe an open issue marker here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and double checked the PSK variant of HPKE, and info is assumed to be known to the attacker in our analysis. Here that's not the case, so I think PSK is the right place for it. But agreed, let's drop a TODO to follow up with Karthik et al.

draft-ietf-tls-esni.md Show resolved Hide resolved
draft-ietf-tls-esni.md Outdated Show resolved Hide resolved
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 this pull request may close these issues.

2 participants