Tighten up SSLCipherPreferenceList

Some minor tweaks with no intended functional changes, in preparation
for using SSLCipherPreferenceList for TLS 1.3 ciphers as well.

Bug: 545123692
Change-Id: I1d7ad9942c2651feb02ebbd626a63a726a6a6964
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/101568
Auto-Submit: Lily Chen <chlily@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Lily Chen <chlily@google.com>

ssl/handoff.cc

diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index c8d9897..00d8cf4 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -146,9 +146,9 @@
       return false;
     }
   }
-  STACK_OF(SSL_CIPHER) *configured =
-      ssl->config->cipher_list ? ssl->config->cipher_list->ciphers.get()
-                               : ssl->ctx->cipher_list->ciphers.get();
+  const STACK_OF(SSL_CIPHER) *configured =
+      ssl->config->cipher_list ? ssl->config->cipher_list->ciphers()
+                               : ssl->ctx->cipher_list->ciphers();
   bssl::UniquePtr<STACK_OF(SSL_CIPHER)> unsupported(sk_SSL_CIPHER_new_null());
   if (!unsupported) {
     return false;
@@ -164,7 +164,7 @@
   if (sk_SSL_CIPHER_num(unsupported.get()) && !ssl->config->cipher_list) {
     ssl->config->cipher_list = bssl::MakeUnique<SSLCipherPreferenceList>();
     if (!ssl->config->cipher_list ||
-        !ssl->config->cipher_list->Init(*ssl->ctx->cipher_list)) {
+        !ssl->config->cipher_list->CopyFrom(*ssl->ctx->cipher_list)) {
       return false;
     }
   }

ssl/handshake_server.cc

diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 64ea061..0b4f1a4 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -160,11 +160,11 @@
                                        uint32_t mask_k, uint32_t mask_a) {
   SSLImpl *const ssl = hs->ssl;
   const STACK_OF(SSL_CIPHER) *prio, *allow;
-  // in_group_flags will either be NULL, or will point to an array of bytes
+  // in_group_flags will either be empty, or will contain an array of bytes
   // which indicate equal-preference groups in the `prio` stack. See the
   // comment about `in_group_flags` in the `SSLCipherPreferenceList`
   // struct.
-  const bool *in_group_flags;
+  Span<const bool> in_group_flags;
   // best_index contains the index of the best matching cipher suite found so
   // far, indexed into `allow`. If `best_index` is `SIZE_MAX`, no matching
   // cipher suite has been found yet.
@@ -174,18 +174,18 @@
       hs->config->cipher_list ? hs->config->cipher_list.get()
                               : ssl->ctx->cipher_list.get();
   if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) {
-    prio = server_pref->ciphers.get();
-    in_group_flags = server_pref->in_group_flags;
+    prio = server_pref->ciphers();
+    in_group_flags = server_pref->in_group_flags();
     allow = client_pref;
   } else {
     prio = client_pref;
-    in_group_flags = nullptr;
-    allow = server_pref->ciphers.get();
+    in_group_flags = Span<const bool>();
+    allow = server_pref->ciphers();
   }
 
   for (size_t i = 0; i < sk_SSL_CIPHER_num(prio); i++) {
     const SSL_CIPHER *c = sk_SSL_CIPHER_value(prio, i);
-    const bool in_group = in_group_flags != nullptr && in_group_flags[i];
+    const bool in_group = !in_group_flags.empty() && in_group_flags[i];
 
     size_t cipher_index;
     if (  // Check if the cipher is supported for the current version.

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index b60f5a1..e3962b4 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -287,20 +287,35 @@
 //    A         E
 //    B -> D -> F
 //    C
-struct SSLCipherPreferenceList {
+class SSLCipherPreferenceList {
+ public:
   static constexpr bool kAllowUniquePtr = true;
 
   SSLCipherPreferenceList() = default;
-  ~SSLCipherPreferenceList();
+  ~SSLCipherPreferenceList() = default;
 
+  // Initializes a list with the specified ciphers and flags.
   bool Init(UniquePtr<STACK_OF(SSL_CIPHER)> ciphers,
-            Span<const bool> in_group_flags);
-  bool Init(const SSLCipherPreferenceList &);
+            Array<bool> in_group_flags);
 
+  // Makes `this` a deep copy of another (already initialized) instance.
+  bool CopyFrom(const SSLCipherPreferenceList &);
+
+  // Removes `cipher` from the preference list.
   void Remove(const SSL_CIPHER *cipher);
 
-  UniquePtr<STACK_OF(SSL_CIPHER)> ciphers;
-  bool *in_group_flags = nullptr;
+  size_t size() const { return sk_SSL_CIPHER_num(ciphers_.get()); }
+
+  const STACK_OF(SSL_CIPHER) *ciphers() const { return ciphers_.get(); }
+  STACK_OF(SSL_CIPHER) *ciphers() { return ciphers_.get(); }
+
+  Span<const bool> in_group_flags() const { return in_group_flags_; }
+
+ private:
+  // SSL_CIPHERs are maintained in a stack so they are easily accessible in the
+  // form required for `SSL{_CTX}_get_ciphers`.
+  UniquePtr<STACK_OF(SSL_CIPHER)> ciphers_;
+  Array<bool> in_group_flags_;
 };
 
 // AllCiphers returns an array of all supported ciphers, sorted by id.

ssl/ssl_cipher.cc

diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc
index e32bec5..4b7a78b 100644
--- a/ssl/ssl_cipher.cc
+++ b/ssl/ssl_cipher.cc
@@ -605,50 +605,44 @@
   *head = curr;
 }
 
-SSLCipherPreferenceList::~SSLCipherPreferenceList() {
-  OPENSSL_free(in_group_flags);
-}
-
-bool SSLCipherPreferenceList::Init(UniquePtr<STACK_OF(SSL_CIPHER)> ciphers_arg,
-                                   Span<const bool> in_group_flags_arg) {
-  if (sk_SSL_CIPHER_num(ciphers_arg.get()) != in_group_flags_arg.size()) {
+bool SSLCipherPreferenceList::Init(UniquePtr<STACK_OF(SSL_CIPHER)> ciphers,
+                                   Array<bool> in_group_flags) {
+  if (sk_SSL_CIPHER_num(ciphers.get()) != in_group_flags.size()) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
     return false;
   }
 
-  Array<bool> copy;
-  if (!copy.CopyFrom(in_group_flags_arg)) {
-    return false;
-  }
-  ciphers = std::move(ciphers_arg);
-  size_t unused_len;
-  copy.Release(&in_group_flags, &unused_len);
+  ciphers_ = std::move(ciphers);
+  in_group_flags_ = std::move(in_group_flags);
   return true;
 }
 
-bool SSLCipherPreferenceList::Init(const SSLCipherPreferenceList &other) {
-  size_t size = sk_SSL_CIPHER_num(other.ciphers.get());
-  Span<const bool> other_flags(other.in_group_flags, size);
+bool SSLCipherPreferenceList::CopyFrom(const SSLCipherPreferenceList &other) {
   UniquePtr<STACK_OF(SSL_CIPHER)> other_ciphers(
-      sk_SSL_CIPHER_dup(other.ciphers.get()));
+      sk_SSL_CIPHER_dup(other.ciphers()));
   if (!other_ciphers) {
     return false;
   }
-  return Init(std::move(other_ciphers), other_flags);
+  Array<bool> other_flags;
+  if (!other_flags.CopyFrom(other.in_group_flags())) {
+    return false;
+  }
+  return Init(std::move(other_ciphers), std::move(other_flags));
 }
 
 void SSLCipherPreferenceList::Remove(const SSL_CIPHER *cipher) {
   size_t index;
-  if (!sk_SSL_CIPHER_find(ciphers.get(), &index, cipher)) {
+  if (!sk_SSL_CIPHER_find(ciphers_.get(), &index, cipher)) {
     return;
   }
-  if (!in_group_flags[index] /* last element of group */ && index > 0) {
-    in_group_flags[index - 1] = false;
+  if (!in_group_flags_[index] /* last element of group */ && index > 0) {
+    in_group_flags_[index - 1] = false;
   }
-  for (size_t i = index; i < sk_SSL_CIPHER_num(ciphers.get()) - 1; ++i) {
-    in_group_flags[i] = in_group_flags[i + 1];
+  for (size_t i = index; i < size() - 1; ++i) {
+    in_group_flags_[i] = in_group_flags_[i + 1];
   }
-  sk_SSL_CIPHER_delete(ciphers.get(), index);
+  sk_SSL_CIPHER_delete(ciphers_.get(), index);
+  in_group_flags_.Shrink(size());
 }
 
 bool ssl_cipher_is_deprecated(const SSL_CIPHER *cipher) {
@@ -1133,7 +1127,8 @@
 
   UniquePtr<SSLCipherPreferenceList> pref_list =
       MakeUnique<SSLCipherPreferenceList>();
-  if (!pref_list || !pref_list->Init(std::move(cipherstack), in_group_flags)) {
+  if (!pref_list ||
+      !pref_list->Init(std::move(cipherstack), std::move(in_group_flags))) {
     return false;
   }
 
@@ -1141,7 +1136,7 @@
 
   // Configuring an empty cipher list is an error but still updates the
   // output.
-  if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers.get()) == 0) {
+  if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers()) == 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH);
     return false;
   }

ssl/ssl_lib.cc

diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 101ca7e..fd526a6 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -2146,15 +2146,15 @@
 int SSL_set_tmp_dh(SSL *ssl, const DH *dh) { return 1; }
 
 STACK_OF(SSL_CIPHER) *SSL_CTX_get_ciphers(const SSL_CTX *ctx) {
-  return FromOpaque(ctx)->cipher_list->ciphers.get();
+  return FromOpaque(ctx)->cipher_list->ciphers();
 }
 
 int SSL_CTX_cipher_in_group(const SSL_CTX *ctx, size_t i) {
   auto *ctx_impl = FromOpaque(ctx);
-  if (i >= sk_SSL_CIPHER_num(ctx_impl->cipher_list->ciphers.get())) {
+  if (i >= sk_SSL_CIPHER_num(ctx_impl->cipher_list->ciphers())) {
     return 0;
   }
-  return ctx_impl->cipher_list->in_group_flags[i];
+  return ctx_impl->cipher_list->in_group_flags()[i];
 }
 
 STACK_OF(SSL_CIPHER) *SSL_get_ciphers(const SSL *ssl) {
@@ -2168,8 +2168,8 @@
   }
 
   return ssl_impl->config->cipher_list
-             ? ssl_impl->config->cipher_list->ciphers.get()
-             : ssl_impl->ctx->cipher_list->ciphers.get();
+             ? ssl_impl->config->cipher_list->ciphers()
+             : ssl_impl->ctx->cipher_list->ciphers();
 }
 
 const char *SSL_get_cipher_list(const SSL *ssl, int n) {