Chiebot-Mirror
/
boringssl
mirror of https://gitee.com/mirrors/boringssl.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Fix an edge case in SSL_write's retry mechanism.

Browse Source 
This is split out from
https://boringssl-review.googlesource.com/c/boringssl/+/47544 just to
get the bugfixes and tests out of the way of the refactor.

If we trip the SSL_R_BAD_LENGTH check in tls_write_app_data, wnum is set
to zero. But wnum should only be cleared on a successful write. It
tracks the number of input bytes that have been written to the transport
but not yet reported to the caller. Instead, move it to the success
return in that function. All the other error paths already set it to
something else.

Change-Id: Ib22f9cf04454ecdb0062077f183be5070ab7d791
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53545
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
chromium-5359
David Benjamin 2 years ago committed by Boringssl LUCI CQ
parent 28883d4d81
commit 64bf8c50a3
2 changed files with 89 additions and 19 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 2
      ssl/s3_pkt.cc
  2. 106
      ssl/ssl_test.cc

2
ssl/s3_pkt.cc
Unescape View File

@ -143,7 +143,6 @@ int tls_write_app_data(SSL *ssl, bool *out_needs_handshake, const uint8_t *in,
// TODO(davidben): Switch this logic to |size_t| and |bssl::Span|.
assert(ssl->s3->wnum <= INT_MAX);
unsigned tot = ssl->s3->wnum;
ssl->s3->wnum = 0;
// Ensure that if we end up with a smaller value of data to write out than
// the the original len from a write which didn't complete for non-blocking
@ -188,6 +187,7 @@ int tls_write_app_data(SSL *ssl, bool *out_needs_handshake, const uint8_t *in,
}
if (ret == (int)n || (ssl->mode & SSL_MODE_ENABLE_PARTIAL_WRITE)) {
ssl->s3->wnum = 0;
return tot + ret;
}

106
ssl/ssl_test.cc
Unescape View File

@ -3904,18 +3904,18 @@ TEST_P(SSLVersionTest, AutoChain) {
{cert_.get(), cert_.get()}));
}
static bool ExpectBadWriteRetry() {
static bool ExpectSingleError(int lib, int reason) {
const char *expected = ERR_reason_error_string(ERR_PACK(lib, reason));
int err = ERR_get_error();
if (ERR_GET_LIB(err) != ERR_LIB_SSL ||
ERR_GET_REASON(err) != SSL_R_BAD_WRITE_RETRY) {
if (ERR_GET_LIB(err) != lib || ERR_GET_REASON(err) != reason) {
char buf[ERR_ERROR_STRING_BUF_LEN];
ERR_error_string_n(err, buf, sizeof(buf));
fprintf(stderr, "Wanted SSL_R_BAD_WRITE_RETRY, got: %s.\n", buf);
fprintf(stderr, "Wanted %s, got: %s.\n", expected, buf);
return false;
}
if (ERR_peek_error() != 0) {
fprintf(stderr, "Unexpected error following SSL_R_BAD_WRITE_RETRY.\n");
fprintf(stderr, "Unexpected error following %s.\n", expected);
return false;
}
@ -3931,8 +3931,6 @@ TEST_P(SSLVersionTest, SSLWriteRetry) {
SCOPED_TRACE(enable_partial_write);
// Connect a client and server.
ASSERT_TRUE(UseCertAndKey(client_ctx_.get()));
ASSERT_TRUE(Connect());
if (enable_partial_write) {
@ -3951,9 +3949,7 @@ TEST_P(SSLVersionTest, SSLWriteRetry) {
ASSERT_EQ(SSL_get_error(client_.get(), ret), SSL_ERROR_WANT_WRITE);
break;
}
ASSERT_EQ(ret, 5);
count++;
}
@ -3966,14 +3962,14 @@ TEST_P(SSLVersionTest, SSLWriteRetry) {
ASSERT_EQ(SSL_get_error(client_.get(),
SSL_write(client_.get(), data, kChunkLen - 1)),
SSL_ERROR_SSL);
ASSERT_TRUE(ExpectBadWriteRetry());
ASSERT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_BAD_WRITE_RETRY));
// Retrying with a different buffer pointer is not legal.
char data2[] = "hello";
ASSERT_EQ(SSL_get_error(client_.get(),
SSL_write(client_.get(), data2, kChunkLen)),
SSL_ERROR_SSL);
ASSERT_TRUE(ExpectBadWriteRetry());
ASSERT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_BAD_WRITE_RETRY));
// With |SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER|, the buffer may move.
SSL_set_mode(client_.get(), SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
@ -3985,7 +3981,7 @@ TEST_P(SSLVersionTest, SSLWriteRetry) {
ASSERT_EQ(SSL_get_error(client_.get(),
SSL_write(client_.get(), data2, kChunkLen - 1)),
SSL_ERROR_SSL);
ASSERT_TRUE(ExpectBadWriteRetry());
ASSERT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_BAD_WRITE_RETRY));
// Retrying with a larger buffer is legal.
ASSERT_EQ(SSL_get_error(client_.get(),
@ -4003,20 +3999,94 @@ TEST_P(SSLVersionTest, SSLWriteRetry) {
// pending record, skip over that many bytes of input (on assumption they
// are the same), and write the remainder. If SSL_MODE_ENABLE_PARTIAL_WRITE
// is set, this will complete in two steps.
char data3[] = "_____!";
char data_longer[] = "_____!!!!!";
if (enable_partial_write) {
ASSERT_EQ(SSL_write(client_.get(), data3, kChunkLen + 1), kChunkLen);
ASSERT_EQ(SSL_write(client_.get(), data3 + kChunkLen, 1), 1);
ASSERT_EQ(SSL_write(client_.get(), data_longer, 2 * kChunkLen),
kChunkLen);
ASSERT_EQ(SSL_write(client_.get(), data_longer + kChunkLen, kChunkLen),
kChunkLen);
} else {
ASSERT_EQ(SSL_write(client_.get(), data3, kChunkLen + 1), kChunkLen + 1);
ASSERT_EQ(SSL_write(client_.get(), data_longer, 2 * kChunkLen),
2 * kChunkLen);
}
// Check the last write was correct. The data will be spread over two
// records, so SSL_read returns twice.
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0);
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), 1);
ASSERT_EQ(buf[0], '!');
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "!!!!!", kChunkLen), 0);
// Fill the transport buffer again. This time only leave room for one
// record.
count = 0;
for (;;) {
int ret = SSL_write(client_.get(), data, kChunkLen);
if (ret <= 0) {
ASSERT_EQ(SSL_get_error(client_.get(), ret), SSL_ERROR_WANT_WRITE);
break;
}
ASSERT_EQ(ret, 5);
count++;
}
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0);
count--;
// Retry the last write, with a longer input. The first half is the most
// recently failed write, from filling the buffer. |SSL_write| should write
// that to the transport, and then attempt to write the second half.
int ret = SSL_write(client_.get(), data_longer, 2 * kChunkLen);
if (enable_partial_write) {
// If partial writes are allowed, the write will succeed partially.
ASSERT_EQ(ret, kChunkLen);
// Check the first half and make room for another record.
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0);
count--;
// Finish writing the input.
ASSERT_EQ(SSL_write(client_.get(), data_longer + kChunkLen, kChunkLen),
kChunkLen);
} else {
// Otherwise, although the first half made it to the transport, the second
// half is blocked.
ASSERT_EQ(ret, -1);
ASSERT_EQ(SSL_get_error(client_.get(), -1), SSL_ERROR_WANT_WRITE);
// Check the first half and make room for another record.
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0);
count--;
// Retrying with fewer bytes than previously attempted is an error. If the
// input length is less than the number of bytes successfully written, the
// check happens at a different point, with a different error.
//
// TODO(davidben): Should these cases use the same error?
ASSERT_EQ(
SSL_get_error(client_.get(),
SSL_write(client_.get(), data_longer, kChunkLen - 1)),
SSL_ERROR_SSL);
ASSERT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_BAD_LENGTH));
// Complete the write with the correct retry.
ASSERT_EQ(SSL_write(client_.get(), data_longer, 2 * kChunkLen),
2 * kChunkLen);
}
// Drain the input and ensure everything was written correctly.
for (unsigned i = 0; i < count; i++) {
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0);
}
// The final write is spread over two records.
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "hello", kChunkLen), 0);
ASSERT_EQ(SSL_read(server_.get(), buf, sizeof(buf)), kChunkLen);
ASSERT_EQ(OPENSSL_memcmp(buf, "!!!!!", kChunkLen), 0);
}
}

Write Preview
Loading…
Cancel
Save