ObjGeometryLoader: Fix out of bound error

Apparently this was a major security issue reported to security.qt-project.org

Change-Id: Id52f035134ca6111e24b5820eb1b64b99449e47f
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

7 files changed, 124 insertions, 8 deletions

diff --git a/src/plugins/geometryloaders/default/objgeometryloader.cpp b/src/plugins/geometryloaders/default/objgeometryloader.cpp
index 6ebe91a09..b144ceacd 100644
--- a/src/plugins/geometryloaders/default/objgeometryloader.cpp
+++ b/src/plugins/geometryloaders/default/objgeometryloader.cpp
@@ -61,8 +61,6 @@ inline uint qHash(const FaceIndices &faceIndices)
 
 bool ObjGeometryLoader::doLoad(QIODevice *ioDev, const QString &subMesh)
 {
-    int faceCount = 0;
-
     // Parse faces taking into account each vertex in a face can index different indices
     // for the positions, normals and texture coords;
     // Generate unique vertices (in OpenGL parlance) and output to points, texCoords,
@@ -150,8 +148,6 @@ bool ObjGeometryLoader::doLoad(QIODevice *ioDev, const QString &subMesh)
                 }
             } else if (!skipping && tokens.size() >= 4 && qstrncmp(tokens.charPtrAt(0), "f ", 2) == 0) {
                 // Process face
-                ++faceCount;
-
                 int faceVertices = tokens.size() - 1;
 
                 QVarLengthArray<FaceIndices, 4> face; // try to avoid allocations in the common case of triangulated data
@@ -224,11 +220,15 @@ bool ObjGeometryLoader::doLoad(QIODevice *ioDev, const QString &subMesh)
         m_normals.resize(vertexCount);
 
     for (auto it = faceIndexMap.cbegin(), endIt = faceIndexMap.cend(); it != endIt; ++it) {
-        m_points[it.value()] = positions[it.key().positionIndex];
+        const uint positionIndex = it.key().positionIndex;
+        const uint texCoordIndex = it.key().texCoordIndex;
+        const uint normalIndex = it.key().normalIndex;
+
+        m_points[it.value()] = (positionIndex < uint(positions.size())) ? positions[positionIndex] : QVector3D();
         if (hasTexCoords)
-            m_texCoords[it.value()] = std::numeric_limits<unsigned int>::max() != it.key().texCoordIndex ? texCoords[it.key().texCoordIndex] : QVector2D();
+            m_texCoords[it.value()] = (texCoordIndex < uint(texCoords.size())) ? texCoords[texCoordIndex] : QVector2D();
         if (hasNormals)
-            m_normals[it.value()] = normals[it.key().normalIndex];
+            m_normals[it.value()] = (normalIndex < uint(normals.size())) ? normals[normalIndex] : QVector3D();
     }
 
     // Now iterate over the face indices and lookup the unique vertex index
diff --git a/tests/auto/auto.pro b/tests/auto/auto.pro
index cc7e5590a..61c704389 100644
--- a/tests/auto/auto.pro
+++ b/tests/auto/auto.pro
@@ -8,7 +8,8 @@ SUBDIRS = \
     cmake \
     input \
     animation \
-    extras
+    extras \
+    geometryloaders
 
 installed_cmake.depends = cmake
 
diff --git a/tests/auto/geometryloaders/geometryloaders.pro b/tests/auto/geometryloaders/geometryloaders.pro
new file mode 100644
index 000000000..dd9e0df3e
--- /dev/null
+++ b/tests/auto/geometryloaders/geometryloaders.pro
@@ -0,0 +1,4 @@
+TEMPLATE = subdirs
+
+SUBDIRS = \
+    objgeometryloader
diff --git a/tests/auto/geometryloaders/objgeometryloader/invalid_vertex_position.obj b/tests/auto/geometryloaders/objgeometryloader/invalid_vertex_position.obj
new file mode 100644
index 000000000..c9445ff17
--- /dev/null
+++ b/tests/auto/geometryloaders/objgeometryloader/invalid_vertex_position.obj
@@ -0,0 +1,2 @@
+v 0 0 0
+f 1 10 100 1000 100000 1000000 10000000 100000000
diff --git a/tests/auto/geometryloaders/objgeometryloader/objgeometryloader.pro b/tests/auto/geometryloaders/objgeometryloader/objgeometryloader.pro
new file mode 100644
index 000000000..cb098a221
--- /dev/null
+++ b/tests/auto/geometryloaders/objgeometryloader/objgeometryloader.pro
@@ -0,0 +1,16 @@
+TEMPLATE = app
+
+TARGET = tst_objgeometryloader
+
+QT += 3dcore 3dcore-private 3drender 3drender-private testlib
+
+CONFIG += testcase
+
+SOURCES += \
+    tst_objgeometryloader.cpp
+
+OTHER_FILES += \
+    invalid_vertex_position.obj
+
+RESOURCES += \
+    resources.qrc
diff --git a/tests/auto/geometryloaders/objgeometryloader/resources.qrc b/tests/auto/geometryloaders/objgeometryloader/resources.qrc
new file mode 100644
index 000000000..00d9a2590
--- /dev/null
+++ b/tests/auto/geometryloaders/objgeometryloader/resources.qrc
@@ -0,0 +1,5 @@
+<RCC>
+    <qresource prefix="/">
+        <file>invalid_vertex_position.obj</file>
+    </qresource>
+</RCC>
diff --git a/tests/auto/geometryloaders/objgeometryloader/tst_objgeometryloader.cpp b/tests/auto/geometryloaders/objgeometryloader/tst_objgeometryloader.cpp
new file mode 100644
index 000000000..dfd26bdd2
--- /dev/null
+++ b/tests/auto/geometryloaders/objgeometryloader/tst_objgeometryloader.cpp
@@ -0,0 +1,88 @@
+/****************************************************************************
+**
+** Copyright (C) 2018 Klaralvdalens Datakonsult AB (KDAB).
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl-3.0.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or (at your option) the GNU General
+** Public license version 3 or any later version approved by the KDE Free
+** Qt Foundation. The licenses are as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-2.0.html and
+** https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include <QtTest/QTest>
+#include <QtCore/private/qfactoryloader_p.h>
+#include <Qt3DRender/private/qgeometryloaderinterface_p.h>
+#include <Qt3DRender/private/qgeometryloaderfactory_p.h>
+
+using namespace Qt3DRender;
+
+class tst_ObjGeometryLoader : public QObject
+{
+    Q_OBJECT
+private Q_SLOTS:
+
+    void checkOutOfBoundFaceAccess_data()
+    {
+        QTest::addColumn<QString>("filePath");
+
+        QTest::newRow("invalid position index") << ":invalid_vertex_position.obj";
+    }
+
+
+    void checkOutOfBoundFaceAccess()
+    {
+        // GIVEN
+        QFETCH(QString, filePath);
+
+        QFactoryLoader geometryLoader(QGeometryLoaderFactory_iid,
+                                      QLatin1String("/geometryloaders"),
+                                      Qt::CaseInsensitive);
+
+        QScopedPointer<QGeometryLoaderInterface> loader;
+        loader.reset(qLoadPlugin<QGeometryLoaderInterface, QGeometryLoaderFactory>(&geometryLoader, QLatin1String("obj")));
+
+        if (!loader)
+            QSKIP("ObjLoaderPlugin not deployed");
+
+        QFile file(filePath);
+        QVERIFY(file.open(QIODevice::ReadOnly));
+
+        // WHEN
+        loader->load(&file);
+
+        // THEN
+        // -> shouldn't crash
+    }
+};
+
+QTEST_MAIN(tst_ObjGeometryLoader)
+
+#include "tst_objgeometryloader.moc"