kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Drop retransmits in DTLS tests.

Browse Source 
BoringSSL currently retransmits non-deterministically on an internal timer
(rather than one supplied externally), so the tests currently fail flakily
depending on timing. Valgrind is a common source for this. We still assume an
in-order and reliable channel, but drop retransmits silently:

- Handshake messages may arrive with old sequence numbers.

- Retransmitted CCS records arrive from the previous epoch.

- We may receive a retransmitted Finished after we believe the handshake has
  completed. (Aside: even in a real implementation, only Finished is possible
  here. Even with out-of-order delivery, retransmitted or reordered messages
  earlier in the handshake come in under a different epoch.)

Note that because DTLS renego and a Finished retransmit are ambiguous at the
record layer[*], this precludes us writing tests for DTLS renego. But DTLS
renego should get removed anyway. As BoringSSL currently implements renego,
this ambiguity is also a source of complexity in the real implementation. (See
the SSL3_MT_FINISHED check in dtls1_read_bytes.)

[*] As a further fun aside, it's also complex if dispatching renego vs Finished
after handshake message reassembly. The spec doesn't directly say the sequence
number is reset across renegos, but it says "The first message each side
transmits in /each/ handshake always has message_seq = 0". This means that such
an implementation needs the handshake message reassembly logic be aware that a
Finished fragment with high sequence number is NOT an out-of-order fragment for
the next handshake.

Change-Id: I35d13560f82bcb5eeda62f4de1571d28c818cc36
Reviewed-on: https://boringssl-review.googlesource.com/2770
Reviewed-by: Adam Langley <agl@google.com>
This commit is contained in:
David Benjamin 2015-01-08 13:44:37 -05:00 committed by Adam Langley
parent f3a8b12ac3
commit c67a3ae6ba
2 changed files with 28 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
ssl/test/runner/conn.go
View File

@ -756,8 +756,11 @@ Again:
if typ != want {
// A client might need to process a HelloRequest from
// the server, thus receiving a handshake message when
// application data is expected is ok.
if !c.isClient {
// application data is expected is ok. Moreover, a DTLS
// peer who sends Finished second may retransmit the
// final leg. BoringSSL retrainsmits on an internal
// timer, so this may also occur in test code.
if !c.isClient && !c.isDTLS {
return c.in.setErrorLocked(c.sendAlert(alertNoRenegotiation))
}
}
@ -1093,9 +1096,9 @@ func (c *Conn) Read(b []byte) (n int, err error) {
// Soft error, like EAGAIN
return 0, err
}
if c.hand.Len() > 0 {
if c.hand.Len() > 0 && !c.isDTLS {
// We received handshake bytes, indicating the
// start of a renegotiation.
// start of a renegotiation or a DTLS retransmit.
if err := c.handleRenegotiation(); err != nil {
return 0, err
}

27
ssl/test/runner/dtls.go
View File

@ -38,6 +38,7 @@ func wireToVersion(vers uint16, isDTLS bool) uint16 {
}
func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, *block, error) {
Again:
recordHeaderLen := dtlsRecordHeaderLen
if c.rawInput == nil {
@ -81,6 +82,13 @@ func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, *block, error) {
}
}
seq := b.data[3:11]
if !bytes.Equal(seq[:2], c.in.seq[:2]) {
// If the epoch didn't match, silently drop the record.
// BoringSSL retransmits on an internal timer, so it may flakily
// revisit the previous epoch if retransmiting ChangeCipherSpec
// and Finished.
goto Again
}
// For test purposes, we assume a reliable channel. Require
// that the explicit sequence number matches the incrementing
// one we maintain. A real implementation would maintain a
@ -242,9 +250,9 @@ func (c *Conn) dtlsWriteRecord(typ recordType, data []byte) (n int, err error) {
func (c *Conn) dtlsDoReadHandshake() ([]byte, error) {
// Assemble a full handshake message. For test purposes, this
// implementation assumes fragments arrive in order. It may
// need to be cleverer if we ever test BoringSSL's retransmit
// behavior.
// implementation assumes fragments arrive in order, but tolerates
// retransmits. It may need to be cleverer if we ever test BoringSSL's
// retransmit behavior.
for len(c.handMsg) < 4+c.handMsgLen {
// Get a new handshake record if the previous has been
// exhausted.
@ -273,9 +281,16 @@ func (c *Conn) dtlsDoReadHandshake() ([]byte, error) {
}
fragment := c.hand.Next(fragLen)
// Check it's a fragment for the right message.
if fragSeq != c.recvHandshakeSeq {
return nil, errors.New("dtls: bad handshake sequence number")
if fragSeq < c.recvHandshakeSeq {
// BoringSSL retransmits based on an internal timer, so
// it may flakily retransmit part of a handshake
// message. Ignore those fragments.
//
// TODO(davidben): Revise this if BoringSSL's retransmit
// logic is made more deterministic.
continue
} else if fragSeq > c.recvHandshakeSeq {
return nil, errors.New("dtls: handshake messages sent out of order")
}
// Check that the length is consistent.