diff options
author | David Benjamin <davidben@chromium.org> | 2014-07-12 21:27:45 +0400 |
---|---|---|
committer | Adam Langley <agl@google.com> | 2014-07-15 01:42:30 +0400 |
commit | 9c651c9ef6d0ec8a4fa953efcdd85e3130015fe6 (patch) | |
tree | ef2e2f5384ab1f3bb1bcea7d92712f59c5082e62 | |
parent | ced551f9673426a7287d958aa1f3967f24107da9 (diff) |
Be strict about requiring ServerKeyExchange.
Missing ServerKeyExchange is handled, but only because it hits an
ERR_R_INTERNAL_ERROR in ssl3_send_client_key_exchange in trying to find the
server ECDH parameters. Be strict about requiring it for ECDHE.
Change-Id: Ifce5b73c8bd14746b8a2185f479d550e9e3f84df
Reviewed-on: https://boringssl-review.googlesource.com/1157
Reviewed-by: Adam Langley <agl@google.com>
-rw-r--r-- | ssl/s3_clnt.c | 7 | ||||
-rw-r--r-- | ssl/ssl_ciph.c | 25 | ||||
-rw-r--r-- | ssl/ssl_locl.h | 1 | ||||
-rw-r--r-- | ssl/test/runner/common.go | 4 | ||||
-rw-r--r-- | ssl/test/runner/handshake_server.go | 2 | ||||
-rw-r--r-- | ssl/test/runner/runner.go | 11 |
6 files changed, 49 insertions, 1 deletions
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 6c10a32b..a28fd33d 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1286,6 +1286,13 @@ int ssl3_get_key_exchange(SSL *s) if (s->s3->tmp.message_type != SSL3_MT_SERVER_KEY_EXCHANGE) { + if (ssl_cipher_requires_server_key_exchange(s->s3->tmp.new_cipher)) + { + OPENSSL_PUT_ERROR(SSL, ssl3_get_key_exchange, SSL_R_UNEXPECTED_MESSAGE); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE); + return -1; + } + #ifndef OPENSSL_NO_PSK /* In plain PSK ciphersuite, ServerKeyExchange can be omitted if no identity hint is sent. Set diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c index 4c4bb67c..28287a26 100644 --- a/ssl/ssl_ciph.c +++ b/ssl/ssl_ciph.c @@ -1883,3 +1883,28 @@ int ssl_cipher_has_server_public_key(const SSL_CIPHER *cipher) /* All other ciphers include it. */ return 1; } + +/* ssl_cipher_requires_server_key_exchange returns 1 if |cipher| + * requires a ServerKeyExchange message. Otherwise it returns 0. + * + * Unlike ssl_cipher_has_server_public_key, some ciphers take optional + * ServerKeyExchanges. PSK and RSA_PSK only use the ServerKeyExchange + * to communicate a psk_identity_hint, so it is optional. + * + * Also, as implemented, the RSA key exchange takes an optional + * ServerKeyExchange containing a signed ephemeral RSA encryption key. + * + * TODO(davidben): Can we remove the RSA one? This is a remnant of + * RSA_EXPORT ciphers which required this (it was used to generate an + * ephemeral 512-bit RSA encryption key), but it's allowed for all RSA + * ciphers. There's even a SSL_OP_EPHEMERAL_RSA to always use it. */ +int ssl_cipher_requires_server_key_exchange(const SSL_CIPHER *cipher) + { + /* Ephemeral Diffie-Hellman key exchanges require a + * ServerKeyExchange. */ + if (cipher->algorithm_mkey & SSL_kEDH || + cipher->algorithm_mkey & SSL_kEECDH) + return 1; + /* It is optional in all others. */ + return 0; + } diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index ed3a0b5f..3d1b2ca6 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -994,6 +994,7 @@ int ssl_get_handshake_digest(int i,long *mask,const EVP_MD **md); int ssl_cipher_get_cert_index(const SSL_CIPHER *c); const SSL_CIPHER *ssl_get_cipher_by_char(SSL *ssl, const unsigned char *ptr); int ssl_cipher_has_server_public_key(const SSL_CIPHER *cipher); +int ssl_cipher_requires_server_key_exchange(const SSL_CIPHER *cipher); int ssl_cert_set0_chain(CERT *c, STACK_OF(X509) *chain); int ssl_cert_set1_chain(CERT *c, STACK_OF(X509) *chain); diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 328807a9..ffa35db0 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -359,6 +359,10 @@ type ProtocolBugs struct { // Certificate message is sent and no signature is added to // ServerKeyExchange. UnauthenticatedECDH bool + + // SkipServerKeyExchange causes the server to skip sending + // ServerKeyExchange messages. + SkipServerKeyExchange bool } func (c *Config) serverInit() { diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 0b49a00f..f177fc8b 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -332,7 +332,7 @@ func (hs *serverHandshakeState) doFullHandshake() error { c.sendAlert(alertHandshakeFailure) return err } - if skx != nil { + if skx != nil && !config.Bugs.SkipServerKeyExchange { hs.finishedHash.Write(skx.marshal()) c.writeRecord(recordTypeHandshake, skx.marshal()) } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 1207e9ad..3d3e538e 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -200,6 +200,17 @@ var testCases = []testCase{ shouldFail: true, expectedError: ":UNEXPECTED_MESSAGE:", }, + { + name: "SkipServerKeyExchange", + config: Config{ + CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, + Bugs: ProtocolBugs{ + SkipServerKeyExchange: true, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_MESSAGE:", + }, } func doExchange(tlsConn *Conn, messageLen int) error { |