From 4417d055e291246129420f3df6197310c5c6d7a0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 5 Apr 2015 04:17:25 -0400 Subject: [PATCH] Remove buffered_app_data as well. This conceivably has a use, but NSS doesn't do this buffer either and it still suffers from the same problems as the other uses of record_pqueue. This removes the last use of record_pqueue. It also opens the door to removing pqueue altogether as it isn't the right data structure for either of the remaining uses either. (It's not clear it was right for record_pqueue either, but I don't feel like digging into this code.) Change-Id: If8a43e7332b3cd11a78a516f3e8ebf828052316f Reviewed-on: https://boringssl-review.googlesource.com/4239 Reviewed-by: Adam Langley --- include/openssl/dtls1.h | 16 ------ ssl/d1_lib.c | 18 +------ ssl/d1_pkt.c | 103 +------------------------------------- ssl/test/runner/runner.go | 23 ++++----- 4 files changed, 12 insertions(+), 148 deletions(-) diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h index 5d60519d..b2ea94cb 100644 --- a/include/openssl/dtls1.h +++ b/include/openssl/dtls1.h @@ -105,11 +105,6 @@ struct hm_header_st { uint16_t epoch; }; -typedef struct record_pqueue_st { - uint16_t epoch; - pqueue q; -} record_pqueue; - /* TODO(davidben): This structure is used for both incoming messages and * outgoing messages. |fragment| and |reassembly| are only used in the former * and should be moved elsewhere. */ @@ -157,10 +152,6 @@ typedef struct dtls1_state_st { * TODO(davidben): This data structure may as well be a STACK_OF(T). */ pqueue sent_messages; - /* Buffered application records. Only for records between CCS and Finished to - * prevent either protocol violation or unnecessary message loss. */ - record_pqueue buffered_app_data; - unsigned int mtu; /* max DTLS packet size */ struct hm_header_st w_msg_hdr; @@ -180,13 +171,6 @@ typedef struct dtls1_state_st { unsigned int change_cipher_spec_ok; } DTLS1_STATE; -typedef struct dtls1_record_data_st { - uint8_t *packet; - unsigned int packet_length; - SSL3_BUFFER rbuf; - SSL3_RECORD rrec; -} DTLS1_RECORD_DATA; - #ifdef __cplusplus } /* extern C */ diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 61a862df..fc59e333 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -98,19 +98,14 @@ int dtls1_new(SSL *s) { d1->buffered_messages = pqueue_new(); d1->sent_messages = pqueue_new(); - d1->buffered_app_data.q = pqueue_new(); - if (!d1->buffered_messages || !d1->sent_messages || - !d1->buffered_app_data.q) { + if (!d1->buffered_messages || !d1->sent_messages) { if (d1->buffered_messages) { pqueue_free(d1->buffered_messages); } if (d1->sent_messages) { pqueue_free(d1->sent_messages); } - if (d1->buffered_app_data.q) { - pqueue_free(d1->buffered_app_data.q); - } OPENSSL_free(d1); ssl3_free(s); return 0; @@ -130,7 +125,6 @@ int dtls1_new(SSL *s) { static void dtls1_clear_queues(SSL *s) { pitem *item = NULL; hm_fragment *frag = NULL; - DTLS1_RECORD_DATA *rdata; while ((item = pqueue_pop(s->d1->buffered_messages)) != NULL) { frag = (hm_fragment *)item->data; @@ -143,15 +137,6 @@ static void dtls1_clear_queues(SSL *s) { dtls1_hm_fragment_free(frag); pitem_free(item); } - - while ((item = pqueue_pop(s->d1->buffered_app_data.q)) != NULL) { - rdata = (DTLS1_RECORD_DATA *)item->data; - if (rdata->rbuf.buf) { - OPENSSL_free(rdata->rbuf.buf); - } - OPENSSL_free(item->data); - pitem_free(item); - } } void dtls1_free(SSL *s) { @@ -165,7 +150,6 @@ void dtls1_free(SSL *s) { pqueue_free(s->d1->buffered_messages); pqueue_free(s->d1->sent_messages); - pqueue_free(s->d1->buffered_app_data.q); OPENSSL_free(s->d1); s->d1 = NULL; diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index 56bdae00..7b2cab15 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c @@ -183,90 +183,10 @@ static int satsub64be(const uint8_t *v1, const uint8_t *v2) { static int dtls1_record_replay_check(SSL *s, DTLS1_BITMAP *bitmap); static void dtls1_record_bitmap_update(SSL *s, DTLS1_BITMAP *bitmap); -static int dtls1_buffer_record(SSL *s, record_pqueue *q, - uint8_t *priority); static int dtls1_process_record(SSL *s); static int do_dtls1_write(SSL *s, int type, const uint8_t *buf, unsigned int len); -/* copy buffered record into SSL structure */ -static int dtls1_copy_record(SSL *s, pitem *item) { - DTLS1_RECORD_DATA *rdata; - - rdata = (DTLS1_RECORD_DATA *)item->data; - - if (s->s3->rbuf.buf != NULL) { - OPENSSL_free(s->s3->rbuf.buf); - } - - s->packet = rdata->packet; - s->packet_length = rdata->packet_length; - memcpy(&(s->s3->rbuf), &(rdata->rbuf), sizeof(SSL3_BUFFER)); - memcpy(&(s->s3->rrec), &(rdata->rrec), sizeof(SSL3_RECORD)); - - /* Set proper sequence number for mac calculation */ - memcpy(&(s->s3->read_sequence[2]), &(rdata->packet[5]), 6); - - return 1; -} - -static int dtls1_buffer_record(SSL *s, record_pqueue *queue, - uint8_t *priority) { - DTLS1_RECORD_DATA *rdata; - pitem *item; - - /* Limit the size of the queue to prevent DOS attacks */ - if (pqueue_size(queue->q) >= 100) { - return 0; - } - - rdata = OPENSSL_malloc(sizeof(DTLS1_RECORD_DATA)); - item = pitem_new(priority, rdata); - if (rdata == NULL || item == NULL) { - if (rdata != NULL) { - OPENSSL_free(rdata); - } - if (item != NULL) { - pitem_free(item); - } - - OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR); - return -1; - } - - rdata->packet = s->packet; - rdata->packet_length = s->packet_length; - memcpy(&(rdata->rbuf), &(s->s3->rbuf), sizeof(SSL3_BUFFER)); - memcpy(&(rdata->rrec), &(s->s3->rrec), sizeof(SSL3_RECORD)); - - item->data = rdata; - - s->packet = NULL; - s->packet_length = 0; - memset(&(s->s3->rbuf), 0, sizeof(SSL3_BUFFER)); - memset(&(s->s3->rrec), 0, sizeof(SSL3_RECORD)); - - if (!ssl3_setup_buffers(s)) { - goto internal_error; - } - - /* insert should not fail, since duplicates are dropped */ - if (pqueue_insert(queue->q, item) == NULL) { - goto internal_error; - } - - return 1; - -internal_error: - OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR); - if (rdata->rbuf.buf != NULL) { - OPENSSL_free(rdata->rbuf.buf); - } - OPENSSL_free(rdata); - pitem_free(item); - return -1; -} - static int dtls1_process_record(SSL *s) { int al; SSL3_RECORD *rr; @@ -540,21 +460,6 @@ start: * s->s3->rrec.length - number of bytes. */ rr = &s->s3->rrec; - /* We are not handshaking and have no data yet, - * so process data buffered during the last handshake - * in advance, if any. - */ - if (s->state == SSL_ST_OK && rr->length == 0) { - pitem *item; - item = pqueue_pop(s->d1->buffered_app_data.q); - if (item) { - dtls1_copy_record(s, item); - - OPENSSL_free(item->data); - pitem_free(item); - } - } - /* Check for timeout */ if (dtls1_handle_timeout(s) > 0) { goto start; @@ -581,12 +486,8 @@ start: if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE && rr->type != SSL3_RT_ALERT) { /* We now have an unexpected record between CCS and Finished. Most likely - * the packets were reordered on their way, so buffer the application data - * for later processing rather than dropping the connection. */ - if (dtls1_buffer_record(s, &(s->d1->buffered_app_data), rr->seq_num) < 0) { - OPENSSL_PUT_ERROR(SSL, dtls1_read_bytes, ERR_R_INTERNAL_ERROR); - return -1; - } + * the packets were reordered on their way. DTLS is unreliable, so drop the + * packet and expect the peer to retransmit. */ rr->length = 0; goto start; } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 084ae463..6e80f941 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -695,6 +695,8 @@ var testCases = []testCase{ AppDataAfterChangeCipherSpec: []byte("TEST MESSAGE"), }, }, + // BoringSSL's DTLS implementation will drop the out-of-order + // application data. }, { name: "AlertAfterChangeCipherSpec", @@ -1174,21 +1176,14 @@ func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, i return err } - var testMessage []byte - if config.Bugs.AppDataAfterChangeCipherSpec != nil { - // We've already sent a message. Expect the shim to echo it - // back. - testMessage = config.Bugs.AppDataAfterChangeCipherSpec - } else { - if messageLen == 0 { - messageLen = 32 - } - testMessage = make([]byte, messageLen) - for i := range testMessage { - testMessage[i] = 0x42 - } - tlsConn.Write(testMessage) + if messageLen == 0 { + messageLen = 32 } + testMessage := make([]byte, messageLen) + for i := range testMessage { + testMessage[i] = 0x42 + } + tlsConn.Write(testMessage) buf := make([]byte, len(testMessage)) if test.protocol == dtls {