Fix DTLS_ANY_VERSION and add tests.
This fixes bugs that kept the tests from working:
- Resolve DTLS version and cookie before the session.
- In DTLS_ANY_VERSION, ServerHello should be read with first_packet = 1. This
is a regression from f2fedefdcaf62f10b566f55858c25f35112072ea. We'll want to
do the same for TLS, but first let's change this to a boolean has_version in a
follow-up.
Things not yet fixed:
- DTLS code is not EVP_AEAD-aware. Those ciphers are disabled for now.
- On the client, DTLS_ANY_VERSION creates SSL_SESSIONs with the wrong
ssl_version. The tests pass because we no longer enforce the match as of
e37216f56009fbf48c3a1e733b7a546ca6dfc2af. (In fact, we've gone from the server
ignoring ssl_version and client enforcing to the client mostly ignoring
ssl_version and the server enforcing.)
- ssl3_send_client_hello's ssl_version check checks for equality against
s->version rather than >.
Change-Id: I5a0dde221b2009413df9b9443882b9bf3b29519c
Reviewed-on: https://boringssl-review.googlesource.com/2403
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index cc7980f..71c1c9b 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -317,6 +317,10 @@
{
if (ciph->algorithm_enc == SSL_RC4)
return NULL;
+ /* TODO(davidben): EVP_AEAD does not work in DTLS yet. */
+ if (ciph->algorithm2 & SSL_CIPHER_ALGORITHM2_AEAD ||
+ ciph->algorithm2 & SSL_CIPHER_ALGORITHM2_STATEFUL_AEAD)
+ return NULL;
}
return ciph;
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index dae47d7..9356580 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -587,7 +587,15 @@
{
if (version != s->version)
{
- /* unexpected version, silently discard */
+ /* The record's version doesn't match, so
+ * silently drop it.
+ *
+ * TODO(davidben): This doesn't work. The DTLS
+ * record layer is not packet-based, so the
+ * remainder of the packet isn't dropped and we
+ * get a framing error. It's also unclear what
+ * it means to silently drop a record in a
+ * packet containing two records. */
rr->length = 0;
s->packet_length = 0;
goto again;
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 66f32d5..6f47e95 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -805,6 +805,11 @@
uint8_t compression_method;
unsigned long mask_ssl;
+ /* DTLS_ANY_VERSION does not sniff the version ahead of time,
+ * so disable the version check. */
+ if (SSL_IS_DTLS(s))
+ s->first_packet = 1;
+
n=s->method->ssl_get_message(s,
SSL3_ST_CR_SRVR_HELLO_A,
SSL3_ST_CR_SRVR_HELLO_B,
@@ -813,6 +818,9 @@
SSL_GET_MESSAGE_HASH_MESSAGE,
&ok);
+ if (SSL_IS_DTLS(s))
+ s->first_packet = 0;
+
if (!ok) return((int)n);
CBS_init(&server_hello, s->init_msg, n);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index f3b3779..fa4a591 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -851,50 +851,6 @@
/* Load the client random. */
memcpy(s->s3->client_random, CBS_data(&client_random), SSL3_RANDOM_SIZE);
- s->hit=0;
- /* Versions before 0.9.7 always allow clients to resume sessions in renegotiation.
- * 0.9.7 and later allow this by default, but optionally ignore resumption requests
- * with flag SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION (it's a new flag rather
- * than a change to default behavior so that applications relying on this for security
- * won't even compile against older library versions).
- *
- * 1.0.1 and later also have a function SSL_renegotiate_abbreviated() to request
- * renegotiation but not a new session (s->new_session remains unset): for servers,
- * this essentially just means that the SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
- * setting will be ignored.
- */
- if ((s->new_session && (s->options & SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION)))
- {
- if (!ssl_get_new_session(s,1))
- goto err;
- }
- else
- {
- i = ssl_get_prev_session(s, &early_ctx);
- if (i == PENDING_SESSION)
- {
- ret = PENDING_SESSION;
- goto err;
- }
- else if (i == -1)
- {
- goto err;
- }
-
- /* Only resume if the session's version matches the negotiated
- * version: most clients do not accept a mismatch. */
- if (i == 1 && s->version == s->session->ssl_version)
- {
- s->hit = 1;
- }
- else
- {
- /* No session was found or it was unacceptable. */
- if (!ssl_get_new_session(s, 1))
- goto err;
- }
- }
-
if (SSL_IS_DTLS(s))
{
CBS cookie;
@@ -956,7 +912,50 @@
al = SSL_AD_PROTOCOL_VERSION;
goto f_err;
}
- s->session->ssl_version = s->version;
+ }
+ }
+
+ s->hit=0;
+ /* Versions before 0.9.7 always allow clients to resume sessions in renegotiation.
+ * 0.9.7 and later allow this by default, but optionally ignore resumption requests
+ * with flag SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION (it's a new flag rather
+ * than a change to default behavior so that applications relying on this for security
+ * won't even compile against older library versions).
+ *
+ * 1.0.1 and later also have a function SSL_renegotiate_abbreviated() to request
+ * renegotiation but not a new session (s->new_session remains unset): for servers,
+ * this essentially just means that the SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
+ * setting will be ignored.
+ */
+ if ((s->new_session && (s->options & SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION)))
+ {
+ if (!ssl_get_new_session(s,1))
+ goto err;
+ }
+ else
+ {
+ i = ssl_get_prev_session(s, &early_ctx);
+ if (i == PENDING_SESSION)
+ {
+ ret = PENDING_SESSION;
+ goto err;
+ }
+ else if (i == -1)
+ {
+ goto err;
+ }
+
+ /* Only resume if the session's version matches the negotiated
+ * version: most clients do not accept a mismatch. */
+ if (i == 1 && s->version == s->session->ssl_version)
+ {
+ s->hit = 1;
+ }
+ else
+ {
+ /* No session was found or it was unacceptable. */
+ if (!ssl_get_new_session(s, 1))
+ goto err;
}
}
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index a4dd06d..0202407 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -229,14 +229,10 @@
const SSL_METHOD *method;
if (config->is_dtls) {
- // TODO(davidben): Get DTLS 1.2 working and test the version negotiation
- // codepath. This doesn't currently work because
- // - Session resumption is broken: https://crbug.com/403378
- // - DTLS hasn't been updated for EVP_AEAD.
if (config->is_server) {
- method = DTLSv1_server_method();
+ method = DTLS_server_method();
} else {
- method = DTLSv1_client_method();
+ method = DTLS_client_method();
}
} else {
if (config->is_server) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index f106e60..79f8ee0 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -889,11 +889,12 @@
name string
version uint16
flag string
+ hasDTLS bool
}{
- {"SSL3", VersionSSL30, "-no-ssl3"},
- {"TLS1", VersionTLS10, "-no-tls1"},
- {"TLS11", VersionTLS11, "-no-tls11"},
- {"TLS12", VersionTLS12, "-no-tls12"},
+ {"SSL3", VersionSSL30, "-no-ssl3", false},
+ {"TLS1", VersionTLS10, "-no-tls1", true},
+ {"TLS11", VersionTLS11, "-no-tls11", false},
+ {"TLS12", VersionTLS12, "-no-tls12", true},
}
var testCipherSuites = []struct {
@@ -935,10 +936,21 @@
{"RC4-SHA", TLS_RSA_WITH_RC4_128_SHA},
}
+func hasComponent(suiteName, component string) bool {
+ return strings.Contains("-"+suiteName+"-", "-"+component+"-")
+}
+
func isTLS12Only(suiteName string) bool {
- return strings.HasSuffix(suiteName, "-GCM") ||
- strings.HasSuffix(suiteName, "-SHA256") ||
- strings.HasSuffix(suiteName, "-SHA384")
+ return hasComponent(suiteName, "GCM") ||
+ hasComponent(suiteName, "SHA256") ||
+ hasComponent(suiteName, "SHA384")
+}
+
+func isDTLSCipher(suiteName string) bool {
+ // TODO(davidben): AES-GCM exists in DTLS 1.2 but is currently
+ // broken because DTLS is not EVP_AEAD-aware.
+ return !hasComponent(suiteName, "RC4") &&
+ !hasComponent(suiteName, "GCM")
}
func addCipherSuiteTests() {
@@ -949,7 +961,7 @@
var cert Certificate
var certFile string
var keyFile string
- if strings.Contains(suite.name, "ECDSA") {
+ if hasComponent(suite.name, "ECDSA") {
cert = getECDSACertificate()
certFile = ecdsaCertificateFile
keyFile = ecdsaKeyFile
@@ -960,7 +972,7 @@
}
var flags []string
- if strings.HasPrefix(suite.name, "PSK-") || strings.Contains(suite.name, "-PSK-") {
+ if hasComponent(suite.name, "PSK") {
flags = append(flags,
"-psk", psk,
"-psk-identity", pskIdentity)
@@ -1003,8 +1015,7 @@
resumeSession: true,
})
- // TODO(davidben): Fix DTLS 1.2 support and test that.
- if ver.version == VersionTLS10 && strings.Index(suite.name, "RC4") == -1 {
+ if ver.hasDTLS && isDTLSCipher(suite.name) {
testCases = append(testCases, testCase{
testType: clientTest,
protocol: dtls,
@@ -1593,31 +1604,43 @@
}
for _, runnerVers := range tlsVersions {
- expectedVersion := shimVers.version
- if runnerVers.version < shimVers.version {
- expectedVersion = runnerVers.version
+ protocols := []protocol{tls}
+ if runnerVers.hasDTLS && shimVers.hasDTLS {
+ protocols = append(protocols, dtls)
}
- suffix := shimVers.name + "-" + runnerVers.name
+ for _, protocol := range protocols {
+ expectedVersion := shimVers.version
+ if runnerVers.version < shimVers.version {
+ expectedVersion = runnerVers.version
+ }
- testCases = append(testCases, testCase{
- testType: clientTest,
- name: "VersionNegotiation-Client-" + suffix,
- config: Config{
- MaxVersion: runnerVers.version,
- },
- flags: flags,
- expectedVersion: expectedVersion,
- })
+ suffix := shimVers.name + "-" + runnerVers.name
+ if protocol == dtls {
+ suffix += "-DTLS"
+ }
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "VersionNegotiation-Server-" + suffix,
- config: Config{
- MaxVersion: runnerVers.version,
- },
- flags: flags,
- expectedVersion: expectedVersion,
- })
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ testType: clientTest,
+ name: "VersionNegotiation-Client-" + suffix,
+ config: Config{
+ MaxVersion: runnerVers.version,
+ },
+ flags: flags,
+ expectedVersion: expectedVersion,
+ })
+
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ testType: serverTest,
+ name: "VersionNegotiation-Server-" + suffix,
+ config: Config{
+ MaxVersion: runnerVers.version,
+ },
+ flags: flags,
+ expectedVersion: expectedVersion,
+ })
+ }
}
}
}
@@ -1904,69 +1927,80 @@
}
func addResumptionVersionTests() {
- // TODO(davidben): Once DTLS 1.2 is working, test that as well.
for _, sessionVers := range tlsVersions {
for _, resumeVers := range tlsVersions {
- suffix := "-" + sessionVers.name + "-" + resumeVers.name
-
- testCases = append(testCases, testCase{
- name: "Resume-Client" + suffix,
- resumeSession: true,
- config: Config{
- MaxVersion: sessionVers.version,
- CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
- Bugs: ProtocolBugs{
- AllowSessionVersionMismatch: true,
- },
- },
- expectedVersion: sessionVers.version,
- resumeConfig: &Config{
- MaxVersion: resumeVers.version,
- CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
- Bugs: ProtocolBugs{
- AllowSessionVersionMismatch: true,
- },
- },
- expectedResumeVersion: resumeVers.version,
- })
-
- testCases = append(testCases, testCase{
- name: "Resume-Client-NoResume" + suffix,
- flags: []string{"-expect-session-miss"},
- resumeSession: true,
- config: Config{
- MaxVersion: sessionVers.version,
- CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
- },
- expectedVersion: sessionVers.version,
- resumeConfig: &Config{
- MaxVersion: resumeVers.version,
- CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
- },
- newSessionsOnResume: true,
- expectedResumeVersion: resumeVers.version,
- })
-
- var flags []string
- if sessionVers.version != resumeVers.version {
- flags = append(flags, "-expect-session-miss")
+ protocols := []protocol{tls}
+ if sessionVers.hasDTLS && resumeVers.hasDTLS {
+ protocols = append(protocols, dtls)
}
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Resume-Server" + suffix,
- flags: flags,
- resumeSession: true,
- config: Config{
- MaxVersion: sessionVers.version,
- CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
- },
- expectedVersion: sessionVers.version,
- resumeConfig: &Config{
- MaxVersion: resumeVers.version,
- CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
- },
- expectedResumeVersion: resumeVers.version,
- })
+ for _, protocol := range protocols {
+ suffix := "-" + sessionVers.name + "-" + resumeVers.name
+ if protocol == dtls {
+ suffix += "-DTLS"
+ }
+
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ name: "Resume-Client" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: sessionVers.version,
+ CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+ Bugs: ProtocolBugs{
+ AllowSessionVersionMismatch: true,
+ },
+ },
+ expectedVersion: sessionVers.version,
+ resumeConfig: &Config{
+ MaxVersion: resumeVers.version,
+ CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+ Bugs: ProtocolBugs{
+ AllowSessionVersionMismatch: true,
+ },
+ },
+ expectedResumeVersion: resumeVers.version,
+ })
+
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ name: "Resume-Client-NoResume" + suffix,
+ flags: []string{"-expect-session-miss"},
+ resumeSession: true,
+ config: Config{
+ MaxVersion: sessionVers.version,
+ CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+ },
+ expectedVersion: sessionVers.version,
+ resumeConfig: &Config{
+ MaxVersion: resumeVers.version,
+ CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+ },
+ newSessionsOnResume: true,
+ expectedResumeVersion: resumeVers.version,
+ })
+
+ var flags []string
+ if sessionVers.version != resumeVers.version {
+ flags = append(flags, "-expect-session-miss")
+ }
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ testType: serverTest,
+ name: "Resume-Server" + suffix,
+ flags: flags,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: sessionVers.version,
+ CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+ },
+ expectedVersion: sessionVers.version,
+ resumeConfig: &Config{
+ MaxVersion: resumeVers.version,
+ CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+ },
+ expectedResumeVersion: resumeVers.version,
+ })
+ }
}
}
}
