Bug 1018033: Prevent buffer read overflow due to integer overflow in …

…mozilla::pkix::der::Input::EnsureLength, r=keeler

--HG--
extra : rebase_source : e4e88d61e448fa475a106a06b9f32181906fba0f

lib/mozpkix/lib/pkixder.h

@@ -228,7 +228,7 @@ class Input
 
   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;

lib/mozpkix/test/gtest/pkixder_input_tests.cpp

@@ -216,6 +216,23 @@ TEST_F(pkixder_input_tests, ReadBytePastEnd)
   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;
@@ -259,6 +276,23 @@ TEST_F(pkixder_input_tests, ReadWordWithInsufficentData)
   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;
@@ -351,6 +385,22 @@ TEST_F(pkixder_input_tests, InputSkipToSECItem)
   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;