From db0f35dda18403accabe98e7780f3dfc516f49de Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 10 May 2017 16:46:14 -0400 Subject: [PATCH] Fix #2400 Add NO_RENEGOTIATE option Reviewed-by: Tim Hudson Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/3432) --- apps/apps.h | 5 +- apps/s_server.c | 3 + doc/man3/SSL_CONF_cmd.pod | 10 +++ doc/man3/SSL_CTX_set_options.pod | 5 ++ include/openssl/ssl.h | 4 + ssl/record/rec_layer_s3.c | 6 +- ssl/ssl_conf.c | 4 + ssl/ssl_err.c | 2 + ssl/ssl_lib.c | 18 ++-- ssl/statem/extensions_srvr.c | 1 + ssl/statem/statem_clnt.c | 5 ++ ssl/statem/statem_lib.c | 4 + ssl/statem/statem_srvr.c | 4 + test/ssl-tests/17-renegotiate.conf | 118 +++++++++++++++++++++++++- test/ssl-tests/17-renegotiate.conf.in | 58 +++++++++++++ 15 files changed, 238 insertions(+), 9 deletions(-) diff --git a/apps/apps.h b/apps/apps.h index 3c1da48ff8..31cf7b0961 100644 --- a/apps/apps.h +++ b/apps/apps.h @@ -215,7 +215,7 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate, OPT_S_ONRESUMP, OPT_S_NOLEGACYCONN, OPT_S_STRICT, OPT_S_SIGALGS, \ OPT_S_CLIENTSIGALGS, OPT_S_GROUPS, OPT_S_CURVES, OPT_S_NAMEDCURVE, \ OPT_S_CIPHER, OPT_S_DHPARAM, OPT_S_RECORD_PADDING, OPT_S_DEBUGBROKE, \ - OPT_S_COMP, OPT_S__LAST + OPT_S_COMP, OPT_S_NO_RENEGOTIATION, OPT_S__LAST # define OPT_S_OPTIONS \ {"no_ssl3", OPT_S_NOSSL3, '-',"Just disable SSLv3" }, \ @@ -231,6 +231,8 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate, {"serverpref", OPT_S_SERVERPREF, '-', "Use server's cipher preferences"}, \ {"legacy_renegotiation", OPT_S_LEGACYRENEG, '-', \ "Enable use of legacy renegotiation (dangerous)"}, \ + {"no_renegotiation", OPT_S_NO_RENEGOTIATION, '-', \ + "Disable all renegotiation."}, \ {"legacy_server_connect", OPT_S_LEGACYCONN, '-', \ "Allow initial connection to servers that don't support RI"}, \ {"no_resumption_on_reneg", OPT_S_ONRESUMP, '-', \ @@ -284,6 +286,7 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate, case OPT_S_CIPHER: \ case OPT_S_DHPARAM: \ case OPT_S_RECORD_PADDING: \ + case OPT_S_NO_RENEGOTIATION: \ case OPT_S_DEBUGBROKE #define IS_NO_PROT_FLAG(o) \ diff --git a/apps/s_server.c b/apps/s_server.c index 82fe5a4b74..d678e8e257 100644 --- a/apps/s_server.c +++ b/apps/s_server.c @@ -2785,6 +2785,9 @@ static void print_connection_info(SSL *con) BIO_printf(bio_s_out, "Reused session-id\n"); BIO_printf(bio_s_out, "Secure Renegotiation IS%s supported\n", SSL_get_secure_renegotiation_support(con) ? "" : " NOT"); + if ((SSL_get_options(con) & SSL_OP_NO_RENEGOTIATION)) + BIO_printf(bio_s_out, "Renegotiation is DISABLED\n"); + if (keymatexportlabel != NULL) { BIO_printf(bio_s_out, "Keying material exporter:\n"); BIO_printf(bio_s_out, " Label: '%s'\n", keymatexportlabel); diff --git a/doc/man3/SSL_CONF_cmd.pod b/doc/man3/SSL_CONF_cmd.pod index 18cc88f59b..7b751fdef3 100644 --- a/doc/man3/SSL_CONF_cmd.pod +++ b/doc/man3/SSL_CONF_cmd.pod @@ -125,6 +125,11 @@ Attempts to pad TLS 1.3 records so that they are a multiple of B in length on send. A B of 0 or 1 turns off padding. Otherwise, the B must be >1 or <=16384. +=item B<-no_renegotiation> + +Disables all attempts at renegotiation in TLSv1.2 and earlier, same as setting +B. + =item B<-min_protocol>, B<-max_protocol> Sets the minimum and maximum supported protocol. @@ -257,6 +262,11 @@ Attempts to pad TLS 1.3 records so that they are a multiple of B in length on send. A B of 0 or 1 turns off padding. Otherwise, the B must be >1 or <=16384. +=item B + +Disables all attempts at renegotiation in TLSv1.2 and earlier, same as setting +B. + =item B This sets the supported signature algorithms for TLS v1.2. For clients this diff --git a/doc/man3/SSL_CTX_set_options.pod b/doc/man3/SSL_CTX_set_options.pod index d12a0399c0..5155a1f679 100644 --- a/doc/man3/SSL_CTX_set_options.pod +++ b/doc/man3/SSL_CTX_set_options.pod @@ -170,6 +170,11 @@ RFC7366 Encrypt-then-MAC option on TLS and DTLS connection. If this option is set, Encrypt-then-MAC is disabled. Clients will not propose, and servers will not accept the extension. +=item SSL_OP_NO_RENEGOTIATION + +Disable all renegotiation in TLSv1.2 and earlier. Do not send HelloRequest +messages, and ignore renegotiation requests via ClientHello. + =back The following options no longer have any effect but their identifiers are diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 8eb3c5389f..3b0ac5021c 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -370,6 +370,9 @@ typedef int (*SSL_verify_cb)(int preverify_ok, X509_STORE_CTX *x509_ctx); SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_1|SSL_OP_NO_TLSv1_2|SSL_OP_NO_TLSv1_3) # define SSL_OP_NO_DTLS_MASK (SSL_OP_NO_DTLSv1|SSL_OP_NO_DTLSv1_2) +/* Disallow all renegotiation */ +# define SSL_OP_NO_RENEGOTIATION 0x40000000U + /* * Make server add server-hello extension from early version of cryptopro * draft, when GOST ciphersuite is negotiated. Required for interoperability @@ -2386,6 +2389,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_SSL_READ_EX 434 # define SSL_F_SSL_READ_INTERNAL 523 # define SSL_F_SSL_RENEGOTIATE 516 +# define SSL_F_SSL_RENEGOTIATE_ABBREVIATED 546 # define SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT 320 # define SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT 321 # define SSL_F_SSL_SESSION_DUP 348 diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 01caf4c372..045a74c113 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1433,13 +1433,15 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, */ if (s->server && SSL_is_init_finished(s) && - !s->s3->send_connection_binding && (s->version > SSL3_VERSION) && !SSL_IS_TLS13(s) && + (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) && (s->rlayer.handshake_fragment_len >= 4) && (s->rlayer.handshake_fragment[0] == SSL3_MT_CLIENT_HELLO) && (s->session != NULL) && (s->session->cipher != NULL) && - !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) { + ((!s->s3->send_connection_binding && + !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) || + (s->options & SSL_OP_NO_RENEGOTIATION))) { SSL3_RECORD_set_length(rr, 0); SSL3_RECORD_set_read(rr); ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION); diff --git a/ssl/ssl_conf.c b/ssl/ssl_conf.c index 484bb61feb..41c7ff7d83 100644 --- a/ssl/ssl_conf.c +++ b/ssl/ssl_conf.c @@ -358,6 +358,7 @@ static int cmd_Options(SSL_CONF_CTX *cctx, const char *value) SSL_FLAG_TBL("UnsafeLegacyRenegotiation", SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION), SSL_FLAG_TBL_INV("EncryptThenMac", SSL_OP_NO_ENCRYPT_THEN_MAC), + SSL_FLAG_TBL("NoRenegotiation", SSL_OP_NO_RENEGOTIATION), }; if (value == NULL) return -3; @@ -573,6 +574,7 @@ static const ssl_conf_cmd_tbl ssl_conf_cmds[] = { SSL_CONF_CMD_SWITCH("serverpref", SSL_CONF_FLAG_SERVER), SSL_CONF_CMD_SWITCH("legacy_renegotiation", 0), SSL_CONF_CMD_SWITCH("legacy_server_connect", SSL_CONF_FLAG_SERVER), + SSL_CONF_CMD_SWITCH("no_renegotiation", 0), SSL_CONF_CMD_SWITCH("no_resumption_on_reneg", SSL_CONF_FLAG_SERVER), SSL_CONF_CMD_SWITCH("no_legacy_server_connect", SSL_CONF_FLAG_SERVER), SSL_CONF_CMD_SWITCH("strict", 0), @@ -639,6 +641,8 @@ static const ssl_switch_tbl ssl_cmd_switches[] = { {SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION, 0}, /* legacy_server_connect */ {SSL_OP_LEGACY_SERVER_CONNECT, 0}, + /* no_renegotiation */ + {SSL_OP_NO_RENEGOTIATION, 0}, /* no_resumption_on_reneg */ {SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION, 0}, /* no_legacy_server_connect */ diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index e334b00370..7152fa2701 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -216,6 +216,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { {ERR_FUNC(SSL_F_SSL_READ_EX), "SSL_read_ex"}, {ERR_FUNC(SSL_F_SSL_READ_INTERNAL), "ssl_read_internal"}, {ERR_FUNC(SSL_F_SSL_RENEGOTIATE), "SSL_renegotiate"}, + {ERR_FUNC(SSL_F_SSL_RENEGOTIATE_ABBREVIATED), + "SSL_renegotiate_abbreviated"}, {ERR_FUNC(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT), "ssl_scan_clienthello_tlsext"}, {ERR_FUNC(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT), diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 028b69da08..e90c4b8732 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -1922,9 +1922,12 @@ int SSL_renegotiate(SSL *s) return 0; } - if (s->renegotiate == 0) - s->renegotiate = 1; + if ((s->options & SSL_OP_NO_RENEGOTIATION)) { + SSLerr(SSL_F_SSL_RENEGOTIATE, SSL_R_NO_RENEGOTIATION); + return 0; + } + s->renegotiate = 1; s->new_session = 1; return (s->method->ssl_renegotiate(s)); @@ -1932,12 +1935,17 @@ int SSL_renegotiate(SSL *s) int SSL_renegotiate_abbreviated(SSL *s) { - if (SSL_IS_TLS13(s)) + if (SSL_IS_TLS13(s)) { + SSLerr(SSL_F_SSL_RENEGOTIATE_ABBREVIATED, SSL_R_WRONG_SSL_VERSION); return 0; + } - if (s->renegotiate == 0) - s->renegotiate = 1; + if ((s->options & SSL_OP_NO_RENEGOTIATION)) { + SSLerr(SSL_F_SSL_RENEGOTIATE_ABBREVIATED, SSL_R_NO_RENEGOTIATION); + return 0; + } + s->renegotiate = 1; s->new_session = 0; return (s->method->ssl_renegotiate(s)); diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 95bacdffc4..fe181abad0 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -817,6 +817,7 @@ EXT_RETURN tls_construct_stoc_renegotiate(SSL *s, WPACKET *pkt, if (!s->s3->send_connection_binding) return EXT_RETURN_NOT_SENT; + /* Still add this even if SSL_OP_NO_RENEGOTIATION is set */ if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_renegotiate) || !WPACKET_start_sub_packet_u16(pkt) || !WPACKET_start_sub_packet_u8(pkt) diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index d4382e89cc..020589f1b1 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -3454,6 +3454,11 @@ MSG_PROCESS_RETURN tls_process_hello_req(SSL *s, PACKET *pkt) return MSG_PROCESS_ERROR; } + if ((s->options & SSL_OP_NO_RENEGOTIATION)) { + ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION); + return MSG_PROCESS_FINISHED_READING; + } + /* * This is a historical discrepancy (not in the RFC) maintained for * compatibility reasons. If a TLS client receives a HelloRequest it will diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index c2b14853c2..29fb9dcf1f 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -116,6 +116,10 @@ int tls_setup_handshake(SSL *s) } if (SSL_IS_FIRST_HANDSHAKE(s)) { s->ctx->stats.sess_accept++; + } else if ((s->options & SSL_OP_NO_RENEGOTIATION)) { + /* Renegotiation is disabled */ + ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION); + return 0; } else if (!s->s3->send_connection_binding && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) { diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 02c6e569d8..c26c93bdc9 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1246,6 +1246,10 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) } /* Check if this is actually an unexpected renegotiation ClientHello */ if (s->renegotiate == 0 && !SSL_IS_FIRST_HANDSHAKE(s)) { + if ((s->options & SSL_OP_NO_RENEGOTIATION)) { + ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION); + goto err; + } s->renegotiate = 1; s->new_session = 1; } diff --git a/test/ssl-tests/17-renegotiate.conf b/test/ssl-tests/17-renegotiate.conf index 3f3769ff02..12cf791310 100644 --- a/test/ssl-tests/17-renegotiate.conf +++ b/test/ssl-tests/17-renegotiate.conf @@ -1,6 +1,6 @@ # Generated with generate_ssl_tests.pl -num_tests = 10 +num_tests = 14 test-0 = 0-renegotiate-client-no-resume test-1 = 1-renegotiate-client-resume @@ -12,6 +12,10 @@ test-6 = 6-renegotiate-aead-to-non-aead test-7 = 7-renegotiate-non-aead-to-aead test-8 = 8-renegotiate-non-aead-to-non-aead test-9 = 9-renegotiate-aead-to-aead +test-10 = 10-no-renegotiation-server-by-client +test-11 = 11-no-renegotiation-server-by-server +test-12 = 12-no-renegotiation-client-by-server +test-13 = 13-no-renegotiation-client-by-client # =========================================================== [0-renegotiate-client-no-resume] @@ -314,3 +318,115 @@ client = 9-renegotiate-aead-to-aead-client-extra RenegotiateCiphers = AES256-GCM-SHA384 +# =========================================================== + +[10-no-renegotiation-server-by-client] +ssl_conf = 10-no-renegotiation-server-by-client-ssl + +[10-no-renegotiation-server-by-client-ssl] +server = 10-no-renegotiation-server-by-client-server +client = 10-no-renegotiation-server-by-client-client + +[10-no-renegotiation-server-by-client-server] +Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem +CipherString = DEFAULT +MaxProtocol = TLSv1.2 +Options = NoRenegotiation +PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem + +[10-no-renegotiation-server-by-client-client] +CipherString = DEFAULT +VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem +VerifyMode = Peer + +[test-10] +ExpectedResult = ClientFail +HandshakeMode = RenegotiateClient +Method = TLS +ResumptionExpected = No + + +# =========================================================== + +[11-no-renegotiation-server-by-server] +ssl_conf = 11-no-renegotiation-server-by-server-ssl + +[11-no-renegotiation-server-by-server-ssl] +server = 11-no-renegotiation-server-by-server-server +client = 11-no-renegotiation-server-by-server-client + +[11-no-renegotiation-server-by-server-server] +Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem +CipherString = DEFAULT +MaxProtocol = TLSv1.2 +Options = NoRenegotiation +PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem + +[11-no-renegotiation-server-by-server-client] +CipherString = DEFAULT +VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem +VerifyMode = Peer + +[test-11] +ExpectedResult = ServerFail +HandshakeMode = RenegotiateServer +Method = TLS +ResumptionExpected = No + + +# =========================================================== + +[12-no-renegotiation-client-by-server] +ssl_conf = 12-no-renegotiation-client-by-server-ssl + +[12-no-renegotiation-client-by-server-ssl] +server = 12-no-renegotiation-client-by-server-server +client = 12-no-renegotiation-client-by-server-client + +[12-no-renegotiation-client-by-server-server] +Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem +CipherString = DEFAULT +MaxProtocol = TLSv1.2 +PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem + +[12-no-renegotiation-client-by-server-client] +CipherString = DEFAULT +Options = NoRenegotiation +VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem +VerifyMode = Peer + +[test-12] +ExpectedResult = ServerFail +HandshakeMode = RenegotiateServer +Method = TLS +ResumptionExpected = No + + +# =========================================================== + +[13-no-renegotiation-client-by-client] +ssl_conf = 13-no-renegotiation-client-by-client-ssl + +[13-no-renegotiation-client-by-client-ssl] +server = 13-no-renegotiation-client-by-client-server +client = 13-no-renegotiation-client-by-client-client + +[13-no-renegotiation-client-by-client-server] +Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem +CipherString = DEFAULT +MaxProtocol = TLSv1.2 +PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem + +[13-no-renegotiation-client-by-client-client] +CipherString = DEFAULT +Options = NoRenegotiation +VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem +VerifyMode = Peer + +[test-13] +ExpectedResult = ClientFail +HandshakeMode = RenegotiateClient +Method = TLS +ResumptionExpected = No + + diff --git a/test/ssl-tests/17-renegotiate.conf.in b/test/ssl-tests/17-renegotiate.conf.in index b5d07b0705..35175dce51 100644 --- a/test/ssl-tests/17-renegotiate.conf.in +++ b/test/ssl-tests/17-renegotiate.conf.in @@ -185,6 +185,64 @@ our @tests_tls1_2 = ( "ResumptionExpected" => "No", "ExpectedResult" => "Success" } + }, + { + name => "no-renegotiation-server-by-client", + server => { + "Options" => "NoRenegotiation", + "MaxProtocol" => "TLSv1.2" + }, + client => { }, + test => { + "Method" => "TLS", + "HandshakeMode" => "RenegotiateClient", + "ResumptionExpected" => "No", + "ExpectedResult" => "ClientFail" + } + }, + { + name => "no-renegotiation-server-by-server", + server => { + "Options" => "NoRenegotiation", + "MaxProtocol" => "TLSv1.2" + }, + client => { }, + test => { + "Method" => "TLS", + "HandshakeMode" => "RenegotiateServer", + "ResumptionExpected" => "No", + "ExpectedResult" => "ServerFail" + } + }, + { + name => "no-renegotiation-client-by-server", + server => { + "MaxProtocol" => "TLSv1.2" + }, + client => { + "Options" => "NoRenegotiation", + }, + test => { + "Method" => "TLS", + "HandshakeMode" => "RenegotiateServer", + "ResumptionExpected" => "No", + "ExpectedResult" => "ServerFail" + } + }, + { + name => "no-renegotiation-client-by-client", + server => { + "MaxProtocol" => "TLSv1.2" + }, + client => { + "Options" => "NoRenegotiation", + }, + test => { + "Method" => "TLS", + "HandshakeMode" => "RenegotiateClient", + "ResumptionExpected" => "No", + "ExpectedResult" => "ClientFail" + } } ); -- 2.34.1