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>

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 {