CVE-2015-6781: Numeric Errors (CWE-189)
value: Integer overflow in the FontData::Bound function in data/font_data.cc in Google sfntly, as used in Google Chrome before 47.0.2526.73, allows remote attackers to cause a denial of service or possibly have unspecified other impact via a crafted offset or length value within font data in an SFNT container.
CWE description: Weaknesses in this category are related to improper calculation or conversion of numbers.
Published: 2015-12-06T01:59Z
Repository: https://github.com/googlei18n/sfntly
Language: C
Files changed: cpp/src/sfntly/data/font_data.cc, cpp/src/sfntly/data/font_data.h, cpp/src/sfntly/port/logging.h
Commit date: 2015-09-23 16:06:28 -0700
Commit message: Check for integer overflow in sfntly::FontData::Bound().

Also delete dead code and cleanup some nits.

This is cl/96914065.

Diff: diff --git a/cpp/src/sfntly/data/font_data.cc b/cpp/src/sfntly/data/font_data.cc
index d2b95ea..95bee3e 100644
--- a/cpp/src/sfntly/data/font_data.cc
+++ b/cpp/src/sfntly/data/font_data.cc
@@ -12,37 +12,47 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
-#include <limits.h>
+#include "sfntly/data/font_data.h"
+
 #include <algorithm>
 #include <functional>
+#include <limits>
 
-#include "sfntly/data/font_data.h"
+#include "sfntly/port/logging.h"
 
 namespace sfntly {
 
 int32_t FontData::Size() const {
   return std::min<int32_t>(array_->Size() - bound_offset_, bound_length_);
 }
 
-bool FontData::Bound(int32_t offset, int32_t length) {
-  if (offset + length > Size() || offset < 0 || length < 0)
-    return false;
-
-  bound_offset_ += offset;
+void FontData::Bound(int32_t offset, int32_t length) {
+  // Inputs should not be negative.
+  CHECK(offset >= 0);
+  CHECK(length >= 0);
+
+  // Check to make sure |bound_offset_| will not overflow.
+  CHECK(bound_offset_ <= std::numeric_limits<int32_t>::max() - offset);
+  const int32_t new_offset = bound_offset_ + offset;
+
+  if (length == GROWABLE_SIZE) {
+    // When |length| has the special value of GROWABLE_SIZE, it means the size
+    // should not have any artificial limits, thus it is just the underlying
+    // |array_|'s size. Just make sure |new_offset| is still within bounds.
+    CHECK(new_offset <= array_->Size());
+  } else {
+    // When |length| has any other value, |new_offset| + |length| points to the
+    // end of the array. Make sure that is within bounds, but use subtraction to
+    // avoid an integer overflow.
+    CHECK(new_offset <= array_->Size() - length);
+  }
+
+  bound_offset_ = new_offset;
   bound_length_ = length;
-  return true;
-}
-
-bool FontData::Bound(int32_t offset) {
-if (offset > Size() || offset < 0)
-    return false;
-
-  bound_offset_ += offset;
-  return true;
 }
 
 int32_t FontData::Length() const {
   return std::min<int32_t>(array_->Length() - bound_offset_, bound_length_);
 }
@@ -58,11 +68,11 @@ FontData::FontData(FontData* data, int32_t offset, int32_t length) {
 
 FontData::FontData(FontData* data, int32_t offset) {
   Init(data->array_);
   Bound(data->bound_offset_ + offset,
         (data->bound_length_ == GROWABLE_SIZE)
-            ? GROWABLE_SIZE : data->bound_length_ - offset);
+        ? GROWABLE_SIZE : data->bound_length_ - offset);
 }
 
 FontData::~FontData() {}
 
 void FontData::Init(ByteArray* ba) {
diff --git a/cpp/src/sfntly/data/font_data.h b/cpp/src/sfntly/data/font_data.h
index d02e8b7..e0e7e79 100644
--- a/cpp/src/sfntly/data/font_data.h
+++ b/cpp/src/sfntly/data/font_data.h
@@ -17,15 +17,13 @@
 #ifndef SFNTLY_CPP_SRC_SFNTLY_DATA_FONT_DATA_H_
 #define SFNTLY_CPP_SRC_SFNTLY_DATA_FONT_DATA_H_
 
 #include <limits.h>
 
-#include <vector>
-
-#include "sfntly/port/type.h"
 #include "sfntly/data/byte_array.h"
 #include "sfntly/port/refcount.h"
+#include "sfntly/port/type.h"
 
 namespace sfntly {
 
 struct DataSize {
   enum {
@@ -58,20 +56,11 @@ class FontData : virtual public RefCount {
 
   // Sets limits on the size of the FontData. The FontData is then only
   // visible within the bounds set.
   // @param offset the start of the new bounds
   // @param length the number of bytes in the bounded array
-  // @return true if the bounding range was successful; false otherwise
-  virtual bool Bound(int32_t offset, int32_t length);
-
-  // Sets limits on the size of the FontData. This is a offset bound only so if
-  // the FontData is writable and growable then there is no limit to that growth
-  // from the bounding operation.
-  // @param offset the start of the new bounds which must be within the current
-  //        size of the FontData
-  // @return true if the bounding range was successful; false otherwise
-  virtual bool Bound(int32_t offset);
+  virtual void Bound(int32_t offset, int32_t length);
 
   // Makes a slice of this FontData. The returned slice will share the data with
   // the original <code>FontData</code>.
   // @param offset the start of the slice
   // @param length the number of bytes in the slice
diff --git a/cpp/src/sfntly/port/logging.h b/cpp/src/sfntly/port/logging.h
new file mode 100644
index 0000000..1d9e319
--- /dev/null
+++ b/cpp/src/sfntly/port/logging.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2015 Google Inc. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
+#define SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
+
+#include <stdio.h>
+#include <stdlib.h>
+
+// Cheap base/logging.h knock off.
+
+#define CHECK(expr) \
+    if (!(expr)) { \
+      printf("CHECK failed\n"); \
+      abort(); \
+    }
+
+#endif  // SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_

Vulnerable code: #include <limits.h>
#include "sfntly/data/font_data.h"
bool FontData::Bound(int32_t offset, int32_t length) {
  if (offset + length > Size() || offset < 0 || length < 0)
    return false;

  bound_offset_ += offset;
  return true;
}

bool FontData::Bound(int32_t offset) {
if (offset > Size() || offset < 0)
    return false;

  bound_offset_ += offset;
  return true;
            ? GROWABLE_SIZE : data->bound_length_ - offset);
#include <vector>

#include "sfntly/port/type.h"
  // @return true if the bounding range was successful; false otherwise
  virtual bool Bound(int32_t offset, int32_t length);

  // Sets limits on the size of the FontData. This is a offset bound only so if
  // the FontData is writable and growable then there is no limit to that growth
  // from the bounding operation.
  // @param offset the start of the new bounds which must be within the current
  //        size of the FontData
  // @return true if the bounding range was successful; false otherwise
  virtual bool Bound(int32_t offset);
Fixed code: #include "sfntly/data/font_data.h"

#include <limits>
#include "sfntly/port/logging.h"
void FontData::Bound(int32_t offset, int32_t length) {
  // Inputs should not be negative.
  CHECK(offset >= 0);
  CHECK(length >= 0);

  // Check to make sure |bound_offset_| will not overflow.
  CHECK(bound_offset_ <= std::numeric_limits<int32_t>::max() - offset);
  const int32_t new_offset = bound_offset_ + offset;

  if (length == GROWABLE_SIZE) {
    // When |length| has the special value of GROWABLE_SIZE, it means the size
    // should not have any artificial limits, thus it is just the underlying
    // |array_|'s size. Just make sure |new_offset| is still within bounds.
    CHECK(new_offset <= array_->Size());
  } else {
    // When |length| has any other value, |new_offset| + |length| points to the
    // end of the array. Make sure that is within bounds, but use subtraction to
    // avoid an integer overflow.
    CHECK(new_offset <= array_->Size() - length);
  }

  bound_offset_ = new_offset;
        ? GROWABLE_SIZE : data->bound_length_ - offset);
#include "sfntly/port/type.h"
  virtual void Bound(int32_t offset, int32_t length);
/*
 * Copyright 2015 Google Inc. All Rights Reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifndef SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
#define SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_

#include <stdio.h>
#include <stdlib.h>

// Cheap base/logging.h knock off.

#define CHECK(expr) \
    if (!(expr)) { \
      printf("CHECK failed\n"); \
      abort(); \
    }

#endif  // SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_