tris: prevent sending 0.5RTT data #34

Lekensteyn · 2017-09-21T12:04:14Z

Three patches, two of them are needed to interop as client with servers that send session tickets (#50) and are included because of contextual conflicts. The final patch is:

tris: prevent sending 0.5-RTT data

Disable 0.5-RTT as it has weaker security properties than 1-RTT. The
same security considerations from TLS False Start (RFC 7918) apply.

Currently the server Handshake method returns as soon as it has sent its
parameters, but it does not wait for the client to authenticate the
handshake via a Finished message. This broke a test that assumed that
the Handshake message performs a full handshake and also
(unintentionally?) enabled the server to send application data before
the handshake is complete ("0.5-RTT data").

Fix this by moving the implicit Finished message check in the handshake
message reader to the server handshake itself (previously readRecord
would process the Finished message as a side-effect of requesting
recordTypeApplicationData). And in the special case where 0-RTT data is
actually desired, process the Finished message in the ConfirmHandshake
and Read functions.

NOTE: 0.5-RTT is not disabled when the server enables 0-RTT. It is the
server responsibility to use ConfirmHandshake before writing anything.

Explicitly panic when ConfirmHandshake is used for client connections,
this is not the intended use of that API.

FiloSottile · 2017-09-21T12:11:23Z

This was very intentional. There might be protocols that will want to use 0.5RTT data, and it's not as vulnerable to replay because there is no data to replay. Any reason to disable this protocol feature beyond fixing a test? Sent from a small keyboard

…

On 21 Sep 2017, at 14:04, Peter Wu ***@***.***> wrote: Currently the server Handshake method returns as soon as it has sent its parameters, but it does not wait for the client to authenticate the handshake via a Finished message. This broke a test that assumed that the Handshake message performs a full handshake and also (unintentionally?) enabled the server to send application data before the handshake is complete ("0.5RTT data"). Fix this by moving the implicit Finished message check in the handshake message reader to the server handshake itself (previously readRecord would process the Finished message as a side-effect of requesting recordTypeApplicationData). And in the special case where 0RTT data is actually desired, process the Finished message in the ConfirmHandshake and Read functions. NOTE: 0.5RTT is not disabled yet when the server enables 0RTT. It is the server responsibility to use ConfirmHandshake before writing anything. Explicitly panic when trying ConfirmHandshake for client connections, this is not the intended use of that API. You can view, comment on, or merge this pull request online at: #34 Commit Summary tris: prevent sending 0.5RTT data File Changes M 13.go (33) M conn.go (34) M handshake_server.go (10) Patch Links: https://github.com/cloudflare/tls-tris/pull/34.patch https://github.com/cloudflare/tls-tris/pull/34.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Lekensteyn · 2017-09-21T14:01:34Z

While it is technically allowed by the spec, I wanted to be sure that the Handshake method follows the state machine, eventually ending up in the CONNECTED state (and authenticating the client by default).

If an application needs to send 0.5RTT data, I think it should opt-in (via an option that can be added in the future).

FiloSottile · 2017-09-21T14:07:56Z

Define "authenticating the client", it's entirely possible I'm missing an attack scenario. Do you mean simply making sure it's not a replayed connection (with no data)?

Lekensteyn · 2017-09-21T14:13:47Z

One concern is related to denial of service, an attacker can flood Client Hello and if the server believes that the client handshake has been authenticated, it might unnecessarily commit resources.

Another concern is about changing the documented, expected functionality. Handshake should run the handshake protocol and error out if things went wrong. If an attacker replays the CH, then it will only be caught during the next read.

lziest · 2017-09-22T22:51:31Z

can we have a reference of the test we want to fix?

lziest · 2017-09-22T22:55:20Z

13.go

@@ -189,7 +210,9 @@ func (hs *serverHandshakeState) readClientFinished13() error {
 	// Any read operation after handshakeRunning and before handshakeConfirmed
 	// will be holding this lock, which we release as soon as the confirmation
 	// happens, even if the Read call might do more work.
-	c.confirmMutex.Unlock()
+	if !hasHandshakeLock {


call me ignorant, can you explain the logic behind this extra check? I feel this is fixing a bug. But unlock only if we have locked it, right?

The Read and ConfirmHandshake methods can run in parallel, that's why this lock was originally needed. If ConfirmHandshake consumes data, Read must not interfere with it.

Since the Handshake method will always be executed before the main Read and ConfirmHandshake functionality gets executed (and thus before taking the confirmMutex lock), it is safe not to acquire to lock in case readClientFinished13 is executed via the handshake (hence the new hasHandshakeLock name).

I could have added a c.confirmMutex.Lock() call in the handshake function, but it would not really be required (and thus I did not do that to avoid the false impression that it is needed for something).

I will read into it, in general check then unlock always sounds racing (pun intended)

Lekensteyn · 2017-09-22T23:24:42Z

can we have a reference of the test we want to fix?

See c03e274 ("tris: add test for TLS 1.3 certificate validation"). The test fails because s.Close() closes the server socket before the client can write its Finished message.

Lekensteyn · 2017-09-29T13:17:05Z

Since 0.5 RTT data has known caveats, I took the conservative approach and disabled it for non-0RTT.

Viewpoints differ, here is one from Antoine who suggests that that 0.5 RTT is used with 0-RTT in QUIC: https://www.ietf.org/mail-archive/web/tls/current/msg19337.html (this scenario will still work even after this patch).

"Any expectations of 0.5 data being directed to a specific client need to be eliminated." -- Hugo
https://www.ietf.org/mail-archive/web/tls/current/msg19317.html

Discussion about how 0.5 is strictly weaker than 1-RTT and how this could cause attacks if a client advertises weak ciphers.
https://www.ietf.org/mail-archive/web/tls/current/msg19299.html

More messages can be found here: https://mailarchive.ietf.org/arch/search/?q=%220.5+RTT%22&f_list=tls

@FiloSottile do you agree with this conservative approach, disabling 0.5RTT data unless 0RTT data is also enabled?

Lekensteyn · 2017-10-11T11:14:01Z

This is essentially the same problem as TLS False Start (https://tools.ietf.org/html/rfc7918), but then in the opposite direction. Its security considerations also apply.

Lekensteyn · 2017-11-16T19:36:47Z

New version pushed with comments added. I will probably change this when implementing draft 22 (which has contextual conflicts), so maybe review later.

Lekensteyn · 2017-11-27T19:29:54Z

Until this is decided, I will back out the test from commit 0f32ff2 from the client branch to allow client tests to pass.

grittygrease · 2017-11-28T01:29:50Z

I'm in favor of this change. When it comes to sending data in an unauthenticated context, the conservative approach should win out unless there are extraordinary gains.

grittygrease

I'm happy with this change. It will have to be updated for keyUpdate and other post-handshake messages, but this nicely sets the stage.

grittygrease · 2017-12-01T00:43:51Z

conn.go

+	// At this point, earlyDataReader has read all early data and received
+	// the end_of_early_data signal. Expect a Finished message.
+	// Locks held so far: c.confirmMutex, c.in
+	for c.phase != handshakeConfirmed { // c.phase == discardingEarlyData || c.phase == waitingClientFinished


This comment is helpful, but looks odd after the {

Fixed this and the other comment, thanks for reviewing

grittygrease · 2017-12-01T00:44:52Z

handshake_server.go

+		// it, delay the Finished check until HandshakeConfirmed() is
+		// called or until all early data is Read(). Otherwise, complete
+		// authenticating the client now (there is no support for
+		// sending 0.50RTT data to a potential unauthenticated client).


0.5 RTT instead of 0.50RTT?

Lekensteyn · 2017-12-01T15:27:16Z

Some clarifications on the last patch:

0.5-RTT data is still possible if the server enables 0-RTT. Writing will not wait for the Client Finished if 0-RTT is enabled.
Server MUST invoke ConfirmHandshake if it wants to have replay protection (e.g. if a HTTP method is found not to be idempotent).
As soon as the server reads more 0-RTT data than is actually available (i.e. when it does two Read calls), it WILL will wait for Client Finished. (This behavior was present before this patch.)

I also happened to came across this boringssl documentation: https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#Early-data
They describe that 0.5-RTT ("half-RTT") is enabled with 0-RTT which seems to match with what this PR is trying to achieve.

Do not assume that every post-handshake messages are HelloRequests that try to trigger a renegotiation. This could result in a no_renegotiation (rather than an unexpected_message) alert even for other message types (like NewSessionTicket). This change makes the caller of readRecord(recordTypeApplicationData) responsible for checking for renegotiation in case of a handshake message, but that is currently already the case. And the condition "c.phase == waitingClientFinished" can only be hit by the server, so that won't have break the handshake either. Related: #50

Prevent unexpected_message alert, PSK resumption is not supported so ignore the received session ticket as client.

Disable 0.5-RTT as it has weaker security properties than 1-RTT. The same security considerations from TLS False Start (RFC 7918) apply. Currently the server Handshake method returns as soon as it has sent its parameters, but it does not wait for the client to authenticate the handshake via a Finished message. This broke a test that assumed that the Handshake message performs a full handshake and also (unintentionally?) enabled the server to send application data before the handshake is complete ("0.5-RTT data"). Fix this by moving the implicit Finished message check in the handshake message reader to the server handshake itself (previously readRecord would process the Finished message as a side-effect of requesting recordTypeApplicationData). And in the special case where 0-RTT data is actually desired, process the Finished message in the ConfirmHandshake and Read functions. NOTE: 0.5-RTT is not disabled when the server enables 0-RTT. It is the server responsibility to use ConfirmHandshake before writing anything. Explicitly panic when ConfirmHandshake is used for client connections, this is not the intended use of that API.

Lekensteyn · 2017-12-01T19:23:24Z

(just rebased the patches on latest master, no further changes)

Lekensteyn mentioned this pull request Sep 21, 2017

Initial TLS 1.3 client support (v2) #43

Closed

6 tasks

lziest reviewed Sep 22, 2017

View reviewed changes

Lekensteyn force-pushed the pwu/finished-rework branch from 015cd9b to b4ca3a8 Compare September 29, 2017 14:20

Lekensteyn force-pushed the pwu/finished-rework branch from b4ca3a8 to 6c01334 Compare November 16, 2017 19:35

Lekensteyn added the needs-work label Nov 23, 2017

Lekensteyn force-pushed the pwu/finished-rework branch from 6c01334 to 28fc7e7 Compare November 24, 2017 15:35

Lekensteyn removed the needs-work label Nov 24, 2017

Lekensteyn mentioned this pull request Nov 27, 2017

Initial TLS 1.3 client support (v2.1) #55

Merged

2 tasks

grittygrease reviewed Dec 1, 2017

View reviewed changes

Lekensteyn force-pushed the pwu/finished-rework branch from 28fc7e7 to 44c8e56 Compare December 1, 2017 01:41

Lekensteyn and others added 3 commits December 1, 2017 19:08

tris: ignore NewSessionTicket messages as client

8523d7d

Prevent unexpected_message alert, PSK resumption is not supported so ignore the received session ticket as client.

Lekensteyn force-pushed the pwu/finished-rework branch from 44c8e56 to b1e5fea Compare December 1, 2017 19:22

Lekensteyn mentioned this pull request Dec 4, 2017

Draft 22 changes for the server and client #57

Merged

1 task

grittygrease approved these changes Dec 4, 2017

View reviewed changes

Lekensteyn merged commit b1e5fea into master Dec 4, 2017

Lekensteyn deleted the pwu/finished-rework branch December 4, 2017 19:18

tris: prevent sending 0.5RTT data #34

tris: prevent sending 0.5RTT data #34

Uh oh!

Conversation

Lekensteyn commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FiloSottile commented Sep 21, 2017 via email

Uh oh!

Lekensteyn commented Sep 21, 2017

Uh oh!

FiloSottile commented Sep 21, 2017 via email

Uh oh!

Lekensteyn commented Sep 21, 2017

Uh oh!

lziest commented Sep 22, 2017

Uh oh!

lziest Sep 22, 2017

Choose a reason for hiding this comment

Uh oh!

Lekensteyn Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lziest Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

Lekensteyn commented Sep 22, 2017

Uh oh!

Lekensteyn commented Sep 29, 2017

Uh oh!

Lekensteyn commented Oct 11, 2017

Uh oh!

Lekensteyn commented Nov 16, 2017

Uh oh!

Lekensteyn commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grittygrease commented Nov 28, 2017

Uh oh!

grittygrease left a comment

Choose a reason for hiding this comment

Uh oh!

grittygrease Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

Lekensteyn Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

grittygrease Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

Lekensteyn commented Dec 1, 2017

Uh oh!

Lekensteyn commented Dec 1, 2017

Uh oh!

Uh oh!

Lekensteyn commented Sep 21, 2017 •

edited

Loading

Lekensteyn Sep 22, 2017 •

edited

Loading

Lekensteyn commented Nov 27, 2017 •

edited

Loading