Merge pull request #158 from jwmcglynn/bounds-checking

Add checks for boundary conditions for malformed glTF files

.gitignore

@@ -67,4 +67,5 @@ imgui.ini
 loader_example
 tests/tester
 tests/tester_noexcept
+tests/issue-97.gltf
 

.travis.yml

@@ -42,6 +42,10 @@ script:
   - ${CC} -v
   - ${CXX} ${EXTRA_CXXFLAGS} -std=c++11 -Wall -g -o loader_example loader_example.cc
   - ./loader_example ./models/Cube/Cube.gltf
-  - cd examples/raytrace
+  - cd tests
+  - make
+  - ./tester
+  - ./tester_noexcept
+  - cd ../examples/raytrace
   - ../../premake5 gmake
   - make

models/BoundsChecking/invalid-buffer-index.gltf

@@ -0,0 +1,50 @@
+{
+  "scenes": [],
+  "nodes": [],
+  "meshes": [
+    {
+      "primitives": [
+        {
+          "attributes": {},
+          "indices": 0
+        }
+      ]
+    }
+  ],
+  "buffers": [
+    {
+      "uri": "simpleTriangle.bin",
+      "byteLength": 44
+    }
+  ],
+  "bufferViews": [
+    {
+      "buffer": 0,
+      "byteOffset": 0,
+      "byteLength": 6,
+      "target": 34963
+    },
+    {
+      "buffer": 1,
+      "byteOffset": 0,
+      "byteLength": 6,
+      "target": 34963
+    }
+  ],
+  "images": [
+    {
+      "bufferView": 1,
+      "mimeType": "image/png"
+    }
+  ],
+  "accessors": [
+    {
+      "bufferView": 0,
+      "componentType": 5123,
+      "count": 3,
+      "type": "SCALAR",
+      "max": [2],
+      "min": [0]
+    }
+  ]
+}

models/BoundsChecking/invalid-buffer-view-index.gltf

@@ -0,0 +1,33 @@
+{
+  "scenes": [],
+  "nodes": [],
+  "buffers": [],
+  "meshes": [
+    {
+      "primitives": [
+        {
+          "attributes": {},
+          "indices": 0
+        }
+      ]
+    }
+  ],
+  "bufferViews": [
+    {
+      "buffer": 0,
+      "byteOffset": 0,
+      "byteLength": 6,
+      "target": 34963
+    }
+  ],
+  "accessors": [
+    {
+      "bufferView": 1,
+      "componentType": 5123,
+      "count": 3,
+      "type": "SCALAR",
+      "max": [2],
+      "min": [0]
+    }
+  ]
+}

models/BoundsChecking/invalid-primitive-indices.gltf

@@ -0,0 +1,33 @@
+{
+  "scenes": [],
+  "nodes": [],
+  "buffers": [],
+  "meshes": [
+    {
+      "primitives": [
+        {
+          "attributes": {},
+          "indices": 1
+        }
+      ]
+    }
+  ],
+  "bufferViews": [
+    {
+      "buffer": 0,
+      "byteOffset": 0,
+      "byteLength": 6,
+      "target": 34963
+    }
+  ],
+  "accessors": [
+    {
+      "bufferView": 1,
+      "componentType": 5123,
+      "count": 3,
+      "type": "SCALAR",
+      "max": [2],
+      "min": [0]
+    }
+  ]
+}

tests/tester.cc

@@ -82,7 +82,69 @@ TEST_CASE("extension-with-empty-object", "[issue-97]") {
     REQUIRE(m.materials[0].extensions.size() == 1);
     REQUIRE(m.materials[0].extensions.count("VENDOR_material_some_ext") == 1);
   }
-    
+
 }
 
+TEST_CASE("invalid-primitive-indices", "[bounds-checking]") {
+  tinygltf::Model model;
+  tinygltf::TinyGLTF ctx;
+  std::string err;
+  std::string warn;
 
+  // Loading is expected to fail, but not crash.
+  bool ret = ctx.LoadASCIIFromFile(
+      &model, &err, &warn,
+      "../models/BoundsChecking/invalid-primitive-indices.gltf");
+  REQUIRE_THAT(err,
+               Catch::Contains("primitive indices accessor out of bounds"));
+  REQUIRE_FALSE(ret);
+}
+
+TEST_CASE("invalid-buffer-view-index", "[bounds-checking]") {
+  tinygltf::Model model;
+  tinygltf::TinyGLTF ctx;
+  std::string err;
+  std::string warn;
+
+  // Loading is expected to fail, but not crash.
+  bool ret = ctx.LoadASCIIFromFile(
+      &model, &err, &warn,
+      "../models/BoundsChecking/invalid-buffer-view-index.gltf");
+  REQUIRE_THAT(err, Catch::Contains("accessor[0] invalid bufferView"));
+  REQUIRE_FALSE(ret);
+}
+
+TEST_CASE("invalid-buffer-index", "[bounds-checking]") {
+  tinygltf::Model model;
+  tinygltf::TinyGLTF ctx;
+  std::string err;
+  std::string warn;
+
+  // Loading is expected to fail, but not crash.
+  bool ret = ctx.LoadASCIIFromFile(
+      &model, &err, &warn,
+      "../models/BoundsChecking/invalid-buffer-index.gltf");
+  REQUIRE_THAT(
+      err, Catch::Contains("image[0] buffer \"1\" not found in the scene."));
+  REQUIRE_FALSE(ret);
+}
+
+TEST_CASE("glb-invalid-length", "[bounds-checking]") {
+  tinygltf::Model model;
+  tinygltf::TinyGLTF ctx;
+  std::string err;
+  std::string warn;
+
+  // This glb has a much longer length than the provided data and should fail
+  // initial range checks.
+  const unsigned char glb_invalid_length[] = "glTF"
+      "\x20\x00\x00\x00" "\x6c\x66\x00\x00"     //
+  //  |     version     |     length      |
+      "\x02\x00\x00\x00" "\x4a\x53\x4f\x4e{}";  //
+  //  |  model length   |   model format  |
+
+  bool ret = ctx.LoadBinaryFromMemory(&model, &err, &warn, glb_invalid_length,
+                                      sizeof(glb_invalid_length));
+  REQUIRE_THAT(err, Catch::Contains("Invalid glTF binary."));
+  REQUIRE_FALSE(ret);
+}

tiny_gltf.h

@@ -3926,8 +3926,24 @@ bool TinyGLTF::LoadFromString(Model *model, std::string *err, std::string *warn,
       if (primitive.indices >
           -1)  // has indices from parsing step, must be Element Array Buffer
       {
-        model->bufferViews[model->accessors[primitive.indices].bufferView]
-            .target = TINYGLTF_TARGET_ELEMENT_ARRAY_BUFFER;
+        if (size_t(primitive.indices) >= model->accessors.size()) {
+          if (err) {
+            (*err) += "primitive indices accessor out of bounds";
+          }
+          return false;
+        }
+
+        auto bufferView = model->accessors[primitive.indices].bufferView;
+        if (bufferView < 0 || size_t(bufferView) >= model->bufferViews.size()) {
+          if (err) {
+            (*err) += "accessor[" + std::to_string(primitive.indices) +
+                      "] invalid bufferView";
+          }
+          return false;
+        }
+
+        model->bufferViews[bufferView].target =
+            TINYGLTF_TARGET_ELEMENT_ARRAY_BUFFER;
         // we could optionally check if acessors' bufferView type is Scalar, as
         // it should be
       }
@@ -4080,6 +4096,15 @@ bool TinyGLTF::LoadFromString(Model *model, std::string *err, std::string *warn,
 
           const BufferView &bufferView =
               model->bufferViews[size_t(image.bufferView)];
+          if (size_t(bufferView.buffer) >= model->buffers.size()) {
+            if (err) {
+              std::stringstream ss;
+              ss << "image[" << idx << "] buffer \"" << bufferView.buffer
+                 << "\" not found in the scene." << std::endl;
+              (*err) += ss.str();
+            }
+            return false;
+          }
           const Buffer &buffer = model->buffers[size_t(bufferView.buffer)];
 
           if (*LoadImageData == nullptr) {
@@ -4371,7 +4396,7 @@ bool TinyGLTF::LoadBinaryFromMemory(Model *model, std::string *err,
   // In case the Bin buffer is not present, the size is exactly 20 + size of
   // JSON contents,
   // so use "greater than" operator.
-  if ((20 + model_length > size) || (model_length < 1) ||
+  if ((20 + model_length > size) || (model_length < 1) || (length > size) ||
       (model_format != 0x4E4F534A)) {  // 0x4E4F534A = JSON format.
     if (err) {
       (*err) = "Invalid glTF binary.";