From 9446f65667b6e7a8b3927dad8951f86cbcb3de9f Mon Sep 17 00:00:00 2001 From: Jeff McGlynn Date: Thu, 25 Apr 2019 16:26:26 -0700 Subject: [PATCH 1/2] Set up tests to run in travis-ci and ignore test outputs in .gitignore --- .gitignore | 1 + .travis.yml | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 9234d82..879c4cd 100644 --- a/.gitignore +++ b/.gitignore @@ -67,4 +67,5 @@ imgui.ini loader_example tests/tester tests/tester_noexcept +tests/issue-97.gltf diff --git a/.travis.yml b/.travis.yml index 37779f1..558702d 100644 --- a/.travis.yml +++ b/.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 From 8915252407dfbf9800963e8685392a2d51cc3e69 Mon Sep 17 00:00:00 2001 From: Jeff McGlynn Date: Thu, 25 Apr 2019 16:33:56 -0700 Subject: [PATCH 2/2] Add checks for boundary conditions for malformed glTF files When loading untrusted glTF files, ideally an error should be returned if the file is malformed instead of an exception/crash. Add additional validation for crashes found when running tinygltf under a fuzzer, and add test cases to confirm: 1. Validate that the primitive indices value is within the model->accessors bounds before dereferencing. 2. Validate that the accessors bufferView index if valid. 3. Validate that the buffer's index is valid when parsing images. 4. For glb files, validate that the overall length is within the provided input buffer. --- .../BoundsChecking/invalid-buffer-index.gltf | 50 ++++++++++++++ .../invalid-buffer-view-index.gltf | 33 +++++++++ .../invalid-primitive-indices.gltf | 33 +++++++++ models/BoundsChecking/simpleTriangle.bin | Bin 0 -> 44 bytes tests/tester.cc | 64 +++++++++++++++++- tiny_gltf.h | 31 ++++++++- 6 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 models/BoundsChecking/invalid-buffer-index.gltf create mode 100644 models/BoundsChecking/invalid-buffer-view-index.gltf create mode 100644 models/BoundsChecking/invalid-primitive-indices.gltf create mode 100644 models/BoundsChecking/simpleTriangle.bin diff --git a/models/BoundsChecking/invalid-buffer-index.gltf b/models/BoundsChecking/invalid-buffer-index.gltf new file mode 100644 index 0000000..88386ef --- /dev/null +++ b/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] + } + ] +} diff --git a/models/BoundsChecking/invalid-buffer-view-index.gltf b/models/BoundsChecking/invalid-buffer-view-index.gltf new file mode 100644 index 0000000..687fe85 --- /dev/null +++ b/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] + } + ] +} diff --git a/models/BoundsChecking/invalid-primitive-indices.gltf b/models/BoundsChecking/invalid-primitive-indices.gltf new file mode 100644 index 0000000..6dab589 --- /dev/null +++ b/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] + } + ] +} diff --git a/models/BoundsChecking/simpleTriangle.bin b/models/BoundsChecking/simpleTriangle.bin new file mode 100644 index 0000000000000000000000000000000000000000..d642500b9bf7c2f30d58d48514abd23345dcd9cd GIT binary patch literal 44 UcmZQzU}RuoKnD%>s3H(P02K@Yf&c&j literal 0 HcmV?d00001 diff --git a/tests/tester.cc b/tests/tester.cc index bb29e18..a07c549 100644 --- a/tests/tester.cc +++ b/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); +} diff --git a/tiny_gltf.h b/tiny_gltf.h index 3b507a6..7f2e239 100644 --- a/tiny_gltf.h +++ b/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.";