From 91222b8d3835844802b7564ac12e92055e8554eb Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 9 Mar 2017 20:10:56 -0500 Subject: [PATCH] Fix configuring the empty cipher list. Although it returns failure, the cipher list should still be updated. Conscrypt relies on this behavior to support a Java API edge case. Change-Id: If58efafc6a4a81e85a0e2ee2c38873a7a4938123 Reviewed-on: https://boringssl-review.googlesource.com/14165 Reviewed-by: Kenny Root Reviewed-by: David Benjamin Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/ssl_cipher.c | 12 +++++++----- ssl/ssl_test.cc | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/ssl/ssl_cipher.c b/ssl/ssl_cipher.c index dc9cc2a6..4ee3c125 100644 --- a/ssl/ssl_cipher.c +++ b/ssl/ssl_cipher.c @@ -1377,11 +1377,6 @@ int ssl_create_cipher_list( OPENSSL_free(co_list); /* Not needed any longer */ co_list = NULL; - if (sk_SSL_CIPHER_num(cipherstack) == 0) { - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH); - goto err; - } - pref_list = OPENSSL_malloc(sizeof(struct ssl_cipher_preference_list_st)); if (!pref_list) { goto err; @@ -1400,6 +1395,13 @@ int ssl_create_cipher_list( *out_cipher_list = pref_list; pref_list = NULL; + /* Configuring an empty cipher list is an error but still updates the + * output. */ + if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers) == 0) { + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH); + return 0; + } + return 1; err: diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 0dc240aa..5f89b818 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -3229,6 +3229,23 @@ TEST(SSLTest, SetChainAndKey) { nullptr /* no session */)); } +// Configuring the empty cipher list, though an error, should still modify the +// configuration. +TEST(SSLTest, EmptyCipherList) { + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + + // Initially, the cipher list is not empty. + EXPECT_NE(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get()))); + + // Configuring the empty cipher list fails. + EXPECT_FALSE(SSL_CTX_set_cipher_list(ctx.get(), "")); + ERR_clear_error(); + + // But the cipher list is still updated to empty. + EXPECT_EQ(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get()))); +} + // TODO(davidben): Convert this file to GTest properly. TEST(SSLTest, AllTests) { if (!TestCipherRules() ||