From 3b49aa72fe6ec0dd0aa0c1c41fb81e874dc789fa Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 18 Nov 2021 08:52:17 -0800 Subject: Q{CoffPe,Elf,MachO}Parser: check that the magic string is present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/corelib/plugin/qcoffpeparser.cpp | 11 ++++++++--- src/corelib/plugin/qelfparser_p.cpp | 15 +++++++++++---- src/corelib/plugin/qmachparser.cpp | 13 ++++++++++++- 3 files changed, 31 insertions(+), 8 deletions(-) (limited to 'src/corelib/plugin') 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(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)) }; } -- cgit v1.2.3