[openssl-commits] [openssl] master update

[Matt Caswell]

The branch master has been updated
       via  7856332e8c14fd1da1811a9d0afde243dd0f4669 (commit)
       via  a7faa6da317887e14e8e28254a83555983ed6ca7 (commit)
      from  8aefa08cfbc7db7cc10765ee9684090e37983f45 (commit)


- Log -----------------------------------------------------------------
commit 7856332e8c14fd1da1811a9d0afde243dd0f4669
Author: Matt Caswell <matt at openssl.org>
Date:   Wed Nov 2 10:44:15 2016 +0000

    Add a read_ahead test
    
    This test checks that read_ahead works correctly when dealing with large
    records.
    
    Reviewed-by: Richard Levitte <levitte at openssl.org>

commit a7faa6da317887e14e8e28254a83555983ed6ca7
Author: Matt Caswell <matt at openssl.org>
Date:   Wed Nov 2 10:34:12 2016 +0000

    Fix read_ahead
    
    The function ssl3_read_n() takes a parameter |clearold| which, if set,
    causes any old data in the read buffer to be forgotten, and any unread data
    to be moved to the start of the buffer. This is supposed to happen when we
    first read the record header.
    
    However, the data move was only taking place if there was not already
    sufficient data in the buffer to satisfy the request. If read_ahead is set
    then the record header could be in the buffer already from when we read the
    preceding record. So with read_ahead we can get into a situation where even
    though |clearold| is set, the data does not get moved to the start of the
    read buffer when we read the record header. This means there is insufficient
    room in the read buffer to consume the rest of the record body, resulting in
    an internal error.
    
    This commit moves the |clearold| processing to earlier in ssl3_read_n()
    to ensure that it always takes place.
    
    Reviewed-by: Richard Levitte <levitte at openssl.org>

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

Summary of changes:
 ssl/record/rec_layer_s3.c | 24 ++++++++++++------------
 test/sslapitest.c         | 26 +++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 9c8c23c..4535f89 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -241,6 +241,18 @@ int ssl3_read_n(SSL *s, int n, int max, int extend, int clearold)
         /* ... now we can act as if 'extend' was set */
     }
 
+    len = s->rlayer.packet_length;
+    pkt = rb->buf + align;
+    /*
+     * Move any available bytes to front of buffer: 'len' bytes already
+     * pointed to by 'packet', 'left' extra ones at the end
+     */
+    if (s->rlayer.packet != pkt && clearold == 1) {
+        memmove(pkt, s->rlayer.packet, len + left);
+        s->rlayer.packet = pkt;
+        rb->offset = len + align;
+    }
+
     /*
      * For DTLS/UDP reads should not span multiple packets because the read
      * operation returns the whole packet at once (as long as it fits into
@@ -263,18 +275,6 @@ int ssl3_read_n(SSL *s, int n, int max, int extend, int clearold)
 
     /* else we need to read more data */
 
-    len = s->rlayer.packet_length;
-    pkt = rb->buf + align;
-    /*
-     * Move any available bytes to front of buffer: 'len' bytes already
-     * pointed to by 'packet', 'left' extra ones at the end
-     */
-    if (s->rlayer.packet != pkt && clearold == 1) { /* len > 0 */
-        memmove(pkt, s->rlayer.packet, len + left);
-        s->rlayer.packet = pkt;
-        rb->offset = len + align;
-    }
-
     if (n > (int)(rb->len - rb->offset)) { /* does not happen */
         SSLerr(SSL_F_SSL3_READ_N, ERR_R_INTERNAL_ERROR);
         return -1;
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 4d22d8e..a78b060 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -31,7 +31,7 @@ static X509 *ocspcert = NULL;
 #define NUM_EXTRA_CERTS 40
 
 static int execute_test_large_message(const SSL_METHOD *smeth,
-                                      const SSL_METHOD *cmeth)
+                                      const SSL_METHOD *cmeth, int read_ahead)
 {
     SSL_CTX *cctx = NULL, *sctx = NULL;
     SSL *clientssl = NULL, *serverssl = NULL;
@@ -59,6 +59,14 @@ static int execute_test_large_message(const SSL_METHOD *smeth,
         goto end;
     }
 
+    if(read_ahead) {
+        /*
+         * Test that read_ahead works correctly when dealing with large
+         * records
+         */
+        SSL_CTX_set_read_ahead(cctx, 1);
+    }
+
     /*
      * We assume the supplied certificate is big enough so that if we add
      * NUM_EXTRA_CERTS it will make the overall message large enough. The
@@ -105,14 +113,25 @@ static int execute_test_large_message(const SSL_METHOD *smeth,
 
 static int test_large_message_tls(void)
 {
-    return execute_test_large_message(TLS_server_method(), TLS_client_method());
+    return execute_test_large_message(TLS_server_method(), TLS_client_method(),
+                                      0);
+}
+
+static int test_large_message_tls_read_ahead(void)
+{
+    return execute_test_large_message(TLS_server_method(), TLS_client_method(),
+                                      1);
 }
 
 #ifndef OPENSSL_NO_DTLS
 static int test_large_message_dtls(void)
 {
+    /*
+     * read_ahead is not relevant to DTLS because DTLS always acts as if
+     * read_ahead is set.
+     */
     return execute_test_large_message(DTLS_server_method(),
-                                      DTLS_client_method());
+                                      DTLS_client_method(), 0);
 }
 #endif
 
@@ -863,6 +882,7 @@ int main(int argc, char *argv[])
     CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON);
 
     ADD_TEST(test_large_message_tls);
+    ADD_TEST(test_large_message_tls_read_ahead);
 #ifndef OPENSSL_NO_DTLS
     ADD_TEST(test_large_message_dtls);
 #endif