From ebd8b8965c74ab06bb91f7a00b23822e1f1f26ca Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 Jul 2022 18:41:51 -0700 Subject: [PATCH] Track SSL_ERROR_ZERO_RETURN explicitly. Most SSL_ERROR_* values are tracked directly with rwstate. SSL_get_error is just reading the extra return value out from the previous call. However, SSL_ERROR_ZERO_RETURN infers close_notify from the SSL's shutdown state and a zero return value (EOF). This works, but if we implement SSL_read_ex and SSL_write_ex, a zero return value is no longer as carefully correlated with EOF. Moreover, it's already possible to get a non-EOF zero return post-close_notify if BIO_write returns an (arguably incorrect) return value. Instead, track SSL_ERROR_ZERO_RETURN in rwstate explicitly. Since rwstate is exposed as SSL_want and SSL_ERROR_ZERO_RETURN was previously never returned there, I've made it map SSL_ERROR_ZERO_RETURN back to SSL_ERROR_NONE. I've also added a test for BIO_write returning zero, though the real purpose is for a subsequent SSL_write_ex implementation to retain all the other tests we've added in here. Update-Note: This is intended to be safe, but if anything breaks around EOFs, this change is a likely culprit. Bug: 507 Change-Id: Ide0807665f2e02ee695c4976dc5e99fb10502cf0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53946 Auto-Submit: David Benjamin Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- ssl/ssl_buffer.cc | 1 + ssl/ssl_lib.cc | 10 ++++++++-- ssl/ssl_test.cc | 21 +++++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/ssl/ssl_buffer.cc b/ssl/ssl_buffer.cc index d73055fbf..2ca14efae 100644 --- a/ssl/ssl_buffer.cc +++ b/ssl/ssl_buffer.cc @@ -232,6 +232,7 @@ int ssl_handle_open_record(SSL *ssl, bool *out_retry, ssl_open_record_t ret, return 1; case ssl_open_record_close_notify: + ssl->s3->rwstate = SSL_ERROR_ZERO_RETURN; return 0; case ssl_open_record_error: diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 56bc5c22e..3b06bf0a8 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1319,7 +1319,7 @@ int SSL_get_error(const SSL *ssl, int ret_code) { } if (ret_code == 0) { - if (ssl->s3->read_shutdown == ssl_shutdown_close_notify) { + if (ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { return SSL_ERROR_ZERO_RETURN; } // An EOF was observed which violates the protocol, and the underlying @@ -2578,7 +2578,13 @@ void *SSL_CTX_get_ex_data(const SSL_CTX *ctx, int idx) { return CRYPTO_get_ex_data(&ctx->ex_data, idx); } -int SSL_want(const SSL *ssl) { return ssl->s3->rwstate; } +int SSL_want(const SSL *ssl) { + // Historically, OpenSSL did not track |SSL_ERROR_ZERO_RETURN| as an |rwstate| + // value. We do, but map it back to |SSL_ERROR_NONE| to preserve the original + // behavior. + return ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN ? SSL_ERROR_NONE + : ssl->s3->rwstate; +} void SSL_CTX_set_tmp_rsa_callback(SSL_CTX *ctx, RSA *(*cb)(SSL *ssl, int is_export, diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 1db97a9f1..9f488d566 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -8450,6 +8450,11 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { EXPECT_EQ(ret, 0); EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_ZERO_RETURN); + // Further calls to |SSL_read| continue to report |SSL_ERROR_ZERO_RETURN|. + ret = SSL_read(client.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_ZERO_RETURN); + // Although the client has seen close_notify, it should continue to report // |SSL_ERROR_SYSCALL| when its writes fail. ret = SSL_write(client.get(), data, sizeof(data)); @@ -8457,6 +8462,22 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); EXPECT_TRUE(write_failed); write_failed = false; + + // Cause |BIO_write| to fail with a return value of zero instead. + // |SSL_get_error| should not misinterpret this as a close_notify. + // + // This is not actually a correct implementation of |BIO_write|, but the rest + // of the code treats zero from |BIO_write| as an error, so ensure it does so + // correctly. Fixing https://crbug.com/boringssl/503 will make this case moot. + BIO_meth_set_write(method.get(), [](BIO *, const char *, int) -> int { + write_failed = true; + return 0; + }); + ret = SSL_write(client.get(), data, sizeof(data)); + EXPECT_EQ(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); + EXPECT_TRUE(write_failed); + write_failed = false; } // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving