From b1e5feadefddd366be6614af79fac3276a73d0f1 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Thu, 21 Sep 2017 11:34:37 +0100 Subject: [PATCH] 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. --- 13.go | 37 ++++++++++++++++++++++++++++++++----- conn.go | 29 ++++++++++++++++++++++------- handshake_server.go | 10 ++++++++++ 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/13.go b/13.go index 55f7972..fb920fa 100644 --- a/13.go +++ b/13.go @@ -244,12 +244,33 @@ CurvePreferenceLoop: return nil } -// readClientFinished13 is called when, on the second flight of the client, -// a handshake message is received. This might be immediately or after the -// early data. Once done it sends the session tickets. Under c.in lock. -func (hs *serverHandshakeState) readClientFinished13() error { +// readClientFinished13 is called during the server handshake (when no early +// data it available) or after reading all early data. It discards early data if +// the server did not accept it and then verifies the Finished message. Once +// done it sends the session tickets. Under c.in lock. +func (hs *serverHandshakeState) readClientFinished13(hasConfirmLock bool) error { c := hs.c + // If the client advertised and sends early data while the server does + // not accept it, it must be fully skipped until the Finished message. + for c.phase == discardingEarlyData { + if err := c.readRecord(recordTypeApplicationData); err != nil { + return err + } + // Assume receipt of Finished message (will be checked below). + if c.hand.Len() > 0 { + c.phase = waitingClientFinished + break + } + } + + // If the client sends early data followed by a Finished message (but + // no end_of_early_data), the server MUST terminate the connection. + if c.phase != waitingClientFinished { + c.sendAlert(alertUnexpectedMessage) + return errors.New("tls: did not expect Client Finished yet") + } + c.phase = readingClientFinished msg, err := c.readHandshake() if err != nil { @@ -283,7 +304,13 @@ 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 a Handshake is pending, c.confirmMutex will never be locked as + // ConfirmHandshake will wait for the handshake to complete. If a + // handshake was complete, and this was a confirmation, unlock + // c.confirmMutex now to allow readers to proceed. + if hasConfirmLock { + c.confirmMutex.Unlock() + } return hs.sendSessionTicket13() // TODO: do in a goroutine } diff --git a/conn.go b/conn.go index 665cbda..d7ae0ab 100644 --- a/conn.go +++ b/conn.go @@ -844,12 +844,6 @@ Again: return c.in.setErrorLocked(c.sendAlert(alertUnexpectedMessage)) } c.hand.Write(data) - if typ != want && c.phase == waitingClientFinished { - if err := c.hs.readClientFinished13(); err != nil { - c.in.setErrorLocked(err) - break - } - } } if b != nil { @@ -1315,6 +1309,10 @@ func (c *Conn) handleRenegotiation(*helloRequestMsg) error { // client sent valid 0-RTT data. In any other case it's equivalent to // calling Handshake. func (c *Conn) ConfirmHandshake() error { + if c.isClient { + panic("ConfirmHandshake should only be called for servers") + } + if err := c.Handshake(); err != nil { return err } @@ -1341,6 +1339,9 @@ func (c *Conn) ConfirmHandshake() error { defer c.in.Unlock() var input *block + // Try to read all data (if phase==readingEarlyData) or extract the + // remaining data from the previous read that could not fit in the read + // buffer (if c.input != nil). if c.phase == readingEarlyData || c.input != nil { buf := &bytes.Buffer{} if _, err := buf.ReadFrom(earlyDataReader{c}); err != nil { @@ -1350,8 +1351,12 @@ func (c *Conn) ConfirmHandshake() error { input = &block{data: buf.Bytes()} } + // 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 + // not confirmed implies c.phase == discardingEarlyData || c.phase == waitingClientFinished for c.phase != handshakeConfirmed { - if err := c.readRecord(recordTypeApplicationData); err != nil { + if err := c.hs.readClientFinished13(true); err != nil { c.in.setErrorLocked(err) return err } @@ -1402,6 +1407,7 @@ func (r earlyDataReader) Read(b []byte) (n int, err error) { } } + // Following early application data, an end_of_early_data is expected. if err == nil && c.phase != readingEarlyData && c.input == nil { err = io.EOF } @@ -1446,6 +1452,15 @@ func (c *Conn) Read(b []byte) (n int, err error) { return 0, err } if c.hand.Len() > 0 { + if c.phase == waitingClientFinished { + // Server has received all early data, confirm + // by reading the Client Finished message. + if err := c.hs.readClientFinished13(true); err != nil { + c.in.setErrorLocked(err) + return 0, err + } + continue + } if err := c.handlePostHandshake(); err != nil { return 0, err } diff --git a/handshake_server.go b/handshake_server.go index aa4cc4e..f294a80 100644 --- a/handshake_server.go +++ b/handshake_server.go @@ -77,6 +77,16 @@ func (c *Conn) serverHandshake() error { return err } c.hs = &hs + // If the client is sending early data while the server expects + // 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.5-RTT data to a potential unauthenticated client). + if c.phase != readingEarlyData { + if err := hs.readClientFinished13(false); err != nil { + return err + } + } c.handshakeComplete = true return nil } else if isResume {