Add SSL_set_renegotiate_mode. Add a slightly richer API. Notably, one can configure ssl_renegotiate_once to only accept the first renego. Also, this API doesn't repeat the mistake I made with SSL_set_reject_peer_renegotiations which is super-confusing with the negation. Change-Id: I7eb5d534e3e6c553b641793f4677fe5a56451c71 Reviewed-on: https://boringssl-review.googlesource.com/6221 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 01bd7cc..dc258ef 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -2368,11 +2368,25 @@ * https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format. */ OPENSSL_EXPORT void SSL_CTX_set_keylog_bio(SSL_CTX *ctx, BIO *keylog_bio); -/* SSL_set_reject_peer_renegotiations controls whether renegotiation attempts by - * the peer are rejected. It may be set at any point in a connection's lifetime - * to control future renegotiations programmatically. By default, renegotiations - * are rejected. (Renegotiations requested by a client are always rejected.) */ -OPENSSL_EXPORT void SSL_set_reject_peer_renegotiations(SSL *ssl, int reject); +enum ssl_renegotiate_mode_t { + ssl_renegotiate_never = 0, + ssl_renegotiate_once, + ssl_renegotiate_freely, +}; + +/* SSL_set_renegotiate_mode configures how |ssl|, a client, reacts to + * renegotiation attempts by a server. If |ssl| is a server, peer-initiated + * renegotiations are *always* rejected and this function does nothing. + * + * The renegotiation mode defaults to |ssl_renegotiate_never|, but may be set + * at any point in a connection's lifetime. Set it to |ssl_renegotiate_once| to + * allow one renegotiation and |ssl_renegotiate_freely| to allow all + * renegotiations. + * + * There is no support in BoringSSL for initiating renegotiations as a client + * or server. */ +OPENSSL_EXPORT void SSL_set_renegotiate_mode(SSL *ssl, + enum ssl_renegotiate_mode_t mode); /* Underdocumented functions. @@ -2720,6 +2734,11 @@ /* Deprecated functions. */ +/* SSL_set_reject_peer_renegotiations calls |SSL_set_renegotiate_mode| with + * |ssl_never_renegotiate| if |reject| is one and |ssl_renegotiate_freely| if + * zero. */ +OPENSSL_EXPORT void SSL_set_reject_peer_renegotiations(SSL *ssl, int reject); + /* SSL_CIPHER_description writes a description of |cipher| into |buf| and * returns |buf|. If |buf| is NULL, it returns a newly allocated string, to be * freed with |OPENSSL_free|, or NULL on error. @@ -3567,9 +3586,8 @@ uint8_t *alpn_client_proto_list; unsigned alpn_client_proto_list_len; - /* accept_peer_renegotiations, if one, accepts renegotiation attempts from the - * peer. Otherwise, they will be rejected with a fatal error. */ - char accept_peer_renegotiations; + /* renegotiate_mode controls how peer renegotiation attempts are handled. */ + enum ssl_renegotiate_mode_t renegotiate_mode; /* These fields are always NULL and exist only to keep wpa_supplicant happy * about the change to EVP_AEAD. They are only needed for EAP-FAST, which we
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index db49601..f2a26a9 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c
@@ -337,6 +337,20 @@ ssl3_read_bytes(ssl, 0, NULL, 0, 0); } +static int ssl3_can_renegotiate(SSL *ssl) { + switch (ssl->renegotiate_mode) { + case ssl_renegotiate_never: + return 0; + case ssl_renegotiate_once: + return ssl->s3->total_renegotiations == 0; + case ssl_renegotiate_freely: + return 1; + } + + assert(0); + return 0; +} + /* Return up to 'len' payload bytes received in 'type' records. * 'type' is one of the following: * @@ -509,7 +523,7 @@ if (rr->type == SSL3_RT_HANDSHAKE) { /* If peer renegotiations are disabled, all out-of-order handshake records * are fatal. Renegotiations as a server are never supported. */ - if (!s->accept_peer_renegotiations || s->server) { + if (s->server || !ssl3_can_renegotiate(s)) { al = SSL_AD_NO_RENEGOTIATION; OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION); goto f_err;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 66867ba..355195e 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c
@@ -2699,8 +2699,13 @@ ctx->dos_protection_cb = cb; } -void SSL_set_reject_peer_renegotiations(SSL *s, int reject) { - s->accept_peer_renegotiations = !reject; +void SSL_set_renegotiate_mode(SSL *ssl, enum ssl_renegotiate_mode_t mode) { + ssl->renegotiate_mode = mode; +} + +void SSL_set_reject_peer_renegotiations(SSL *ssl, int reject) { + SSL_set_renegotiate_mode( + ssl, reject ? ssl_renegotiate_never : ssl_renegotiate_freely); } int SSL_get_rc4_state(const SSL *ssl, const RC4_KEY **read_key,
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 237ebf1..8d1f37b 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc
@@ -1114,9 +1114,11 @@ if (config->install_ddos_callback) { SSL_CTX_set_dos_protection_cb(ssl_ctx, DDoSCallback); } - if (!config->reject_peer_renegotiations) { - /* Renegotiations are disabled by default. */ - SSL_set_reject_peer_renegotiations(ssl.get(), 0); + if (config->renegotiate_once) { + SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_once); + } + if (config->renegotiate_freely) { + SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_freely); } if (!config->check_close_notify) { SSL_set_quiet_shutdown(ssl.get(), 1);
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index ed37b77..189854e 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -206,9 +206,9 @@ // connection immediately after the handshake rather than echoing // messages from the runner. shimShutsDown bool - // renegotiate indicates the the connection should be renegotiated - // during the exchange. - renegotiate bool + // renegotiate indicates the number of times the connection should be + // renegotiated during the exchange. + renegotiate int // renegotiateCiphers is a list of ciphersuite ids that will be // switched in just before renegotiation. renegotiateCiphers []uint16 @@ -403,12 +403,14 @@ tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage) } - if test.renegotiate { + if test.renegotiate > 0 { if test.renegotiateCiphers != nil { config.CipherSuites = test.renegotiateCiphers } - if err := tlsConn.Renegotiate(); err != nil { - return err + for i := 0; i < test.renegotiate; i++ { + if err := tlsConn.Renegotiate(); err != nil { + return err + } } } else if test.renegotiateCiphers != nil { panic("renegotiateCiphers without renegotiate") @@ -2710,8 +2712,11 @@ if protocol == tls { tests = append(tests, testCase{ name: "Renegotiate-Client", - renegotiate: true, - flags: []string{"-expect-total-renegotiations", "1"}, + renegotiate: 1, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, }) // NPN on client and server; results in post-handshake message. tests = append(tests, testCase{ @@ -3675,8 +3680,7 @@ testCases = append(testCases, testCase{ testType: serverTest, name: "Renegotiate-Server-Forbidden", - renegotiate: true, - flags: []string{"-reject-peer-renegotiations"}, + renegotiate: 1, shouldFail: true, expectedError: ":NO_RENEGOTIATION:", expectedLocalError: "remote error: no renegotiation", @@ -3715,28 +3719,33 @@ FailIfResumeOnRenego: true, }, }, - renegotiate: true, - flags: []string{"-expect-total-renegotiations", "1"}, + renegotiate: 1, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, }) testCases = append(testCases, testCase{ name: "Renegotiate-Client-EmptyExt", - renegotiate: true, + renegotiate: 1, config: Config{ Bugs: ProtocolBugs{ EmptyRenegotiationInfo: true, }, }, + flags: []string{"-renegotiate-freely"}, shouldFail: true, expectedError: ":RENEGOTIATION_MISMATCH:", }) testCases = append(testCases, testCase{ name: "Renegotiate-Client-BadExt", - renegotiate: true, + renegotiate: 1, config: Config{ Bugs: ProtocolBugs{ BadRenegotiationInfo: true, }, }, + flags: []string{"-renegotiate-freely"}, shouldFail: true, expectedError: ":RENEGOTIATION_MISMATCH:", }) @@ -3753,54 +3762,58 @@ }) testCases = append(testCases, testCase{ name: "Renegotiate-Client-NoExt-Allowed", - renegotiate: true, + renegotiate: 1, config: Config{ Bugs: ProtocolBugs{ NoRenegotiationInfo: true, }, }, - flags: []string{"-expect-total-renegotiations", "1"}, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, }) testCases = append(testCases, testCase{ name: "Renegotiate-Client-SwitchCiphers", - renegotiate: true, + renegotiate: 1, config: Config{ CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA}, }, renegotiateCiphers: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, - flags: []string{"-expect-total-renegotiations", "1"}, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, }) testCases = append(testCases, testCase{ name: "Renegotiate-Client-SwitchCiphers2", - renegotiate: true, + renegotiate: 1, config: Config{ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, }, renegotiateCiphers: []uint16{TLS_RSA_WITH_RC4_128_SHA}, - flags: []string{"-expect-total-renegotiations", "1"}, - }) - testCases = append(testCases, testCase{ - name: "Renegotiate-Client-Forbidden", - renegotiate: true, - flags: []string{"-reject-peer-renegotiations"}, - shouldFail: true, - expectedError: ":NO_RENEGOTIATION:", - expectedLocalError: "remote error: no renegotiation", + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, }) testCases = append(testCases, testCase{ name: "Renegotiate-SameClientVersion", - renegotiate: true, + renegotiate: 1, config: Config{ MaxVersion: VersionTLS10, Bugs: ProtocolBugs{ RequireSameRenegoClientVersion: true, }, }, - flags: []string{"-expect-total-renegotiations", "1"}, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, }) testCases = append(testCases, testCase{ name: "Renegotiate-FalseStart", - renegotiate: true, + renegotiate: 1, config: Config{ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, NextProtos: []string{"foo"}, @@ -3808,10 +3821,52 @@ flags: []string{ "-false-start", "-select-next-proto", "foo", + "-renegotiate-freely", "-expect-total-renegotiations", "1", }, shimWritesFirst: true, }) + + // Client-side renegotiation controls. + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-Forbidden-1", + renegotiate: 1, + shouldFail: true, + expectedError: ":NO_RENEGOTIATION:", + expectedLocalError: "remote error: no renegotiation", + }) + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-Once-1", + renegotiate: 1, + flags: []string{ + "-renegotiate-once", + "-expect-total-renegotiations", "1", + }, + }) + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-Freely-1", + renegotiate: 1, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "1", + }, + }) + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-Once-2", + renegotiate: 2, + flags: []string{"-renegotiate-once"}, + shouldFail: true, + expectedError: ":NO_RENEGOTIATION:", + expectedLocalError: "remote error: no renegotiation", + }) + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-Freely-2", + renegotiate: 2, + flags: []string{ + "-renegotiate-freely", + "-expect-total-renegotiations", "2", + }, + }) } func addDTLSReplayTests() {
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 6503494..4e19be6 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -76,7 +76,6 @@ { "-fail-second-ddos-callback", &TestConfig::fail_second_ddos_callback }, { "-handshake-never-done", &TestConfig::handshake_never_done }, { "-use-export-context", &TestConfig::use_export_context }, - { "-reject-peer-renegotiations", &TestConfig::reject_peer_renegotiations }, { "-no-legacy-server-connect", &TestConfig::no_legacy_server_connect }, { "-tls-unique", &TestConfig::tls_unique }, { "-use-async-private-key", &TestConfig::use_async_private_key }, @@ -95,7 +94,9 @@ { "-microsoft-big-sslv3-buffer", &TestConfig::microsoft_big_sslv3_buffer }, { "-verify-fail", &TestConfig::verify_fail }, { "-verify-peer", &TestConfig::verify_peer }, - { "-expect-verify-result", &TestConfig::expect_verify_result } + { "-expect-verify-result", &TestConfig::expect_verify_result }, + { "-renegotiate-once", &TestConfig::renegotiate_once }, + { "-renegotiate-freely", &TestConfig::renegotiate_freely }, }; const Flag<std::string> kStringFlags[] = {
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index ea05271..4601851 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -77,7 +77,6 @@ std::string export_label; std::string export_context; bool use_export_context = false; - bool reject_peer_renegotiations = false; bool no_legacy_server_connect = false; bool tls_unique = false; bool use_async_private_key = false; @@ -98,6 +97,8 @@ bool expect_verify_result = false; std::string signed_cert_timestamps; int expect_total_renegotiations = 0; + bool renegotiate_once = false; + bool renegotiate_freely = false; }; bool ParseConfig(int argc, char **argv, TestConfig *out_config);