OpenSSL is intolerant to key_update_requested while writes are pending #8677
Description
In TLS servers where asynchronous reads and writes may be flowing concurrently (as is common for HTTP/2), OpenSSL's TLS 1.3 implementation is intolerant to KeyUpdates with the key_update_requested message sent. It fails with the following error:
136739560642304:error:1409E10F:SSL routines:ssl3_write_bytes:bad length:ssl/record/rec_layer_s3.c:368:
This appears to be caused by two properties of OpenSSL's implementation:
- When OpenSSL receives a
key_update_requestedin anSSL_read, it begins a "handshake" and blocksSSL_readon its completion, queuing the write immediately. - The
wpend_tot, etc., logic is placed deep in the write path, where it cannot handle multiple streams of input.
Due to (1), key_update_requested causes a read-triggered write. From there, (2) causes the write to get rejected, because it is already in the middle of writing application data. I suspect HTTP/2 clients with 0-RTT will similarly run into problems.
As a suggested fix, we avoided this in BoringSSL in two ways. First, KeyUpdates in BoringSSL don't cause read-triggered writes to begin with. When we see a key_update_requested, we queue up a KeyUpdate on the write side (and then set a flag to avoid DoS), but we do not flush until SSL_write (after that record goes through, we clear the flag to allow new KeyUpdates). This avoids this problem, surprising I/O patterns, tying up the read and write channels, and DoS problems. (During standardization, KeyUpdates were revised because of exactly this problem.)
Second, where read-triggered writes are unavoidable (0-RTT), we've reworked the write flow to separate application write data from internal "incidental" write data, and arrange for the two to coexist. If we read the ServerHello..Finished and try to send EndOfEarlyData..Finished while we're in the middle of a 0-RTT record, we flush the application data first (but, critically, do not "cash in" on the wpend_ret; that belongs to the SSL_write "retry"). If we try to write an application data record, but there is a KeyUpdate or EndOfEarlyData..Finished queued up, we incorporate it. (In this direction, our incidentals don't have the wpend_* business at all. That is all a quirk to deal with SSL_write's API imperfectly fitting how TLS looks when layered over a POSIX-style non-blocking socket.)
Here is a small test program which demonstrates the issue.
$ cat key_update_test.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <openssl/bio.h>
#include <openssl/err.h>
#include <openssl/pem.h>
#include <openssl/ssl.h>
#define CHECK(expr) \
do { \
if (!(expr)) { \
fprintf(stderr, "Check failed at line %d: %s\n", __LINE__, #expr); \
ERR_print_errors_fp(stderr); \
abort(); \
} \
} while (0)
static X509 *GetTestCertificate() {
static const char kCertPEM[] =
"-----BEGIN CERTIFICATE-----\n"
"MIIBzzCCAXagAwIBAgIJANlMBNpJfb/rMAkGByqGSM49BAEwRTELMAkGA1UEBhMC\n"
"QVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdp\n"
"dHMgUHR5IEx0ZDAeFw0xNDA0MjMyMzIxNTdaFw0xNDA1MjMyMzIxNTdaMEUxCzAJ\n"
"BgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5l\n"
"dCBXaWRnaXRzIFB0eSBMdGQwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATmK2ni\n"
"v2Wfl74vHg2UikzVl2u3qR4NRvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYa\n"
"HPUdfvGULUvPciLBo1AwTjAdBgNVHQ4EFgQUq4TSrKuV8IJOFngHVVdf5CaNgtEw\n"
"HwYDVR0jBBgwFoAUq4TSrKuV8IJOFngHVVdf5CaNgtEwDAYDVR0TBAUwAwEB/zAJ\n"
"BgcqhkjOPQQBA0gAMEUCIQDyoDVeUTo2w4J5m+4nUIWOcAZ0lVfSKXQA9L4Vh13E\n"
"BwIgfB55FGohg/B6dGh5XxSZmmi08cueFV7mHzJSYV51yRQ=\n"
"-----END CERTIFICATE-----\n";
BIO *bio = BIO_new_mem_buf(kCertPEM, strlen(kCertPEM));
CHECK(bio);
X509 *ret = PEM_read_bio_X509(bio, NULL, NULL, NULL);
CHECK(ret);
BIO_free(bio);
return ret;
}
static EVP_PKEY *GetTestKey() {
static const char kKeyPEM[] =
"-----BEGIN PRIVATE KEY-----\n"
"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgBw8IcnrUoEqc3VnJ\n"
"TYlodwi1b8ldMHcO6NHJzgqLtGqhRANCAATmK2niv2Wfl74vHg2UikzVl2u3qR4N\n"
"Rvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLB\n"
"-----END PRIVATE KEY-----\n";
BIO *bio = BIO_new_mem_buf(kKeyPEM, strlen(kKeyPEM));
CHECK(bio);
EVP_PKEY *ret = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL);
CHECK(ret);
BIO_free(bio);
return ret;
}
int main(int argc, char **argv) {
if (argc != 2) {
fprintf(stderr,
"Usage: %s [key_update_requested|key_update_not_requested]\n",
argv[0]);
return 1;
}
int key_update_type;
if (strcmp(argv[1], "key_update_requested") == 0) {
key_update_type = SSL_KEY_UPDATE_REQUESTED;
} else if (strcmp(argv[1], "key_update_not_requested") == 0) {
key_update_type = SSL_KEY_UPDATE_NOT_REQUESTED;
} else {
fprintf(stderr,
"Usage: %s [key_update_requested|key_update_not_requested]\n",
argv[0]);
return 1;
}
SSL_CTX *ctx = SSL_CTX_new(TLS_method());
CHECK(ctx);
CHECK(SSL_CTX_set_min_proto_version(ctx, TLS1_3_VERSION));
CHECK(SSL_CTX_set_max_proto_version(ctx, TLS1_3_VERSION));
X509 *cert = GetTestCertificate();
CHECK(SSL_CTX_use_certificate(ctx, cert));
X509_free(cert);
EVP_PKEY *key = GetTestKey();
CHECK(SSL_CTX_use_PrivateKey(ctx, key));
EVP_PKEY_free(key);
SSL *client = SSL_new(ctx);
CHECK(client);
SSL_set_connect_state(client);
SSL *server = SSL_new(ctx);
CHECK(server);
SSL_set_accept_state(server);
BIO *bio1, *bio2;
CHECK(BIO_new_bio_pair(&bio1, 1024, &bio2, 1024));
SSL_set_bio(client, bio1, bio1);
SSL_set_bio(server, bio2, bio2);
// Drive both handshakes to completion.
for (;;) {
int client_ret = SSL_do_handshake(client);
int client_err = SSL_get_error(client, client_ret);
CHECK(client_err == SSL_ERROR_NONE ||
client_err == SSL_ERROR_WANT_READ ||
client_err == SSL_ERROR_WANT_WRITE);
int server_ret = SSL_do_handshake(server);
int server_err = SSL_get_error(server, server_ret);
CHECK(server_err == SSL_ERROR_NONE ||
server_err == SSL_ERROR_WANT_READ ||
server_err == SSL_ERROR_WANT_WRITE);
if (client_ret == 1 && server_ret == 1) {
break;
}
}
// Send a KeyUpdate from the client, followed by some data.
CHECK(SSL_key_update(client, key_update_type));
CHECK(SSL_do_handshake(client) == 1);
CHECK(SSL_write(client, "hello", 5) == 5);
// Write a large message on the server. This does not fit in the buffer.
char buf[2048] = {0};
CHECK(SSL_write(server, buf, sizeof(buf)) == -1);
CHECK(SSL_get_error(server, -1) == SSL_ERROR_WANT_WRITE);
// While the server is waiting for the write to complete, it is also driving
// SSL_read. This is common for HTTP/2 servers.
//
// This SSL_read call should pick up the KeyUpdate which then queues up a
// response. It should continue on to the application data record, the ret ==
// 5 case. (No tying up I/O channels for what is meant to be a cheap,
// transparent key rotation.)
int ret = SSL_read(server, buf, sizeof(buf));
int err = SSL_get_error(server, ret);
if (ret == 5) {
CHECK(memcmp(buf, "hello", 5) == 0);
} else {
// Failing that, it would be less ideal but plausible to fail with
// SSL_ERROR_WANT_WRITE, indicating that we tried to write the KeyUpdate
// response but the buffer was still full. What would not be okay is failing
// with a protocol error.
CHECK(ret == -1);
CHECK(err == SSL_ERROR_WANT_WRITE);
}
SSL_free(client);
SSL_free(server);
SSL_CTX_free(ctx);
printf("OK\n");
return 0;
}
$ gcc -Wall key_update_test.c -Iinclude -o key_update_test libssl.a libcrypto.a -pthread -ldl
$ ./key_update_test key_update_not_requested
OK
$ ./key_update_test key_update_requested
Check failed at line 146: err == SSL_ERROR_WANT_WRITE
136271266330368:error:1409E10F:SSL routines:ssl3_write_bytes:bad length:ssl/record/rec_layer_s3.c:368:
Aborted