Retry sending record split fragment when SSL write fails.

When the write size was exactly SSL3_RT_MAX_PLAIN_LENGTH+1 and record
splitting is needed, an extra byte would be added to the max size of the
message to be written. This would cause the requested size to not exceed
the max. If the SSL_WANT_WRITE error were returned, the next packet
would not get the extra byte added to the max packet size since
record_split_done is set. Since a different set of arguments
(SSL3_RT_MAX_PLAIN_LENGTH+1 vs SSL3_RT_MAX_PLAIN_LENGTH) would be passed
to do_ssl3_write, it would return an "SSL3_WRITE_PENDING:bad write
retry" error.

To avoid a failure in the opposite direction, the max variable increment
is removed as well. This can happen when SSL_MODE_ENABLE_PARTIAL_WRITE
is not enabled and the call to ssl3_write_bytes contains, e.g., a buffer
of 2*SSL3_RT_MAX_PLAIN_LENGTH, where the first call into do_ssl3_write
succeeds writing the first SSL3_RT_MAX_PLAIN_LENGTH bytes, but writing
the second SSL3_RT_MAX_PLAIN_LENGTH bytes fails. This means the first
time the the second section of SSL3_RT_MAX_PLAIN_LENGTH bytes has called
do_ssl3_write with "max" bytes, but next call to ssl3_write_bytes in
turn calls into do_ssl3_write with "max+1" bytes.

Change-Id: Icf8453195c1145a54d31b8e8146801118207df03
Reviewed-on: https://boringssl-review.googlesource.com/1420
Reviewed-by: Kenny Root <kroot@google.com>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

ssl/s3_pkt.c

@@ -596,9 +596,6 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, int len)
 		    !s->s3->record_split_done)
 			{
 			fragment = 1;
-			/* The first byte will be in its own record, so we
-			 * can write an extra byte. */
-			max++;
 			/* record_split_done records that the splitting has
 			 * been done in case we hit an SSL_WANT_WRITE condition.
 			 * In that case, we don't need to do the split again. */
@@ -614,6 +611,7 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, int len)
 		if (i <= 0)
 			{
 			s->s3->wnum=tot;
+			s->s3->record_split_done = 0;
 			return i;
 			}
 

ssl/test/bssl_shim.cc

@@ -183,7 +183,7 @@ static int do_exchange(SSL_SESSION **out_session,
                        int is_resume,
                        int fd,
                        SSL_SESSION *session) {
-  int async = 0;
+  bool async = false, write_different_record_sizes = false;
   const char *expected_certificate_types = NULL;
   const char *expected_next_proto = NULL;
   expected_server_name = NULL;
@@ -264,7 +264,13 @@ static int do_exchange(SSL_SESSION **out_session,
       }
       select_next_proto = argv[i];
     } else if (strcmp(argv[i], "-async") == 0) {
-      async = 1;
+      async = true;
+    } else if (strcmp(argv[i], "-write-different-record-sizes") == 0) {
+      write_different_record_sizes = true;
+    } else if (strcmp(argv[i], "-cbc-record-splitting") == 0) {
+      SSL_set_mode(ssl, SSL_MODE_CBC_RECORD_SPLITTING);
+    } else if (strcmp(argv[i], "-partial-write") == 0) {
+      SSL_set_mode(ssl, SSL_MODE_ENABLE_PARTIAL_WRITE);
     } else {
       fprintf(stderr, "Unknown argument: %s\n", argv[i]);
       return 1;
@@ -348,32 +354,65 @@ static int do_exchange(SSL_SESSION **out_session,
     }
   }
 
-  for (;;) {
-    uint8_t buf[512];
-    int n;
-    do {
-      n = SSL_read(ssl, buf, sizeof(buf));
-    } while (async && retry_async(ssl, n, bio));
-    if (n < 0) {
-      SSL_free(ssl);
-      BIO_print_errors_fp(stdout);
-      return 3;
-    } else if (n == 0) {
-      break;
-    } else {
-      for (int i = 0; i < n; i++) {
-        buf[i] ^= 0xff;
-      }
+  if (write_different_record_sizes) {
+    // This mode writes a number of different record sizes in an attempt to
+    // trip up the CBC record splitting code.
+    uint8_t buf[32769];
+    memset(buf, 0x42, sizeof(buf));
+    static const size_t kRecordSizes[] = {
+        0, 1, 255, 256, 257, 16383, 16384, 16385, 32767, 32768, 32769};
+    for (size_t i = 0; i < sizeof(kRecordSizes) / sizeof(kRecordSizes[0]);
+         i++) {
       int w;
+      const size_t len = kRecordSizes[i];
+      size_t off = 0;
+
+      if (len > sizeof(buf)) {
+        fprintf(stderr, "Bad kRecordSizes value.\n");
+        return 5;
+      }
+
       do {
-        w = SSL_write(ssl, buf, n);
-      } while (async && retry_async(ssl, w, bio));
-      if (w != n) {
+        w = SSL_write(ssl, buf + off, len - off);
+        if (w > 0) {
+          off += (size_t) w;
+        }
+      } while ((async && retry_async(ssl, w, bio)) || (w > 0 && off < len));
+
+      if (w < 0 || off != len) {
         SSL_free(ssl);
         BIO_print_errors_fp(stdout);
         return 4;
       }
     }
+  } else {
+    for (;;) {
+      uint8_t buf[512];
+      int n;
+      do {
+        n = SSL_read(ssl, buf, sizeof(buf));
+      } while (async && retry_async(ssl, n, bio));
+      if (n < 0) {
+        SSL_free(ssl);
+        BIO_print_errors_fp(stdout);
+        return 3;
+      } else if (n == 0) {
+        break;
+      } else {
+        for (int i = 0; i < n; i++) {
+          buf[i] ^= 0xff;
+        }
+        int w;
+        do {
+          w = SSL_write(ssl, buf, n);
+        } while (async && retry_async(ssl, w, bio));
+        if (w != n) {
+          SSL_free(ssl);
+          BIO_print_errors_fp(stdout);
+          return 4;
+        }
+      }
+    }
   }
 
   if (out_session) {

ssl/test/runner/runner.go

@@ -6,6 +6,7 @@ import (
 	"flag"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net"
 	"os"
 	"os/exec"
@@ -381,6 +382,13 @@ func doExchange(testType testType, config *Config, conn net.Conn, messageLen int
 	if err := tlsConn.Handshake(); err != nil {
 		return err
 	}
+
+	if messageLen < 0 {
+		// Read until EOF.
+		_, err := io.Copy(ioutil.Discard, tlsConn)
+		return err
+	}
+
 	if messageLen == 0 {
 		messageLen = 32
 	}
@@ -714,6 +722,38 @@ func addCBCPaddingTests() {
 	})
 }
 
+func addCBCSplittingTests() {
+	testCases = append(testCases, testCase{
+		name: "CBCRecordSplitting",
+		config: Config{
+			MaxVersion:   VersionTLS10,
+			MinVersion:   VersionTLS10,
+			CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA},
+		},
+		messageLen: -1, // read until EOF
+		flags: []string{
+			"-async",
+			"-write-different-record-sizes",
+			"-cbc-record-splitting",
+		},
+	},
+	testCase{
+		name: "CBCRecordSplittingPartialWrite",
+		config: Config{
+			MaxVersion:   VersionTLS10,
+			MinVersion:   VersionTLS10,
+			CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA},
+		},
+		messageLen: -1, // read until EOF
+		flags: []string{
+			"-async",
+			"-write-different-record-sizes",
+			"-cbc-record-splitting",
+			"-partial-write",
+		},
+	})
+}
+
 func addClientAuthTests() {
 	// Add a dummy cert pool to stress certificate authority parsing.
 	// TODO(davidben): Add tests that those values parse out correctly.
@@ -966,6 +1006,7 @@ func main() {
 	addCipherSuiteTests()
 	addBadECDSASignatureTests()
 	addCBCPaddingTests()
+	addCBCSplittingTests()
 	addClientAuthTests()
 	for _, async := range []bool{false, true} {
 		for _, splitHandshake := range []bool{false, true} {