Validation: fix out-of-bounds access when content ends in a string

We can only validate_number() if we know that we have a number to
validate in the first place. If we've reached the end of our string, the
content that follows is not necessarily a number (it could be a Break
byte). More importantly, we could reach the end of the buffer.

This issue was masked by the way we provided data to the parser. It
always came from read-only memory becausee of QByteArray::fromRawData(),
so valgrind never caught any issues. Using QByteArray directly wouldn't
have helped because it always inserts a terminating null byte, which
always validates as a correct number (unsigned 0) and fails to trigger
valgrind.

So we need to use malloc() directly to make Valgrind complain. And there
was already a test that did:

==26543== Invalid read of size 1
==26543==    at 0x483EA10: memmove (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26543==    by 0x43CEEA: read_bytes_unchecked (cborinternal_p.h:239)
==26543==    by 0x43CFEC: extract_number_checked (cborinternal_p.h:286)
==26543==    by 0x43D3E9: validate_number (cborvalidation.c:304)
==26543==    by 0x43DC7B: validate_value (cborvalidation.c:551)
==26543==    by 0x43DE8C: cbor_value_validate (cborvalidation.c:645)
==26543==    by 0x4328D2: tst_Parser::strictValidation() (tst_parser.cpp:1637)
==26543==    by 0x434632: tst_Parser::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (tst_parser.moc:291)
==26543==    by 0x4C0B36D: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.cpp:2310)
==26543==    by 0x48673E9: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.h:122)
==26543==    by 0x4860256: QTest::TestMethods::invokeTestOnData(int) const (qtestcase.cpp:922)
==26543==    by 0x4860D4B: QTest::TestMethods::invokeTest(int, char const*, QTest::WatchDog*) const (qtestcase.cpp:1121)
==26543==  Address 0x61c4db1 is 0 bytes after a block of size 1 alloc'd
==26543==    at 0x483777F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26543==    by 0x403891: ParserWrapper::allocateMemory(unsigned long) (tst_parser.cpp:181)
==26543==    by 0x436898: ParserWrapper::init(QByteArray const&) (tst_parser.cpp:126)
==26543==    by 0x432712: tst_Parser::strictValidation() (tst_parser.cpp:1634)
==26543==    by 0x434632: tst_Parser::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (tst_parser.moc:291)
==26543==    by 0x4C0B36D: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.cpp:2310)
==26543==    by 0x48673E9: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (qmetaobject.h:122)
==26543==    by 0x4860256: QTest::TestMethods::invokeTestOnData(int) const (qtestcase.cpp:922)
==26543==    by 0x4860D4B: QTest::TestMethods::invokeTest(int, char const*, QTest::WatchDog*) const (qtestcase.cpp:1121)
==26543==    by 0x4862083: QTest::TestMethods::invokeTests(QObject*) const (qtestcase.cpp:1465)
==26543==    by 0x4862C14: QTest::qRun() (qtestcase.cpp:1903)
==26543==    by 0x48626C3: QTest::qExec(QObject*, int, char**) (qtestcase.cpp:1792)
==26543==
PASS   : tst_Parser::strictValidation(bytearray-0)

This commit goes further and makes it so an out-of-bounds access will
cause a pagefault.

Fixes #156.

Signed-off-by: Thiago Macieira <[email protected]>

Author: thiagomacieira

src/cborvalidation.c

@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2017 Intel Corporation
+** Copyright (C) 2019 Intel Corporation
 **
 ** Permission is hereby granted, free of charge, to any person obtaining a copy
 ** of this software and associated documentation files (the "Software"), to deal
@@ -561,13 +561,17 @@ static CborError validate_value(CborValue *it, uint32_t flags, int recursionLeft
             return err;
 
         while (1) {
-            err = validate_number(it, type, flags);
+            CborValue next;
+            err = _cbor_value_get_string_chunk(it, &ptr, &n, &next);
             if (err)
                 return err;
+            if (ptr) {
+                err = validate_number(it, type, flags);
+                if (err)
+                    return err;
+            }
 
-            err = _cbor_value_get_string_chunk(it, &ptr, &n, it);
-            if (err)
-                return err;
+            *it = next;
             if (!ptr)
                 break;
 

tests/parser/tst_parser.cpp

@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2017 Intel Corporation
+** Copyright (C) 2019 Intel Corporation
 **
 ** Permission is hereby granted, free of charge, to any person obtaining a copy
 ** of this software and associated documentation files (the "Software"), to deal
@@ -23,12 +23,23 @@
 ****************************************************************************/
 
 #define _XOPEN_SOURCE 700
+#define  _DARWIN_C_SOURCE 1         /* need MAP_ANON */
 #include <QtTest>
 #include "cbor.h"
 #include <stdio.h>
 #include <stdarg.h>
 
+#if defined(Q_OS_UNIX)
+#  include <sys/mman.h>
+#  include <unistd.h>
+#elif defined(Q_OS_WIN)
+#  define WIN32_LEAN_AND_MEAN 1
+#  define NOMINMAX 1
+#  include <windows.h>
+#endif
+
 Q_DECLARE_METATYPE(CborError)
+
 namespace QTest {
 template<> char *toString<CborError>(const CborError &err)
 {
@@ -101,6 +112,95 @@ private slots:
     void recursionLimit();
 };
 
+struct ParserWrapper
+{
+    void *realdata = nullptr;
+    uint8_t *data;
+    size_t len;
+    CborParser parser;
+    CborValue first;
+
+    ~ParserWrapper() { freeMemory(); }
+
+    CborError init(const QByteArray &ba)
+    {
+        freeMemory();
+        data = allocateMemory(ba.size());
+        memcpy(data, ba.data(), ba.size());
+        return cbor_parser_init(data, len, 0, &parser, &first);
+    }
+
+    uint8_t *allocateMemory(size_t);
+    void freeMemory();
+
+    static const size_t PageSize = 4096;
+    static inline size_t mmapAllocation(size_t n)
+    {
+        // round up and add one page
+        return (n + 2*PageSize) & ~(PageSize - 1);
+    }
+    static bool shouldUseMmap();
+};
+
+bool ParserWrapper::shouldUseMmap()
+{
+    static int v = qEnvironmentVariableIntValue("PARSER_NO_MMAP");
+    return !v;
+}
+
+uint8_t *ParserWrapper::allocateMemory(size_t n)
+{
+    len = n;
+    if (shouldUseMmap()) {
+        size_t alloc = mmapAllocation(n);
+#if defined(Q_OS_UNIX)
+        realdata = mmap(nullptr, alloc, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
+        Q_ASSERT_X(realdata != MAP_FAILED, "allocateMemory", "mmap failed!");
+
+        // mark last page inaccessible
+        uint8_t *ptr = static_cast<uint8_t *>(realdata);
+        ptr += alloc - PageSize;
+        mprotect(ptr, PageSize, PROT_NONE);
+
+        ptr -= n;
+        return ptr;
+#elif defined(Q_OS_WIN)
+        // ### implement me
+        DWORD flAllocationType = MEM_COMMIT | MEM_RESERVE;
+        DWORD flProtect = PAGE_READWRITE;
+        realdata = VirtualAlloc(nullptr, alloc, flAllocationType, flProtect);
+        Q_ASSERT_X(realdata, "allocateMemory", "VirtualAlloc failed!");
+
+        // mark last page inaccessible
+        uint8_t *ptr = static_cast<uint8_t *>(realdata);
+        ptr += alloc - PageSize;
+        VirtualProtect(ptr, PageSize, PAGE_NOACCESS, nullptr);
+
+        ptr -= n;
+        return ptr;
+#endif
+    }
+    realdata = malloc(n);
+    return static_cast<uint8_t *>(realdata);
+}
+
+void ParserWrapper::freeMemory()
+{
+    if (shouldUseMmap()) {
+        if (realdata) {
+#if defined(Q_OS_UNIX)
+            size_t alloc = mmapAllocation(len);
+            munmap(realdata, alloc);
+#elif defined(Q_OS_WIN)
+            VirtualFree(realdata, 0, MEM_RELEASE);
+#endif
+        }
+        return;
+    }
+
+    free(realdata);
+}
+
 static CborError qstring_printf(void *out, const char *fmt, ...)
 {
     auto str = static_cast<QString *>(out);
@@ -1947,12 +2047,11 @@ void tst_Parser::strictValidation()
     QFETCH(CborError, expectedError);
 
     QString decoded;
-    CborParser parser;
-    CborValue first;
-    CborError err = cbor_parser_init(reinterpret_cast<const quint8 *>(data.constData()), data.length(), 0, &parser, &first);
+    ParserWrapper w;
+    CborError err = w.init(data);
     QVERIFY2(!err, QByteArray("Got error \"") + cbor_error_string(err) + "\"");
 
-    err = cbor_value_validate(&first, flags);
+    err = cbor_value_validate(&w.first, flags);
     QCOMPARE(err, expectedError);
 }