[openssl-commits] [openssl] OpenSSL_1_1_0-stable update

[Matt Caswell]

The branch OpenSSL_1_1_0-stable has been updated
       via  3f99bfed678b09110fda82bc6896fd45eb0b376c (commit)
       via  0f6c9d73cb1e1027c67d993a669719e351c25cfc (commit)
      from  a95a0219a887611ad8e246e33c086255df771072 (commit)


- Log -----------------------------------------------------------------
commit 3f99bfed678b09110fda82bc6896fd45eb0b376c
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>
    (cherry picked from commit 7856332e8c14fd1da1811a9d0afde243dd0f4669)

commit 0f6c9d73cb1e1027c67d993a669719e351c25cfc
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>
    (cherry picked from commit a7faa6da317887e14e8e28254a83555983ed6ca7)

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

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 495bf26..90326d9 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -33,7 +33,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;
@@ -61,6 +61,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
@@ -107,14 +115,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
 
@@ -867,6 +886,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