[openssl-commits] [openssl] master update

Matt Caswell matt at openssl.org
Fri Oct 26 13:21:37 UTC 2018


The branch master has been updated
       via  f1358634af5b84be22cb20fff3dcb613f5f8c978 (commit)
       via  840facc3ccab481e1a0bc2cb6e7740e362df7422 (commit)
      from  21311777ad8b8c2f2fb8eb7a4f9618ceae1e043d (commit)


- Log -----------------------------------------------------------------
commit f1358634af5b84be22cb20fff3dcb613f5f8c978
Author: Matt Caswell <matt at openssl.org>
Date:   Tue Oct 16 17:03:39 2018 +0100

    Add a test for duplicated DTLS records
    
    Reviewed-by: Paul Dale <paul.dale at oracle.com>
    (Merged from https://github.com/openssl/openssl/pull/7414)

commit 840facc3ccab481e1a0bc2cb6e7740e362df7422
Author: Matt Caswell <matt at openssl.org>
Date:   Tue Oct 16 17:08:11 2018 +0100

    Properly handle duplicated messages from the next epoch
    
    Since 1fb9fdc30 we may attempt to buffer a record from the next epoch
    that has already been buffered. Prior to that this never occurred.
    
    We simply ignore a failure to buffer a duplicated record.
    
    Fixes #6902
    
    Reviewed-by: Paul Dale <paul.dale at oracle.com>
    (Merged from https://github.com/openssl/openssl/pull/7414)

-----------------------------------------------------------------------

Summary of changes:
 ssl/record/rec_layer_d1.c |  5 +--
 test/dtlstest.c           | 35 +++++++++++++++++++++
 test/ssltestlib.c         | 80 +++++++++++++++++++++++++++++++++--------------
 test/ssltestlib.h         |  7 +++--
 4 files changed, 97 insertions(+), 30 deletions(-)

diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c
index 43e1f98..1f9b319 100644
--- a/ssl/record/rec_layer_d1.c
+++ b/ssl/record/rec_layer_d1.c
@@ -185,14 +185,11 @@ int dtls1_buffer_record(SSL *s, record_pqueue *queue, unsigned char *priority)
         return -1;
     }
 
-    /* insert should not fail, since duplicates are dropped */
     if (pqueue_insert(queue->q, item) == NULL) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DTLS1_BUFFER_RECORD,
-                 ERR_R_INTERNAL_ERROR);
+        /* Must be a duplicate so ignore it */
         OPENSSL_free(rdata->rbuf.buf);
         OPENSSL_free(rdata);
         pitem_free(item);
-        return -1;
     }
 
     return 1;
diff --git a/test/dtlstest.c b/test/dtlstest.c
index 5afd7e9..772528f 100644
--- a/test/dtlstest.c
+++ b/test/dtlstest.c
@@ -289,6 +289,40 @@ static int test_cookie(void)
     return testresult;
 }
 
+static int test_dtls_duplicate_records(void)
+{
+    SSL_CTX *sctx = NULL, *cctx = NULL;
+    SSL *serverssl = NULL, *clientssl = NULL;
+    int testresult = 0;
+
+    if (!TEST_true(create_ssl_ctx_pair(DTLS_server_method(),
+                                       DTLS_client_method(),
+                                       DTLS1_VERSION, DTLS_MAX_VERSION,
+                                       &sctx, &cctx, cert, privkey)))
+        return 0;
+
+    if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl,
+                                      NULL, NULL)))
+        goto end;
+
+    DTLS_set_timer_cb(clientssl, timer_cb);
+    DTLS_set_timer_cb(serverssl, timer_cb);
+
+    BIO_ctrl(SSL_get_wbio(clientssl), MEMPACKET_CTRL_SET_DUPLICATE_REC, 1, NULL);
+    BIO_ctrl(SSL_get_wbio(serverssl), MEMPACKET_CTRL_SET_DUPLICATE_REC, 1, NULL);
+
+    if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE)))
+        goto end;
+
+    testresult = 1;
+ end:
+    SSL_free(serverssl);
+    SSL_free(clientssl);
+    SSL_CTX_free(sctx);
+    SSL_CTX_free(cctx);
+
+    return testresult;
+}
 
 int setup_tests(void)
 {
@@ -299,6 +333,7 @@ int setup_tests(void)
     ADD_ALL_TESTS(test_dtls_unprocessed, NUM_TESTS);
     ADD_ALL_TESTS(test_dtls_drop_records, TOTAL_RECORDS);
     ADD_TEST(test_cookie);
+    ADD_TEST(test_dtls_duplicate_records);
 
     return 1;
 }
diff --git a/test/ssltestlib.c b/test/ssltestlib.c
index 71b78584..eafac3c 100644
--- a/test/ssltestlib.c
+++ b/test/ssltestlib.c
@@ -284,6 +284,7 @@ typedef struct mempacket_test_ctx_st {
     unsigned int noinject;
     unsigned int dropepoch;
     int droprec;
+    int duprec;
 } MEMPACKET_TEST_CTX;
 
 static int mempacket_test_new(BIO *bi);
@@ -426,12 +427,25 @@ int mempacket_test_inject(BIO *bio, const char *in, int inl, int pktnum,
                           int type)
 {
     MEMPACKET_TEST_CTX *ctx = BIO_get_data(bio);
-    MEMPACKET *thispkt, *looppkt, *nextpkt;
-    int i;
+    MEMPACKET *thispkt = NULL, *looppkt, *nextpkt, *allpkts[3];
+    int i, duprec = ctx->duprec > 0;
+    const unsigned char *inu = (const unsigned char *)in;
+    size_t len = ((inu[RECORD_LEN_HI] << 8) | inu[RECORD_LEN_LO])
+                 + DTLS1_RT_HEADER_LENGTH;
 
     if (ctx == NULL)
         return -1;
 
+    if ((size_t)inl < len)
+        return -1;
+
+    if ((size_t)inl == len)
+        duprec = 0;
+
+    /* We don't support arbitrary injection when duplicating records */
+    if (duprec && pktnum != -1)
+        return -1;
+
     /* We only allow injection before we've started writing any data */
     if (pktnum >= 0) {
         if (ctx->noinject)
@@ -441,25 +455,36 @@ int mempacket_test_inject(BIO *bio, const char *in, int inl, int pktnum,
         ctx->noinject = 1;
     }
 
-    if (!TEST_ptr(thispkt = OPENSSL_malloc(sizeof(*thispkt))))
-        return -1;
-    if (!TEST_ptr(thispkt->data = OPENSSL_malloc(inl))) {
-        mempacket_free(thispkt);
-        return -1;
-    }
+    for (i = 0; i < (duprec ? 3 : 1); i++) {
+        if (!TEST_ptr(allpkts[i] = OPENSSL_malloc(sizeof(*thispkt))))
+            goto err;
+        thispkt = allpkts[i];
 
-    memcpy(thispkt->data, in, inl);
-    thispkt->len = inl;
-    thispkt->num = (pktnum >= 0) ? (unsigned int)pktnum : ctx->lastpkt;
-    thispkt->type = type;
+        if (!TEST_ptr(thispkt->data = OPENSSL_malloc(inl)))
+            goto err;
+        /*
+         * If we are duplicating the packet, we duplicate it three times. The
+         * first two times we drop the first record if there are more than one.
+         * In this way we know that libssl will not be able to make progress
+         * until it receives the last packet, and hence will be forced to
+         * buffer these records.
+         */
+        if (duprec && i != 2) {
+            memcpy(thispkt->data, in + len, inl - len);
+            thispkt->len = inl - len;
+        } else {
+            memcpy(thispkt->data, in, inl);
+            thispkt->len = inl;
+        }
+        thispkt->num = (pktnum >= 0) ? (unsigned int)pktnum : ctx->lastpkt + i;
+        thispkt->type = type;
+    }
 
     for(i = 0; (looppkt = sk_MEMPACKET_value(ctx->pkts, i)) != NULL; i++) {
         /* Check if we found the right place to insert this packet */
         if (looppkt->num > thispkt->num) {
-            if (sk_MEMPACKET_insert(ctx->pkts, thispkt, i) == 0) {
-                mempacket_free(thispkt);
-                return -1;
-            }
+            if (sk_MEMPACKET_insert(ctx->pkts, thispkt, i) == 0)
+                goto err;
             /* If we're doing up front injection then we're done */
             if (pktnum >= 0)
                 return inl;
@@ -480,7 +505,7 @@ int mempacket_test_inject(BIO *bio, const char *in, int inl, int pktnum,
         } else if (looppkt->num == thispkt->num) {
             if (!ctx->noinject) {
                 /* We injected two packets with the same packet number! */
-                return -1;
+                goto err;
             }
             ctx->lastpkt++;
             thispkt->num++;
@@ -490,15 +515,21 @@ int mempacket_test_inject(BIO *bio, const char *in, int inl, int pktnum,
      * We didn't find any packets with a packet number equal to or greater than
      * this one, so we just add it onto the end
      */
-    if (!sk_MEMPACKET_push(ctx->pkts, thispkt)) {
-        mempacket_free(thispkt);
-        return -1;
-    }
+    for (i = 0; i < (duprec ? 3 : 1); i++) {
+        thispkt = allpkts[i];
+        if (!sk_MEMPACKET_push(ctx->pkts, thispkt))
+            goto err;
 
-    if (pktnum < 0)
-        ctx->lastpkt++;
+        if (pktnum < 0)
+            ctx->lastpkt++;
+    }
 
     return inl;
+
+ err:
+    for (i = 0; i < (ctx->duprec > 0 ? 3 : 1); i++)
+        mempacket_free(allpkts[i]);
+    return -1;
 }
 
 static int mempacket_test_write(BIO *bio, const char *in, int inl)
@@ -544,6 +575,9 @@ static long mempacket_test_ctrl(BIO *bio, int cmd, long num, void *ptr)
     case MEMPACKET_CTRL_GET_DROP_REC:
         ret = ctx->droprec;
         break;
+    case MEMPACKET_CTRL_SET_DUPLICATE_REC:
+        ctx->duprec = (int)num;
+        break;
     case BIO_CTRL_RESET:
     case BIO_CTRL_DUP:
     case BIO_CTRL_PUSH:
diff --git a/test/ssltestlib.h b/test/ssltestlib.h
index 31e3037..27b040c 100644
--- a/test/ssltestlib.h
+++ b/test/ssltestlib.h
@@ -37,9 +37,10 @@ void bio_s_mempacket_test_free(void);
  * Mempacket BIO ctrls. We make them large enough to not clash with standard BIO
  * ctrl codes.
  */
-#define MEMPACKET_CTRL_SET_DROP_EPOCH  (1 << 15)
-#define MEMPACKET_CTRL_SET_DROP_REC    (2 << 15)
-#define MEMPACKET_CTRL_GET_DROP_REC    (3 << 15)
+#define MEMPACKET_CTRL_SET_DROP_EPOCH       (1 << 15)
+#define MEMPACKET_CTRL_SET_DROP_REC         (2 << 15)
+#define MEMPACKET_CTRL_GET_DROP_REC         (3 << 15)
+#define MEMPACKET_CTRL_SET_DUPLICATE_REC    (4 << 15)
 
 int mempacket_test_inject(BIO *bio, const char *in, int inl, int pktnum,
                           int type);


More information about the openssl-commits mailing list