From 8df8e64205d0f8334ee4d403306bfcd4287004b0 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Wed, 28 Feb 2018 13:35:39 -0800 Subject: [PATCH] Record whether dummy PQ padding was used. On reflection, I think we'll need to note whether dummy PQ padding was echoed on a given connection. Otherwise measurements in Chrome will be mixed with cases where people have MITM proxies that ignored the extension, or possibly Google frontends that haven't been updated. Therefore this change will be used to filter latency measurements in Chrome to only include those where the extension was echoed and we'll measure at levels of 1 byte (for control), 400 bytes, and 1100 bytes. This also makes it an error if the server didn't echo an extension of the same length as was sent. Change-Id: Ib2a0b29cfb8719a75a28f3cf96710c57d88eaa68 Reviewed-on: https://boringssl-review.googlesource.com/26284 Commit-Queue: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: David Benjamin --- include/openssl/ssl.h | 5 +++++ ssl/internal.h | 10 ++++++++-- ssl/ssl_lib.cc | 8 ++++++++ ssl/t1_lib.cc | 22 ++++++++++++++++------ ssl/test/bssl_shim.cc | 9 +++++++++ ssl/test/runner/handshake_server.go | 4 ++++ ssl/test/runner/runner.go | 1 + ssl/test/test_config.cc | 1 + ssl/test/test_config.h | 1 + 9 files changed, 53 insertions(+), 8 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 760d68baf..35506f7e3 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2952,6 +2952,11 @@ OPENSSL_EXPORT const char *SSL_get_psk_identity(const SSL *ssl); // phase of the experiment. It returns one for success and zero otherwise. OPENSSL_EXPORT int SSL_set_dummy_pq_padding_size(SSL *ssl, size_t num_bytes); +// SSL_dummy_pq_padding_used returns one if the server echoed a dummy PQ padding +// extension and zero otherwise. It may only be called on a client connection +// once the handshake has completed, otherwise it'll return zero. +OPENSSL_EXPORT int SSL_dummy_pq_padding_used(SSL *ssl); + // QUIC Transport Parameters. // diff --git a/ssl/internal.h b/ssl/internal.h index a2205c9df..d13d5f2c9 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1515,8 +1515,9 @@ struct SSL_HANDSHAKE { // |grease_seeded| is true. uint8_t grease_seed[ssl_grease_last_index + 1] = {0}; - // dummy_pq_padding_len is the length of the extension that should be echoed - // in a ServerHello, or zero if no extension should be echoed. + // dummy_pq_padding_len, in a server, is the length of the extension that + // should be echoed in a ServerHello, or zero if no extension should be + // echoed. uint16_t dummy_pq_padding_len = 0; }; @@ -2674,6 +2675,11 @@ struct SSLConnection { // returns |SSL_HANDOFF|. This is copied in |SSL_new| from the |SSL_CTX| // element of the same name and may be cleared if the handoff is declined. bool handoff:1; + + // did_dummy_pq_padding is only valid for a client. In that context, it is + // true iff the client observed the server echoing a dummy PQ padding + // extension. + bool did_dummy_pq_padding:1; }; // From draft-ietf-tls-tls13-18, used in determining PSK modes. diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index d29cc6f60..9f56d5470 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -2440,6 +2440,14 @@ int SSL_set_dummy_pq_padding_size(SSL *ssl, size_t num_bytes) { return 1; } +int SSL_dummy_pq_padding_used(SSL *ssl) { + if (ssl->server || !ssl->s3->initial_handshake_complete) { + return 0; + } + + return ssl->did_dummy_pq_padding; +} + void SSL_CTX_set_msg_callback(SSL_CTX *ctx, void (*cb)(int write_p, int version, int content_type, const void *buf, diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 4d3c4c7ee..97c0c4b9a 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc @@ -557,11 +557,6 @@ static bool ignore_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert, return true; } -static bool ignore_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert, - CBS *contents) { - return true; -} - static bool dont_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) { return true; } @@ -2355,6 +2350,21 @@ static bool ext_dummy_pq_padding_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) { return ext_dummy_pq_padding_add(out, len); } +static bool ext_dummy_pq_padding_parse_serverhello(SSL_HANDSHAKE *hs, + uint8_t *out_alert, + CBS *contents) { + if (contents == nullptr) { + return true; + } + + if (CBS_len(contents) != hs->ssl->dummy_pq_padding_len) { + return false; + } + + hs->ssl->did_dummy_pq_padding = true; + return true; +} + static bool ext_dummy_pq_padding_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert, CBS *contents) { @@ -2816,7 +2826,7 @@ static const struct tls_extension kExtensions[] = { TLSEXT_TYPE_dummy_pq_padding, NULL, ext_dummy_pq_padding_add_clienthello, - ignore_parse_serverhello, + ext_dummy_pq_padding_parse_serverhello, ext_dummy_pq_padding_parse_clienthello, ext_dummy_pq_padding_add_serverhello, }, diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 5790dc3f0..ae26deda1 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -1862,6 +1862,15 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume, if (config->expect_draft_downgrade != !!SSL_is_draft_downgrade(ssl)) { fprintf(stderr, "Got %sdraft downgrade signal, but wanted the opposite.\n", SSL_is_draft_downgrade(ssl) ? "" : "no "); + return false; + } + + const bool did_dummy_pq_padding = !!SSL_dummy_pq_padding_used(ssl); + if (config->expect_dummy_pq_padding != did_dummy_pq_padding) { + fprintf(stderr, + "Dummy PQ padding %s observed, but expected the opposite.\n", + did_dummy_pq_padding ? "was" : "was not"); + return false; } return true; diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 329cfd6d4..0c2459286 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -1386,6 +1386,10 @@ func (hs *serverHandshakeState) processClientExtensions(serverExtensions *server return errors.New("tls: no GREASE extension found") } + if l := hs.clientHello.dummyPQPaddingLen; l != 0 { + serverExtensions.dummyPQPaddingLen = l + } + serverExtensions.serverNameAck = c.config.Bugs.SendServerNameAck return nil diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index fd30e6322..510a48b25 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -7358,6 +7358,7 @@ func addExtensionTests() { flags: []string{ "-max-version", version.shimFlag(tls), "-dummy-pq-padding-len", strconv.Itoa(paddingLen), + "-expect-dummy-pq-padding", }, }) diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 1125aefdd..f50251d6a 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -132,6 +132,7 @@ const Flag kBoolFlags[] = { &TestConfig::allow_false_start_without_alpn }, { "-expect-draft-downgrade", &TestConfig::expect_draft_downgrade }, { "-handoff", &TestConfig::handoff }, + { "-expect-dummy-pq-padding", &TestConfig::expect_dummy_pq_padding }, }; const Flag kStringFlags[] = { diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 8768654da..fb479d10d 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -153,6 +153,7 @@ struct TestConfig { bool expect_draft_downgrade = false; int dummy_pq_padding_len = 0; bool handoff = false; + bool expect_dummy_pq_padding = false; }; bool ParseConfig(int argc, char **argv, TestConfig *out_initial,