Q{CoffPe,Elf,MachO}Parser: check that the magic string is present

Commit 2549a88ba2a48fa2bedce97dd71a2974c6f8840a changed the ELF and
Mach-O parsers to return an offset to the actual data header, not the
magic string, which we stopped searching for anyway. This commit brings
such a validity check back and adds it to the new COFF PE parser.

Change-Id: Iccb47e5527544b6fbd75fffd16b8b2252a76f179
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

3 files changed, 31 insertions, 8 deletions

diff --git a/src/corelib/plugin/qcoffpeparser.cpp b/src/corelib/plugin/qcoffpeparser.cpp
index 4bd0a5516f..f258ef5de2 100644
--- a/src/corelib/plugin/qcoffpeparser.cpp
+++ b/src/corelib/plugin/qcoffpeparser.cpp
@@ -396,15 +396,20 @@ QLibraryScanResult QCoffPeParser::parse(QByteArrayView data, QString *errMsg)
             continue;
         peDebug << "found .qtmetadata section";
 
+        size_t size = qMin(section->SizeOfRawData, section->Misc.VirtualSize);
+        if (size < sizeof(QPluginMetaData::MagicHeader))
+            return error(QLibrary::tr(".qtmetadata section is too small"));
         if (IncludeValidityChecks) {
+            QByteArrayView expectedMagic = QByteArrayView::fromArray(QPluginMetaData::MagicString);
+            QByteArrayView actualMagic = data.sliced(offset, expectedMagic.size());
+            if (expectedMagic != actualMagic)
+                return error(QLibrary::tr(".qtmetadata section has incorrect magic"));
+
             if (section->Characteristics & IMAGE_SCN_MEM_WRITE)
                 return error(QLibrary::tr(".qtmetadata section is writable"));
             if (section->Characteristics & IMAGE_SCN_MEM_EXECUTE)
                 return error(QLibrary::tr(".qtmetadata section is executable"));
         }
-        size_t size = qMin(section->SizeOfRawData, section->Misc.VirtualSize);
-        if (size < sizeof(QPluginMetaData::MagicHeader))
-            return error(QLibrary::tr("section .qtmetadata is too small"));
 
         return { qsizetype(offset + sizeof(QPluginMetaData::MagicString)),
                  qsizetype(size - sizeof(QPluginMetaData::MagicString)) };
diff --git a/src/corelib/plugin/qelfparser_p.cpp b/src/corelib/plugin/qelfparser_p.cpp
index 33af51d59b..6a83c94e20 100644
--- a/src/corelib/plugin/qelfparser_p.cpp
+++ b/src/corelib/plugin/qelfparser_p.cpp
@@ -712,13 +712,20 @@ static QLibraryScanResult scanSections(QByteArrayView data, const ErrorMaker &er
         if (name != QLatin1String(".qtmetadata"))
             continue;
         qEDebug << "found .qtmetadata section";
-        if (IncludeValidityChecks && shdr->sh_flags & (SHF_WRITE | SHF_EXECINSTR)) {
+        if (shdr->sh_size < sizeof(QPluginMetaData::MagicHeader))
+            return error(QLibrary::tr(".qtmetadata section is too small"));
+
+        if (IncludeValidityChecks) {
+            QByteArrayView expectedMagic = QByteArrayView::fromArray(QPluginMetaData::MagicString);
+            QByteArrayView actualMagic = data.sliced(shdr->sh_offset, expectedMagic.size());
+            if (expectedMagic != actualMagic)
+                return error(QLibrary::tr(".qtmetadata section has incorrect magic"));
+
             if (shdr->sh_flags & SHF_WRITE)
                 return error(QLibrary::tr(".qtmetadata section is writable"));
-            return error(QLibrary::tr(".qtmetadata section is executable"));
+            if (shdr->sh_flags & SHF_EXECINSTR)
+                return error(QLibrary::tr(".qtmetadata section is executable"));
         }
-        if (shdr->sh_size < sizeof(QPluginMetaData::MagicHeader))
-            return error(QLibrary::tr("section .qtmetadata is too small"));
 
         return { qsizetype(shdr->sh_offset + sizeof(QPluginMetaData::MagicString)),
                  qsizetype(shdr->sh_size - sizeof(QPluginMetaData::MagicString)) };
diff --git a/src/corelib/plugin/qmachparser.cpp b/src/corelib/plugin/qmachparser.cpp
index f864f2610e..310d8e06c3 100644
--- a/src/corelib/plugin/qmachparser.cpp
+++ b/src/corelib/plugin/qmachparser.cpp
@@ -46,6 +46,10 @@
 
 QT_BEGIN_NAMESPACE
 
+// Whether we include some extra validity checks
+// (checks to ensure we don't read out-of-bounds are always included)
+static constexpr bool IncludeValidityChecks = true;
+
 #if defined(Q_PROCESSOR_X86_64)
 #  define MACHO64
 static const cpu_type_t my_cputype = CPU_TYPE_X86_64;
@@ -193,9 +197,16 @@ QLibraryScanResult  QMachOParser::parse(const char *m_s, ulong fdlen, QString *e
                     return notfound(QString(), errorString);
 
                 if (sect[j].size < sizeof(QPluginMetaData::MagicHeader))
-                    return notfound(QLibrary::tr("section .qtmetadata is too small"), errorString);
+                    return notfound(QLibrary::tr(".qtmetadata section is too small"), errorString);
 
                 qsizetype pos = reinterpret_cast<const char *>(header) - m_s + sect[j].offset;
+                if (IncludeValidityChecks) {
+                    QByteArrayView expectedMagic = QByteArrayView::fromArray(QPluginMetaData::MagicString);
+                    QByteArrayView actualMagic = QByteArrayView(m_s + pos, expectedMagic.size());
+                    if (expectedMagic != actualMagic)
+                        return notfound(QLibrary::tr(".qtmetadata section has incorrect magic"), errorString);
+                }
+
                 pos += sizeof(QPluginMetaData::MagicString);
                 return { pos, qsizetype(sect[j].size - sizeof(QPluginMetaData::MagicString)) };
             }