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 <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

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:

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,

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