Remove support for SSL3_FLAGS_DELAY_CLIENT_FINISHED

This flag was not set anywhere within the codebase (only read). It could
only be set by an app reaching directly into s->s3->flags and setting it
directly. However that method became impossible when libssl was opaquified.

Even in 1.0.2/1.0.1 if an app set the flag directly it is only relevant to
ssl3_connect(), which calls SSL_clear() during initialisation that clears
any flag settings. Therefore it could take effect if the app set the flag
after the handshake has started but before it completed. It seems quite
unlikely that any apps really do this (especially as it is completely
undocumented).

The purpose of the flag is suppress flushing of the write bio on the client
side at the end of the handshake after the client has written the Finished
message whilst resuming a session. This enables the client to send
application data as part of the same flight as the Finished message.

This flag also controls the setting of a second flag SSL3_FLAGS_POP_BUFFER.
There is an interesting comment in the code about this second flag in the
implementation of ssl3_write:

	/* This is an experimental flag that sends the
	 * last handshake message in the same packet as the first
	 * use data - used to see if it helps the TCP protocol during
	 * session-id reuse */

It seems the experiment did not work because as far as I can tell nothing
is using this code. The above comment has been in the code since SSLeay.

This commit removes support for SSL3_FLAGS_DELAY_CLIENT_FINISHED, as well
as the associated SSL3_FLAGS_POP_BUFFER.

Reviewed-by: Rich Salz <rsalz@openssl.org>

diff --git a/CHANGES b/CHANGES
index a06b1e4c24937f98e2a984666bdb6b9913e7ab69..1526aec922e0f005b09995e5eb582de36dac4a8d 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -3,6 +3,12 @@
  _______________
 
  Changes between 1.0.2 and 1.1.0  [xx XXX xxxx]
+  *) Dropped support for the SSL3_FLAGS_DELAY_CLIENT_FINISHED flag. This SSLeay
+     era flag was never set throughout the codebase (only read). Also removed
+     SSL3_FLAGS_POP_BUFFER which was only used if
+     SSL3_FLAGS_DELAY_CLIENT_FINISHED was also set.
+     [Matt Caswell]
+
   *) Changed the default name options in the "ca", "crl", "req" and "x509"
      to be "oneline" instead of "compat".
      [Richard Levitte]

diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index d56105e17834f9ae6c3162842999b9d191e20290..43df925999bc0ef1be1c9d9e22ccda87dd0b2eb7 100644 (file)
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -360,10 +360,10 @@ extern "C" {
 # define SSL3_CT_NUMBER                  9
 
 # define SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS       0x0001
-# define SSL3_FLAGS_DELAY_CLIENT_FINISHED        0x0002
-# define SSL3_FLAGS_POP_BUFFER                   0x0004
+
 /* Removed from OpenSSL 1.1.0 */
 # define TLS1_FLAGS_TLS_PADDING_BUG              0x0
+
 # define TLS1_FLAGS_SKIP_CERT_VERIFY             0x0010
 /*
  * Set when the handshake is ready to process peer's ChangeCipherSpec message.

diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 8940abc41fa2824eb2bd9187f2efdc13b2c8cbdd..fde0defef967c6fb462cfdcf814a0b112fa0dd78 100644 (file)
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -604,8 +604,6 @@ int dtls1_connect(SSL *s)
                 goto end;
             s->state = SSL3_ST_CW_FLUSH;
 
-            /* clear flags */
-            s->s3->flags &= ~SSL3_FLAGS_POP_BUFFER;
             if (s->hit) {
                 s->s3->tmp.next_state = SSL_ST_OK;
 #ifndef OPENSSL_NO_SCTP
@@ -614,17 +612,6 @@ int dtls1_connect(SSL *s)
                     s->s3->tmp.next_state = DTLS1_SCTP_ST_CW_WRITE_SOCK;
                 }
 #endif
-                if (s->s3->flags & SSL3_FLAGS_DELAY_CLIENT_FINISHED) {
-                    s->state = SSL_ST_OK;
-#ifndef OPENSSL_NO_SCTP
-                    if (BIO_dgram_is_sctp(SSL_get_wbio(s))) {
-                        s->d1->next_state = SSL_ST_OK;
-                        s->state = DTLS1_SCTP_ST_CW_WRITE_SOCK;
-                    }
-#endif
-                    s->s3->flags |= SSL3_FLAGS_POP_BUFFER;
-                    s->s3->delay_buf_pop_ret = 0;
-                }
             } else {
 #ifndef OPENSSL_NO_SCTP
                 /*
@@ -711,13 +698,8 @@ int dtls1_connect(SSL *s)
             /* clean a few things up */
             ssl3_cleanup_key_block(s);
 
-            /*
-             * If we are not 'joining' the last two packets, remove the
-             * buffering now
-             */
-            if (!(s->s3->flags & SSL3_FLAGS_POP_BUFFER))
-                ssl_free_wbio_buffer(s);
-            /* else do it later in ssl3_write */
+            /* Remove the buffering */
+            ssl_free_wbio_buffer(s);
 
             s->init_num = 0;
             s->renegotiate = 0;

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 1a925a77c9605dc7169e73bdec3a556117257712..04af8514d8b327b38e1508fe9e8b011548c65897 100644 (file)
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -518,15 +518,8 @@ int ssl3_connect(SSL *s)
                 goto end;
             s->state = SSL3_ST_CW_FLUSH;
 
-            /* clear flags */
-            s->s3->flags &= ~SSL3_FLAGS_POP_BUFFER;
             if (s->hit) {
                 s->s3->tmp.next_state = SSL_ST_OK;
-                if (s->s3->flags & SSL3_FLAGS_DELAY_CLIENT_FINISHED) {
-                    s->state = SSL_ST_OK;
-                    s->s3->flags |= SSL3_FLAGS_POP_BUFFER;
-                    s->s3->delay_buf_pop_ret = 0;
-                }
             } else {
                 /*
                  * Allow NewSessionTicket if ticket expected
@@ -589,13 +582,8 @@ int ssl3_connect(SSL *s)
             BUF_MEM_free(s->init_buf);
             s->init_buf = NULL;
 
-            /*
-             * If we are not 'joining' the last two packets, remove the
-             * buffering now
-             */
-            if (!(s->s3->flags & SSL3_FLAGS_POP_BUFFER))
-                ssl_free_wbio_buffer(s);
-            /* else do it later in ssl3_write */
+            /* remove the buffering */
+            ssl_free_wbio_buffer(s);
 
             s->init_num = 0;
             s->renegotiate = 0;

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 54c902d8c917ad1a7bbda18af7358fb16f6d1c76..8b7c52af52699704e8ce0c7a5efa0dd96e28f059 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4131,54 +4131,12 @@ int ssl3_shutdown(SSL *s)
 
 int ssl3_write(SSL *s, const void *buf, int len)
 {
-    int ret, n;
-
-#if 0
-    if (s->shutdown & SSL_SEND_SHUTDOWN) {
-        s->rwstate = SSL_NOTHING;
-        return (0);
-    }
-#endif
     clear_sys_error();
     if (s->s3->renegotiate)
         ssl3_renegotiate_check(s);
 
-    /*
-     * This is an experimental flag that sends the last handshake message in
-     * the same packet as the first use data - used to see if it helps the
-     * TCP protocol during session-id reuse
-     */
-    /* The second test is because the buffer may have been removed */
-    if ((s->s3->flags & SSL3_FLAGS_POP_BUFFER) && (s->wbio == s->bbio)) {
-        /* First time through, we write into the buffer */
-        if (s->s3->delay_buf_pop_ret == 0) {
-            ret = ssl3_write_bytes(s, SSL3_RT_APPLICATION_DATA, buf, len);
-            if (ret <= 0)
-                return (ret);
-
-            s->s3->delay_buf_pop_ret = ret;
-        }
-
-        s->rwstate = SSL_WRITING;
-        n = BIO_flush(s->wbio);
-        if (n <= 0)
-            return (n);
-        s->rwstate = SSL_NOTHING;
-
-        /* We have flushed the buffer, so remove it */
-        ssl_free_wbio_buffer(s);
-        s->s3->flags &= ~SSL3_FLAGS_POP_BUFFER;
-
-        ret = s->s3->delay_buf_pop_ret;
-        s->s3->delay_buf_pop_ret = 0;
-    } else {
-        ret = s->method->ssl_write_bytes(s, SSL3_RT_APPLICATION_DATA,
+    return s->method->ssl_write_bytes(s, SSL3_RT_APPLICATION_DATA,
                                          buf, len);
-        if (ret <= 0)
-            return (ret);
-    }
-
-    return (ret);
 }
 
 static int ssl3_read_internal(SSL *s, void *buf, int len, int peek)

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 2672918ba6b2f11c4235ba0d996a612ec129c62d..1cdcb8ba9fb512577b597255f11b2b2615743cf2 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1184,7 +1184,6 @@ struct ssl_st {
 
 typedef struct ssl3_state_st {
     long flags;
-    int delay_buf_pop_ret;
     int read_mac_secret_size;
     unsigned char read_mac_secret[EVP_MAX_MD_SIZE];
     int write_mac_secret_size;