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);