Initialize the record buffers after the handshake check.
The new V2ClientHello sniff asserts, for safety, that nothing else has
initialized the record layer before it runs. However, OpenSSL allows you to
avoid explicitly calling SSL_connect/SSL_accept and instead let
SSL_read/SSL_write implicitly handshake for you. This check happens at a fairly
low-level in the ssl3_read_bytes function, at which point the record layer has
already been initialized.
Add some tests to ensure this mode works.
(Later we'll lift the handshake check to a higher-level which is probably
simpler.)
Change-Id: Ibeb7fb78e5eb75af5411ba15799248d94f12820b
Reviewed-on: https://boringssl-review.googlesource.com/3334
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 0b15fed..aa43445 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -592,10 +592,6 @@
SSL3_RECORD *rr;
void (*cb)(const SSL *ssl, int type2, int val) = NULL;
- if (s->s3->rbuf.buf == NULL && !ssl3_setup_buffers(s)) {
- return -1;
- }
-
/* XXX: check what the second '&& type' is about */
if ((type && (type != SSL3_RT_APPLICATION_DATA) &&
(type != SSL3_RT_HANDSHAKE) && type) ||
@@ -616,6 +612,11 @@
}
}
+ if (s->s3->rbuf.buf == NULL && !ssl3_setup_buffers(s)) {
+ /* TODO(davidben): Is this redundant with the calls in the handshake? */
+ return -1;
+ }
+
start:
s->rwstate = SSL_NOTHING;
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index b22b005..90349e8 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -743,10 +743,6 @@
void (*cb)(const SSL *ssl, int type2, int val) = NULL;
uint8_t alert_buffer[2];
- if (s->s3->rbuf.buf == NULL && !ssl3_setup_read_buffer(s)) {
- return -1;
- }
-
if ((type && type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE) ||
(peek && type != SSL3_RT_APPLICATION_DATA)) {
OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, ERR_R_INTERNAL_ERROR);
@@ -788,6 +784,11 @@
}
}
+ if (s->s3->rbuf.buf == NULL && !ssl3_setup_read_buffer(s)) {
+ /* TODO(davidben): Is this redundant with the calls in the handshake? */
+ return -1;
+ }
+
start:
s->rwstate = SSL_NOTHING;
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index bfe6e36..90e6bf9 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -503,125 +503,138 @@
}
int ret;
- do {
+ if (config->implicit_handshake) {
if (config->is_server) {
- ret = SSL_accept(ssl);
+ SSL_set_accept_state(ssl);
} else {
- ret = SSL_connect(ssl);
+ SSL_set_connect_state(ssl);
}
- } while (config->async && retry_async(ssl, ret, bio, &clock_delta));
- if (ret != 1) {
- SSL_free(ssl);
- BIO_print_errors_fp(stdout);
- return 2;
- }
+ } else {
+ do {
+ if (config->is_server) {
+ ret = SSL_accept(ssl);
+ } else {
+ ret = SSL_connect(ssl);
+ }
+ } while (config->async && retry_async(ssl, ret, bio, &clock_delta));
+ if (ret != 1) {
+ SSL_free(ssl);
+ BIO_print_errors_fp(stdout);
+ return 2;
+ }
- if (is_resume && (!!SSL_session_reused(ssl) == config->expect_session_miss)) {
- fprintf(stderr, "session was%s reused\n",
- SSL_session_reused(ssl) ? "" : " not");
- return 2;
- }
+ if (is_resume &&
+ (!!SSL_session_reused(ssl) == config->expect_session_miss)) {
+ fprintf(stderr, "session was%s reused\n",
+ SSL_session_reused(ssl) ? "" : " not");
+ return 2;
+ }
- if (!config->expected_server_name.empty()) {
- const char *server_name =
+ if (!config->expected_server_name.empty()) {
+ const char *server_name =
SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
- if (server_name != config->expected_server_name) {
- fprintf(stderr, "servername mismatch (got %s; want %s)\n",
- server_name, config->expected_server_name.c_str());
- return 2;
+ if (server_name != config->expected_server_name) {
+ fprintf(stderr, "servername mismatch (got %s; want %s)\n",
+ server_name, config->expected_server_name.c_str());
+ return 2;
+ }
+
+ if (!early_callback_called) {
+ fprintf(stderr, "early callback not called\n");
+ return 2;
+ }
}
- if (!early_callback_called) {
- fprintf(stderr, "early callback not called\n");
- return 2;
- }
- }
-
- if (!config->expected_certificate_types.empty()) {
- uint8_t *certificate_types;
- int num_certificate_types =
+ if (!config->expected_certificate_types.empty()) {
+ uint8_t *certificate_types;
+ int num_certificate_types =
SSL_get0_certificate_types(ssl, &certificate_types);
- if (num_certificate_types !=
- (int)config->expected_certificate_types.size() ||
- memcmp(certificate_types,
- config->expected_certificate_types.data(),
- num_certificate_types) != 0) {
- fprintf(stderr, "certificate types mismatch\n");
- return 2;
+ if (num_certificate_types !=
+ (int)config->expected_certificate_types.size() ||
+ memcmp(certificate_types,
+ config->expected_certificate_types.data(),
+ num_certificate_types) != 0) {
+ fprintf(stderr, "certificate types mismatch\n");
+ return 2;
+ }
}
- }
- if (!config->expected_next_proto.empty()) {
- const uint8_t *next_proto;
- unsigned next_proto_len;
- SSL_get0_next_proto_negotiated(ssl, &next_proto, &next_proto_len);
- if (next_proto_len != config->expected_next_proto.size() ||
- memcmp(next_proto, config->expected_next_proto.data(),
- next_proto_len) != 0) {
- fprintf(stderr, "negotiated next proto mismatch\n");
- return 2;
+ if (!config->expected_next_proto.empty()) {
+ const uint8_t *next_proto;
+ unsigned next_proto_len;
+ SSL_get0_next_proto_negotiated(ssl, &next_proto, &next_proto_len);
+ if (next_proto_len != config->expected_next_proto.size() ||
+ memcmp(next_proto, config->expected_next_proto.data(),
+ next_proto_len) != 0) {
+ fprintf(stderr, "negotiated next proto mismatch\n");
+ return 2;
+ }
}
- }
- if (!config->expected_alpn.empty()) {
- const uint8_t *alpn_proto;
- unsigned alpn_proto_len;
- SSL_get0_alpn_selected(ssl, &alpn_proto, &alpn_proto_len);
- if (alpn_proto_len != config->expected_alpn.size() ||
- memcmp(alpn_proto, config->expected_alpn.data(),
- alpn_proto_len) != 0) {
- fprintf(stderr, "negotiated alpn proto mismatch\n");
- return 2;
+ if (!config->expected_alpn.empty()) {
+ const uint8_t *alpn_proto;
+ unsigned alpn_proto_len;
+ SSL_get0_alpn_selected(ssl, &alpn_proto, &alpn_proto_len);
+ if (alpn_proto_len != config->expected_alpn.size() ||
+ memcmp(alpn_proto, config->expected_alpn.data(),
+ alpn_proto_len) != 0) {
+ fprintf(stderr, "negotiated alpn proto mismatch\n");
+ return 2;
+ }
}
- }
- if (!config->expected_channel_id.empty()) {
- uint8_t channel_id[64];
- if (!SSL_get_tls_channel_id(ssl, channel_id, sizeof(channel_id))) {
- fprintf(stderr, "no channel id negotiated\n");
- return 2;
+ if (!config->expected_channel_id.empty()) {
+ uint8_t channel_id[64];
+ if (!SSL_get_tls_channel_id(ssl, channel_id, sizeof(channel_id))) {
+ fprintf(stderr, "no channel id negotiated\n");
+ return 2;
+ }
+ if (config->expected_channel_id.size() != 64 ||
+ memcmp(config->expected_channel_id.data(),
+ channel_id, 64) != 0) {
+ fprintf(stderr, "channel id mismatch\n");
+ return 2;
+ }
}
- if (config->expected_channel_id.size() != 64 ||
- memcmp(config->expected_channel_id.data(),
- channel_id, 64) != 0) {
- fprintf(stderr, "channel id mismatch\n");
- return 2;
- }
- }
- if (config->expect_extended_master_secret) {
- if (!ssl->session->extended_master_secret) {
- fprintf(stderr, "No EMS for session when expected");
- return 2;
+ if (config->expect_extended_master_secret) {
+ if (!ssl->session->extended_master_secret) {
+ fprintf(stderr, "No EMS for session when expected");
+ return 2;
+ }
}
- }
- if (!config->expected_ocsp_response.empty()) {
- const uint8_t *data;
- size_t len;
- SSL_get0_ocsp_response(ssl, &data, &len);
- if (config->expected_ocsp_response.size() != len ||
- memcmp(config->expected_ocsp_response.data(), data, len) != 0) {
- fprintf(stderr, "OCSP response mismatch\n");
- return 2;
+ if (!config->expected_ocsp_response.empty()) {
+ const uint8_t *data;
+ size_t len;
+ SSL_get0_ocsp_response(ssl, &data, &len);
+ if (config->expected_ocsp_response.size() != len ||
+ memcmp(config->expected_ocsp_response.data(), data, len) != 0) {
+ fprintf(stderr, "OCSP response mismatch\n");
+ return 2;
+ }
}
- }
- if (!config->expected_signed_cert_timestamps.empty()) {
- const uint8_t *data;
- size_t len;
- SSL_get0_signed_cert_timestamp_list(ssl, &data, &len);
- if (config->expected_signed_cert_timestamps.size() != len ||
- memcmp(config->expected_signed_cert_timestamps.data(),
- data, len) != 0) {
- fprintf(stderr, "SCT list mismatch\n");
- return 2;
+ if (!config->expected_signed_cert_timestamps.empty()) {
+ const uint8_t *data;
+ size_t len;
+ SSL_get0_signed_cert_timestamp_list(ssl, &data, &len);
+ if (config->expected_signed_cert_timestamps.size() != len ||
+ memcmp(config->expected_signed_cert_timestamps.data(),
+ data, len) != 0) {
+ fprintf(stderr, "SCT list mismatch\n");
+ return 2;
+ }
}
}
if (config->renegotiate) {
if (config->async) {
- fprintf(stderr, "--renegotiate is not supported with --async.\n");
+ fprintf(stderr, "-renegotiate is not supported with -async.\n");
+ return 2;
+ }
+ if (config->implicit_handshake) {
+ fprintf(stderr, "-renegotiate is not supported with -implicit-handshake.\n");
return 2;
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index dd63c5b..19369af 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1563,6 +1563,17 @@
})
testCases = append(testCases, testCase{
protocol: protocol,
+ name: "Basic-Client-Implicit" + suffix,
+ config: Config{
+ Bugs: ProtocolBugs{
+ MaxHandshakeRecordLength: maxHandshakeRecordLength,
+ },
+ },
+ flags: append(flags, "-implicit-handshake"),
+ resumeSession: true,
+ })
+ testCases = append(testCases, testCase{
+ protocol: protocol,
testType: serverTest,
name: "Basic-Server" + suffix,
config: Config{
@@ -1586,6 +1597,18 @@
flags: flags,
resumeSession: true,
})
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ testType: serverTest,
+ name: "Basic-Server-Implicit" + suffix,
+ config: Config{
+ Bugs: ProtocolBugs{
+ MaxHandshakeRecordLength: maxHandshakeRecordLength,
+ },
+ },
+ flags: append(flags, "-implicit-handshake"),
+ resumeSession: true,
+ })
// TLS client auth.
testCases = append(testCases, testCase{
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index c032d96..5d4b787 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -73,6 +73,7 @@
{ "-enable-signed-cert-timestamps",
&TestConfig::enable_signed_cert_timestamps },
{ "-fastradio-padding", &TestConfig::fastradio_padding },
+ { "-implicit-handshake", &TestConfig::implicit_handshake },
};
const Flag<std::string> kStringFlags[] = {
@@ -136,7 +137,8 @@
fastradio_padding(false),
min_version(0),
max_version(0),
- mtu(0) {
+ mtu(0),
+ implicit_handshake(false) {
}
bool ParseConfig(int argc, char **argv, TestConfig *out_config) {
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index ba54227..73ea08c 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -67,6 +67,7 @@
int min_version;
int max_version;
int mtu;
+ bool implicit_handshake;
};
bool ParseConfig(int argc, char **argv, TestConfig *out_config);
