search

refraction-networking / utls

Fork of the Go standard TLS library, providing low-level access to the ClientHello for mimicry purposes.
BSD 3-Clause "New" or "Revised" License
1.71k stars 247 forks source link

Allow BuildHandshakeState to inspect ClientHello before setting SessionTicket/PSK #301

Closed adotkhan closed 4 months ago

adotkhan commented 5 months ago

The TLS-PSK changes introduced in https://github.com/refraction-networking/utls/commit/8094658e76e8f2b57ca84cf52e3a4c39645c467e sets the SessionTicket or PSK (either from cache or explicitly set) on the first call to (*UConn)BuildHandshakeState and locks the sessionController, preventing library user from inspecting the ClientHello to then craft an appropriate SessionTicket / PSK.

Our use case involves building a UConn with a potentially randomzied ClientHello and adding a SessionTicket to it encrypted with an out-of-band shared secret. To craft the appropriate SessionTicket (since the ClientHello parameters need to agree with it), the ClientHello is inspected for example to check whether EMS is present by calling BuildHandshakeState first.

Specifically, with regards to PSK / SessionTicket, the following doc comment on BuildHandshakeState does not apply to SetSessionTicketExtension/SetPskExtension, since the sessionController is locked after the first BuildHandshakeState call and further change to these extensions is disallowed:

// BuildHandshakeState is automatically called before uTLS performs handshake,
// amd should only be called explicitly to inspect/change fields of
// default/mimicked ClientHello.

This draft PR suggests one workaround, it's admittedly a fragile solution, however a review by uTLS maintainers is much appreicated towards finding an appropriate solution.

gaukas commented 5 months ago

Looks interesting, I am gonna give a pass on this PR later this week.

Thanks for contributing to uTLS, @adotkhan!

adotkhan commented 4 months ago

Thank you for the review @gaukas, much appreciated.

I have applied your suggestion regarding removing ExtMasterSecret bool field from MakeClientSessionState to avoid breaking the API.

cbf02ab408c2a1e622facce48124d1f7a95c95e3 reverts BuildHandshakeState behavior to what it was before, and adds a new BuildHandshakeStateWithoutSession which I believe would resolve the concerns you raised.

Since we only needed to inspect a partial/incomplete Client Hello, as more manual use case, I believe it makes sense to not change BuildHandshakeState. This also avoids potential issues with having to remember to call lockSessionState.

adotkhan commented 4 months ago

I missed what you mentioned regarding marshalling the Client Hello after the uLoadSession call. I believe f061fb7e341dbbbbe3bb7cf599d90131c480e4bf should resolve the issues raised better.

gaukas commented 4 months ago

Much obliged! Feel free to mark this pull request ready-for-review and re-request a review from me if you are done with it.

gaukas commented 4 months ago

Released in v1.6.7!