Improve the validation algorithm for binary JSON

Add better boundary checks and catch (hopefully all)
cases where invalid binary JSON could cause crashes.

Change-Id: I206510b7c5e3ba953802a5f46645878e65704ecc
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

src/corelib/json/qjson.cpp

@@ -129,10 +129,12 @@ bool Data::valid() const
         return false;
 
     bool res = false;
-    if (header->root()->is_object)
+    Base *root = header->root();
-        res = static_cast<Object *>(header->root())->isValid();
+    int maxSize = alloc - sizeof(Header);
+    if (root->is_object)
+        res = static_cast<Object *>(root)->isValid(maxSize);
     else
-        res = static_cast<Array *>(header->root())->isValid();
+        res = static_cast<Array *>(root)->isValid(maxSize);
 
     return res;
 }
@@ -195,9 +197,9 @@ int Object::indexOf(const QString &key, bool *exists)
     return min;
 }
 
-bool Object::isValid() const
+bool Object::isValid(int maxSize) const
 {
-    if (tableOffset + length*sizeof(offset) > size)
+    if (size > (uint)maxSize || tableOffset + length*sizeof(offset) > size)
         return false;
 
     QString lastKey;
@@ -206,8 +208,7 @@ bool Object::isValid() const
         if (entryOffset + sizeof(Entry) >= tableOffset)
             return false;
         Entry *e = entryAt(i);
-        int s = e->size();
+        if (!e->isValid(tableOffset - table()[i]))
-        if (table()[i] + s > tableOffset)
             return false;
         QString key = e->key();
         if (key < lastKey)
@@ -221,9 +222,9 @@ bool Object::isValid() const
 
 
 
-bool Array::isValid() const
+bool Array::isValid(int maxSize) const
 {
-    if (tableOffset + length*sizeof(offset) > size)
+    if (size > (uint)maxSize || tableOffset + length*sizeof(offset) > size)
         return false;
 
     for (uint i = 0; i < length; ++i) {
@@ -323,12 +324,12 @@ bool Value::isValid(const Base *b) const
     int s = usedStorage(b);
     if (!s)
         return true;
-    if (s < 0 || offset + s > (int)b->tableOffset)
+    if (s < 0 || s > (int)b->tableOffset - offset)
         return false;
     if (type == QJsonValue::Array)
-        return static_cast<Array *>(base(b))->isValid();
+        return static_cast<Array *>(base(b))->isValid(s);
     if (type == QJsonValue::Object)
-        return static_cast<Object *>(base(b))->isValid();
+        return static_cast<Object *>(base(b))->isValid(s);
     return true;
 }
 

src/corelib/json/qjson_p.h

@@ -302,12 +302,19 @@ public:
     String(const char *data) { d = (Data *)data; }
 
     struct Data {
-        qle_int length;
+        qle_uint length;
         qle_ushort utf16[1];
     };
 
     Data *d;
 
+    int byteSize() const { return sizeof(uint) + sizeof(ushort) * d->length; }
+    bool isValid(int maxSize) const {
+        // Check byteSize() <= maxSize, avoiding integer overflow
+        maxSize -= sizeof(uint);
+        return maxSize >= 0 && uint(d->length) <= maxSize / sizeof(ushort);
+    }
+
     inline String &operator=(const QString &str)
     {
         d->length = str.length();
@@ -376,11 +383,16 @@ public:
     Latin1String(const char *data) { d = (Data *)data; }
 
     struct Data {
-        qle_short length;
+        qle_ushort length;
         char latin1[1];
     };
     Data *d;
 
+    int byteSize() const { return sizeof(ushort) + sizeof(char)*(d->length); }
+    bool isValid(int maxSize) const {
+        return byteSize() <= maxSize;
+    }
+
     inline Latin1String &operator=(const QString &str)
     {
         int len = d->length = str.length();
@@ -567,7 +579,7 @@ public:
     }
     int indexOf(const QString &key, bool *exists);
 
-    bool isValid() const;
+    bool isValid(int maxSize) const;
 };
 
 
@@ -577,7 +589,7 @@ public:
     inline Value at(int i) const;
     inline Value &operator [](int i);
 
-    bool isValid() const;
+    bool isValid(int maxSize) const;
 };
 
 
@@ -631,12 +643,12 @@ public:
     // key
     // value data follows key
 
-    int size() const {
+    uint size() const {
         int s = sizeof(Entry);
         if (value.latinKey)
-            s += sizeof(ushort) + qFromLittleEndian(*(ushort *) ((const char *)this + sizeof(Entry)));
+            s += shallowLatin1Key().byteSize();
         else
-            s += sizeof(uint) + sizeof(ushort)*qFromLittleEndian(*(int *) ((const char *)this + sizeof(Entry)));
+            s += shallowKey().byteSize();
         return alignedSize(s);
     }
 
@@ -662,6 +674,15 @@ public:
         return shallowKey().toString();
     }
 
+    bool isValid(int maxSize) const {
+        if (maxSize < (int)sizeof(Entry))
+            return false;
+        maxSize -= sizeof(Entry);
+        if (value.latinKey)
+            return shallowLatin1Key().isValid(maxSize);
+        return shallowKey().isValid(maxSize);
+    }
+
     bool operator ==(const QString &key) const;
     inline bool operator !=(const QString &key) const { return !operator ==(key); }
     inline bool operator >=(const QString &key) const;

tests/auto/corelib/json/tst_qtjson.cpp

@@ -103,6 +103,7 @@ private Q_SLOTS:
     void fromBinary();
     void toAndFromBinary_data();
     void toAndFromBinary();
+    void invalidBinaryData();
     void parseNumbers();
     void parseStrings();
     void parseDuplicateKeys();
@@ -1779,6 +1780,21 @@ void tst_QtJson::toAndFromBinary()
     QVERIFY(doc == outdoc);
 }
 
+void tst_QtJson::invalidBinaryData()
+{
+    QDir dir(testDataDir + "/invalidBinaryData");
+    QFileInfoList files = dir.entryInfoList();
+    for (int i = 0; i < files.size(); ++i) {
+        if (!files.at(i).isFile())
+            continue;
+        QFile file(files.at(i).filePath());
+        file.open(QIODevice::ReadOnly);
+        QByteArray bytes = file.readAll();
+        QJsonDocument document = QJsonDocument::fromRawData(bytes.constData(), bytes.size());
+        QVERIFY(document.isNull());
+    }
+}
+
 void tst_QtJson::parseNumbers()
 {
     {