From 7273d744a8439fcea194add4d4c5ae618c10d89f Mon Sep 17 00:00:00 2001 From: Alexander Sashnov Date: Thu, 24 Mar 2022 23:15:35 +0700 Subject: [PATCH 1/3] mDNS parser: skip a 'Record' on parse error Skipping an unknown Record is much better then ignoring the entire Message. --- src/src/dns.cpp | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/src/dns.cpp b/src/src/dns.cpp index 68c8794..1f3d849 100644 --- a/src/src/dns.cpp +++ b/src/src/dns.cpp @@ -297,6 +297,9 @@ void writeRecord(QByteArray &packet, quint16 &offset, Record &record, QMap(packet, offset, transactionId) || @@ -325,14 +328,29 @@ bool fromPacket(const QByteArray &packet, Message &message) message.addQuery(query); } quint16 nRecord = nAnswer + nAuthority + nAdditional; + bool with_error = false; + uint records = 0; for (int i = 0; i < nRecord; ++i) { Record record; if (!parseRecord(packet, offset, record)) { - return false; + with_error = true; + } + else { + message.addRecord(record); + records++; } - message.addRecord(record); } - return true; + + if (with_error && reported_errors < MAX_REPORTED_ERRORS) { + qWarning() << "qmdnsengine: mDNS parse error, UDP payload is" << packet.toHex(); + reported_errors++; + if (reported_errors == MAX_REPORTED_ERRORS) { + qWarning() << "qmdnsengine: no more similar errors be reported: maximum report count is reached."; + } + } + + // this message will be processed further if at least one record was parsed + return records > 0; } void toPacket(const Message &message, QByteArray &packet) From 289283954e252ace888d52c2b00eec34c5a0d3fc Mon Sep 17 00:00:00 2001 From: Alexander Sashnov Date: Sun, 17 Apr 2022 15:00:24 +0700 Subject: [PATCH 2/3] Add test: captured from Python zeroconf Add a real-life UDP packed data captured from Python zeroconf module. The current 'master' (0ca80117) incorrectly determines the end of NSEC record, causing next 'A' record unparsed. --- tests/CMakeLists.txt | 1 + tests/TestFromPacket.cpp | 58 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 tests/TestFromPacket.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2c0085a..4cbd360 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,6 +4,7 @@ set(TESTS TestBrowser TestCache TestDns + TestFromPacket TestHostname TestProber TestProvider diff --git a/tests/TestFromPacket.cpp b/tests/TestFromPacket.cpp new file mode 100644 index 0000000..dd274f1 --- /dev/null +++ b/tests/TestFromPacket.cpp @@ -0,0 +1,58 @@ +#include + +#include +#include +#include + +unsigned char b0[] = { + 0x00, 0x00, 0x84, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x04, + 0x10, 0x5f, 0x75, 0x6e, 0x69, 0x70, 0x72, 0x6f, 0x2d, 0x6c, 0x61, 0x70, + 0x74, 0x69, 0x6d, 0x65, 0x72, 0x04, 0x5f, 0x74, 0x63, 0x70, 0x05, 0x6c, + 0x6f, 0x63, 0x61, 0x6c, 0x00, 0x00, 0x0c, 0x00, 0x01, 0x00, 0x00, 0x11, + 0x94, 0x00, 0x0e, 0x0b, 0x75, 0x6e, 0x69, 0x67, 0x6f, 0x2d, 0x31, 0x32, + 0x33, 0x34, 0x35, 0xc0, 0x0c, 0xc0, 0x33, 0x00, 0x21, 0x80, 0x01, 0x00, + 0x00, 0x00, 0x78, 0x00, 0x14, 0x00, 0x00, 0x00, 0x00, 0x1f, 0x90, 0x0b, + 0x75, 0x6e, 0x69, 0x67, 0x6f, 0x2d, 0x31, 0x32, 0x33, 0x34, 0x35, 0xc0, + 0x22, 0xc0, 0x53, 0x00, 0x2f, 0x80, 0x01, 0x00, 0x00, 0x11, 0x94, 0x00, + 0x0a, 0xc0, 0x53, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x08, 0xc0, + 0x53, 0x00, 0x01, 0x80, 0x01, 0x00, 0x00, 0x00, 0x78, 0x00, 0x04, 0xc0, + 0xa8, 0x32, 0x45, 0xc0, 0x33, 0x00, 0x10, 0x80, 0x01, 0x00, 0x00, 0x11, + 0x94, 0x00, 0x3d, 0x10, 0x4f, 0x77, 0x6e, 0x65, 0x72, 0x3d, 0x55, 0x6e, + 0x69, 0x70, 0x72, 0x6f, 0x20, 0x41, 0x70, 0x73, 0x07, 0x44, 0x72, 0x69, + 0x76, 0x65, 0x72, 0x3d, 0x0f, 0x4d, 0x6f, 0x64, 0x65, 0x6c, 0x3d, 0x55, + 0x6e, 0x69, 0x47, 0x6f, 0x20, 0x4f, 0x6e, 0x65, 0x13, 0x73, 0x65, 0x72, + 0x69, 0x61, 0x6c, 0x2d, 0x6e, 0x75, 0x6d, 0x62, 0x65, 0x72, 0x3d, 0x31, + 0x32, 0x33, 0x34, 0x35 +}; + + +class TestFromPacket : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + + void testParseB0(); +}; + + + +void TestFromPacket::testParseB0() +{ + /* This code mimics + * void ServerPrivate::onReadyRead() + * ... + * socket->readDatagram(packet.data(), packet.size(), &address, &port); + */ + + QByteArray packet = QByteArray::fromRawData( + (const char *)b0, sizeof(b0)); + + QMdnsEngine::Message message; + + QVERIFY(fromPacket(packet, message)); +} + + +QTEST_MAIN(TestFromPacket) +#include "TestFromPacket.moc" From cc82f712b281eef07c7fa6875e71ab8b670272ef Mon Sep 17 00:00:00 2001 From: Alexander Sashnov Date: Sun, 17 Apr 2022 15:01:55 +0700 Subject: [PATCH 3/3] Reliably determine the end of record in answer That will allow to skip unknown or incorrectly parsed records, and move one to a next record to the right place. --- src/src/dns.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/src/dns.cpp b/src/src/dns.cpp index 1f3d849..96a3686 100644 --- a/src/src/dns.cpp +++ b/src/src/dns.cpp @@ -142,6 +142,9 @@ bool parseRecord(const QByteArray &packet, quint16 &offset, Record &record) record.setType(type); record.setFlushCache(class_ & 0x8000); record.setTtl(ttl); + + quint16 next_offset = offset + dataLen; + switch (type) { case A: { @@ -234,6 +237,7 @@ bool parseRecord(const QByteArray &packet, quint16 &offset, Record &record) offset += dataLen; break; } + offset = next_offset; return true; }