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>
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index bdd3566..3edbd8b 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -6,6 +6,7 @@
"flag"
"fmt"
"io"
+ "io/ioutil"
"net"
"os"
"os/exec"
@@ -381,6 +382,13 @@
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 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 @@
addCipherSuiteTests()
addBadECDSASignatureTests()
addCBCPaddingTests()
+ addCBCSplittingTests()
addClientAuthTests()
for _, async := range []bool{false, true} {
for _, splitHandshake := range []bool{false, true} {
