commit 0dc8b016ee4b11ee44e284251d9e107d9df31d3d
author David Benjamin <davidben@google.com> Tue Oct 08 13:07:36 2024 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Oct 08 22:04:13 2024 +0000
tree 03cb1379e15426d27bf3c386b8d60700e71cb230
parent 12a48b2b110a2f3ed705fa4660e82831c5709cdb

Bounds-check bssl::Array and bssl::Vector

These are internal types, and we actually don't even index into them
that much, but we should aim to be a bit safer.

Change-Id: Ifad9a4382d63d135c2fdd65fc04ffb0dfca07b7b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71829
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index d4c1c27..a0d4221 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -289,8 +289,14 @@
   size_t size() const { return size_; }
   bool empty() const { return size_ == 0; }
 
-  const T &operator[](size_t i) const { return data_[i]; }
-  T &operator[](size_t i) { return data_[i]; }
+  const T &operator[](size_t i) const {
+    BSSL_CHECK(i < size_);
+    return data_[i];
+  }
+  T &operator[](size_t i) {
+    BSSL_CHECK(i < size_);
+    return data_[i];
+  }
 
   T *begin() { return data_; }
   const T *begin() const { return data_; }
@@ -399,8 +405,14 @@
   size_t size() const { return size_; }
   bool empty() const { return size_ == 0; }
 
-  const T &operator[](size_t i) const { return data_[i]; }
-  T &operator[](size_t i) { return data_[i]; }
+  const T &operator[](size_t i) const {
+    BSSL_CHECK(i < size_);
+    return data_[i];
+  }
+  T &operator[](size_t i) {
+    BSSL_CHECK(i < size_);
+    return data_[i];
+  }
 
   T *begin() { return data_; }
   const T *begin() const { return data_; }

ssl/span_test.cc

diff --git a/ssl/span_test.cc b/ssl/span_test.cc
index 481b0fc..84cb5a3 100644
--- a/ssl/span_test.cc
+++ b/ssl/span_test.cc
@@ -98,5 +98,22 @@
   static_assert(span2[0] == 1, "wrong value");
 }
 
+TEST(SpanDeathTest, BoundsChecks) {
+  // Make an array that's larger than we need, so that a failure to bounds check
+  // won't crash.
+  const int v[] = {1, 2, 3, 4};
+  Span<const int> span(v, 3);
+  // Out of bounds access.
+  EXPECT_DEATH_IF_SUPPORTED(span[3], "");
+  EXPECT_DEATH_IF_SUPPORTED(span.subspan(4), "");
+  EXPECT_DEATH_IF_SUPPORTED(span.first(4), "");
+  EXPECT_DEATH_IF_SUPPORTED(span.last(4), "");
+  // Accessing an empty span.
+  Span<const int> empty(v, 0);
+  EXPECT_DEATH_IF_SUPPORTED(empty[0], "");
+  EXPECT_DEATH_IF_SUPPORTED(empty.front(), "");
+  EXPECT_DEATH_IF_SUPPORTED(empty.back(), "");
+}
+
 }  // namespace
 BSSL_NAMESPACE_END

ssl/ssl_test.cc

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index b21c798..a51b594 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -567,6 +567,13 @@
   return true;
 }
 
+TEST(ArrayDeathTest, BoundsChecks) {
+  Array<int> array;
+  const int v[] = {1, 2, 3, 4};
+  ASSERT_TRUE(array.CopyFrom(v));
+  EXPECT_DEATH_IF_SUPPORTED(array[4], "");
+}
+
 TEST(VectorTest, Resize) {
   Vector<size_t> vec;
   ASSERT_TRUE(vec.empty());
@@ -663,6 +670,15 @@
   EXPECT_EQ(3u, vec[3].array.size());
 }
 
+TEST(VectorDeathTest, BoundsChecks) {
+  Vector<int> vec;
+  ASSERT_TRUE(vec.Push(1));
+  // Within bounds of the capacity, but not the vector.
+  EXPECT_DEATH_IF_SUPPORTED(vec[1], "");
+  // Not within bounds of the capacity either.
+  EXPECT_DEATH_IF_SUPPORTED(vec[10000], "");
+}
+
 TEST(ReconstructSeqnumTest, Increment) {
   // Test simple cases from the beginning of an epoch with both 8- and 16-bit
   // wire sequence numbers.