From 353577cdc798444e1add6215c8e12c10234088ac Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 29 Jun 2017 15:54:58 -0400 Subject: [PATCH] Fix SSL_set_{min,max}_proto_version APIs in invalid versions. SSL_set_max_proto_version(TLS1_3_DRAFT_VERSION) worked unintentionally. Fix that. Also add an error when it fails. Change-Id: I1048fede7b163e1c170e17bf4370b468221a7077 Reviewed-on: https://boringssl-review.googlesource.com/17525 Reviewed-by: David Benjamin Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/ssl_test.cc | 4 ++++ ssl/ssl_versions.c | 13 +++++++++++-- ssl/test/runner/common.go | 5 +++++ ssl/test/runner/runner.go | 40 +++++++++++++++++++++++---------------- 4 files changed, 44 insertions(+), 18 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 84b7496d..7737e756 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -2567,6 +2567,10 @@ TEST(SSLTest, SetVersion) { EXPECT_TRUE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_3_VERSION)); EXPECT_EQ(TLS1_3_VERSION, ctx->conf_max_version); + // TLS1_3_DRAFT_VERSION is not an API-level version. + EXPECT_FALSE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_3_DRAFT_VERSION)); + ERR_clear_error(); + ctx.reset(SSL_CTX_new(DTLS_method())); ASSERT_TRUE(ctx); diff --git a/ssl/ssl_versions.c b/ssl/ssl_versions.c index 65656885..80b62ccd 100644 --- a/ssl/ssl_versions.c +++ b/ssl/ssl_versions.c @@ -95,12 +95,21 @@ static int set_version_bound(const SSL_PROTOCOL_METHOD *method, uint16_t *out, /* The public API uses wire versions, except we use |TLS1_3_VERSION| * everywhere to refer to any draft TLS 1.3 versions. In this direction, we * map it to some representative TLS 1.3 draft version. */ + if (version == TLS1_3_DRAFT_VERSION) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_SSL_VERSION); + return 0; + } if (version == TLS1_3_VERSION) { version = TLS1_3_DRAFT_VERSION; } - return method_supports_version(method, version) && - ssl_protocol_version_from_wire(out, version); + if (!method_supports_version(method, version) || + !ssl_protocol_version_from_wire(out, version)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_SSL_VERSION); + return 0; + } + + return 1; } static int set_min_version(const SSL_PROTOCOL_METHOD *method, uint16_t *out, diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 0a6648ff..bd490d3e 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -26,6 +26,11 @@ const ( VersionTLS13 = 0x0304 ) +const ( + VersionDTLS10 = 0xfeff + VersionDTLS12 = 0xfefd +) + // A draft version of TLS 1.3 that is sent over the wire for the current draft. const tls13DraftVersion = 0x7f12 diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index d9218f26..ba6cc54d 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -1155,16 +1155,27 @@ func runTest(test *testCase, shimPath string, mallocNumToFail int64) error { type tlsVersion struct { name string version uint16 - flag string - hasDTLS bool + // excludeFlag is the legacy shim flag to disable the version. + excludeFlag string + hasDTLS bool + // shimTLS and shimDTLS are values the shim uses to refer to these + // versions in TLS and DTLS, respectively. + shimTLS, shimDTLS int +} + +func (vers tlsVersion) shimFlag(protocol protocol) string { + if protocol == dtls { + return strconv.Itoa(vers.shimDTLS) + } + return strconv.Itoa(vers.shimTLS) } var tlsVersions = []tlsVersion{ - {"SSL3", VersionSSL30, "-no-ssl3", false}, - {"TLS1", VersionTLS10, "-no-tls1", true}, - {"TLS11", VersionTLS11, "-no-tls11", false}, - {"TLS12", VersionTLS12, "-no-tls12", true}, - {"TLS13", VersionTLS13, "-no-tls13", false}, + {"SSL3", VersionSSL30, "-no-ssl3", false, VersionSSL30, 0}, + {"TLS1", VersionTLS10, "-no-tls1", true, VersionTLS10, VersionDTLS10}, + {"TLS11", VersionTLS11, "-no-tls11", false, VersionTLS11, 0}, + {"TLS12", VersionTLS12, "-no-tls12", true, VersionTLS12, VersionDTLS12}, + {"TLS13", VersionTLS13, "-no-tls13", false, VersionTLS13, 0}, } type testCipherSuite struct { @@ -4541,7 +4552,7 @@ func addVersionNegotiationTests() { // Assemble flags to disable all newer versions on the shim. var flags []string for _, vers := range tlsVersions[i+1:] { - flags = append(flags, vers.flag) + flags = append(flags, vers.excludeFlag) } // Test configuring the runner's maximum version. @@ -4561,8 +4572,6 @@ func addVersionNegotiationTests() { suffix += "-DTLS" } - shimVersFlag := strconv.Itoa(int(versionToWire(shimVers.version, protocol == dtls))) - // Determine the expected initial record-layer versions. clientVers := shimVers.version if clientVers > VersionTLS10 { @@ -4598,7 +4607,7 @@ func addVersionNegotiationTests() { ExpectInitialRecordVersion: clientVers, }, }, - flags: []string{"-max-version", shimVersFlag}, + flags: []string{"-max-version", shimVers.shimFlag(protocol)}, expectedVersion: expectedVersion, }) @@ -4625,7 +4634,7 @@ func addVersionNegotiationTests() { ExpectInitialRecordVersion: serverVers, }, }, - flags: []string{"-max-version", shimVersFlag}, + flags: []string{"-max-version", shimVers.shimFlag(protocol)}, expectedVersion: expectedVersion, }) } @@ -4882,7 +4891,7 @@ func addMinimumVersionTests() { // Assemble flags to disable all older versions on the shim. var flags []string for _, vers := range tlsVersions[:i] { - flags = append(flags, vers.flag) + flags = append(flags, vers.excludeFlag) } for _, runnerVers := range tlsVersions { @@ -4895,7 +4904,6 @@ func addMinimumVersionTests() { if protocol == dtls { suffix += "-DTLS" } - shimVersFlag := strconv.Itoa(int(versionToWire(shimVers.version, protocol == dtls))) var expectedVersion uint16 var shouldFail bool @@ -4942,7 +4950,7 @@ func addMinimumVersionTests() { IgnorePeerCipherPreferences: shouldFail, }, }, - flags: []string{"-min-version", shimVersFlag}, + flags: []string{"-min-version", shimVers.shimFlag(protocol)}, expectedVersion: expectedVersion, shouldFail: shouldFail, expectedError: expectedError, @@ -4969,7 +4977,7 @@ func addMinimumVersionTests() { config: Config{ MaxVersion: runnerVers.version, }, - flags: []string{"-min-version", shimVersFlag}, + flags: []string{"-min-version", shimVers.shimFlag(protocol)}, expectedVersion: expectedVersion, shouldFail: shouldFail, expectedError: expectedError,