Bug 1018033 - Prevent buffer read overflow due to integer overflow in mozilla::pkix::der::Input::EnsureLength. r=keeler, a=sledru
authorBrian Smith <brian@briansmith.org>
Thu, 29 May 2014 23:37:40 -0700
changeset 200452 de2314073bf288e8bf3f26f7a6f7b0c27b5df17d
parent 200451 6f561fd4e04525750c614a9c47a4a2dd908698e5
child 200453 12ed2a5094bed9f3b9f7040b25a9937a62495313
push id486
push userasasaki@mozilla.com
push dateMon, 14 Jul 2014 18:39:42 +0000
treeherdermozilla-release@d33428174ff1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler, sledru
bugs1018033
milestone31.0a2
Bug 1018033 - Prevent buffer read overflow due to integer overflow in mozilla::pkix::der::Input::EnsureLength. r=keeler, a=sledru
security/pkix/lib/pkixder.h
security/pkix/test/gtest/pkixder_input_tests.cpp
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -165,17 +165,17 @@ public:
 
   void SkipToEnd()
   {
     input = end;
   }
 
   Result EnsureLength(uint16_t len)
   {
-    if (input + len > end) {
+    if (static_cast<size_t>(end - input) < len) {
       return Fail(SEC_ERROR_BAD_DER);
     }
     return Success;
   }
 
   bool AtEnd() const { return input == end; }
 
   class Mark
--- a/security/pkix/test/gtest/pkixder_input_tests.cpp
+++ b/security/pkix/test/gtest/pkixder_input_tests.cpp
@@ -200,16 +200,33 @@ TEST_F(pkixder_input_tests, ReadBytePast
   ASSERT_EQ(0x11, readByte1);
 
   uint8_t readByte2 = 0;
   ASSERT_EQ(Failure, input.Read(readByte2));
   ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
   ASSERT_NE(0x22, readByte2);
 }
 
+TEST_F(pkixder_input_tests, ReadByteWrapAroundPointer)
+{
+  // The original implementation of our buffer read overflow checks was
+  // susceptible to integer overflows which could make the checks ineffective.
+  // This attempts to verify that we've fixed that. Unfortunately, decrementing
+  // a null pointer is undefined behavior according to the C++ language spec.,
+  // but this should catch the problem on at least some compilers, if not all of
+  // them.
+  const uint8_t* der = nullptr;
+  --der;
+  Input input;
+  ASSERT_EQ(Success, input.Init(der, 0));
+  uint8_t b;
+  ASSERT_EQ(Failure, input.Read(b));
+  ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
+}
+
 TEST_F(pkixder_input_tests, ReadWord)
 {
   Input input;
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
   ASSERT_EQ(Success, input.Init(der, sizeof der));
 
   uint16_t readWord1 = 0;
   ASSERT_EQ(Success, input.Read(readWord1));
@@ -243,16 +260,33 @@ TEST_F(pkixder_input_tests, ReadWordWith
   const uint8_t der[] = { 0x11, 0x22 };
   ASSERT_EQ(Success, input.Init(der, 1));
 
   uint16_t readWord1 = 0;
   ASSERT_EQ(Failure, input.Read(readWord1));
   ASSERT_NE(0x1122, readWord1);
 }
 
+TEST_F(pkixder_input_tests, ReadWordWrapAroundPointer)
+{
+  // The original implementation of our buffer read overflow checks was
+  // susceptible to integer overflows which could make the checks ineffective.
+  // This attempts to verify that we've fixed that. Unfortunately, decrementing
+  // a null pointer is undefined behavior according to the C++ language spec.,
+  // but this should catch the problem on at least some compilers, if not all of
+  // them.
+  const uint8_t* der = nullptr;
+  --der;
+  Input input;
+  ASSERT_EQ(Success, input.Init(der, 0));
+  uint16_t b;
+  ASSERT_EQ(Failure, input.Read(b));
+  ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
+}
+
 TEST_F(pkixder_input_tests, InputSkip)
 {
   Input input;
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
   ASSERT_EQ(Success, input.Init(der, sizeof der));
 
   ASSERT_EQ(Success, input.Skip(1));
 
@@ -335,16 +369,32 @@ TEST_F(pkixder_input_tests, InputSkipToS
   SECItem item;
   ASSERT_EQ(Success, input.Skip(sizeof expectedItemData, item));
   ASSERT_EQ(siBuffer, item.type);
   ASSERT_EQ(sizeof expectedItemData, item.len);
   ASSERT_EQ(der, item.data);
   ASSERT_EQ(0, memcmp(item.data, expectedItemData, sizeof expectedItemData));
 }
 
+TEST_F(pkixder_input_tests, SkipWrapAroundPointer)
+{
+  // The original implementation of our buffer read overflow checks was
+  // susceptible to integer overflows which could make the checks ineffective.
+  // This attempts to verify that we've fixed that. Unfortunately, decrementing
+  // a null pointer is undefined behavior according to the C++ language spec.,
+  // but this should catch the problem on at least some compilers, if not all of
+  // them.
+  const uint8_t* der = nullptr;
+  --der;
+  Input input;
+  ASSERT_EQ(Success, input.Init(der, 0));
+  ASSERT_EQ(Failure, input.Skip(1));
+  ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
+}
+
 TEST_F(pkixder_input_tests, SkipToSECItemPastEnd)
 {
   Input input;
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
   ASSERT_EQ(Success, input.Init(der, sizeof der));
 
   SECItem skippedSECItem;
   ASSERT_EQ(Failure, input.Skip(sizeof der + 1, skippedSECItem));