From eb51454b907b02aa67268c162896fc6778920e4c Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Mon, 15 May 2023 08:07:32 -0700 Subject: QDnsLookup/Unix: rework the buffer-size check code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is neater with a simple offset and avoids the potential UB code this was carrying in: p += size; (p < response + responseLength) It's UB to add to a pointer a size that moves it past the end of its array. In practice we don't expect this to happen because of construction (p is always pointing to a heap or auxiliary-thread stack buffer), but in theory it could happen that said buffer is too close to the end of the virtual address space and adding `size` causes it to overflow back to small values. Change-Id: I5f7f427ded124479baa6fffd175f59939c15c666 Reviewed-by: Mårten Nordheim --- src/network/kernel/qdnslookup_unix.cpp | 65 ++++++++++++++++------------------ 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/src/network/kernel/qdnslookup_unix.cpp b/src/network/kernel/qdnslookup_unix.cpp index 497f704913..988334e5eb 100644 --- a/src/network/kernel/qdnslookup_unix.cpp +++ b/src/network/kernel/qdnslookup_unix.cpp @@ -177,21 +177,21 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN } char host[PACKETSZ], answer[PACKETSZ]; - unsigned char *p = response + sizeof(HEADER); + qptrdiff offset = sizeof(HEADER); int status; if (ntohs(header->qdcount) == 1) { // Skip the query host, type (2 bytes) and class (2 bytes). - status = dn_expand(response, response + responseLength, p, host, sizeof(host)); + status = dn_expand(response, response + responseLength, response + offset, host, sizeof(host)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Could not expand domain name"); return; } - if ((p - response) + status + 4 >= responseLength) + if (offset + status + 4 >= responseLength) header->qdcount = 0xffff; // invalid reply below else - p += status + 4; + offset += status + 4; } if (ntohs(header->qdcount) > 1) { reply->error = QDnsLookup::InvalidReplyError; @@ -202,8 +202,8 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN // Extract results. const int answerCount = ntohs(header->ancount); int answerIndex = 0; - while ((p < response + responseLength) && (answerIndex < answerCount)) { - status = dn_expand(response, response + responseLength, p, host, sizeof(host)); + while ((offset < responseLength) && (answerIndex < answerCount)) { + status = dn_expand(response, response + responseLength, response + offset, host, sizeof(host)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Could not expand domain name"); @@ -211,20 +211,17 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN } const QString name = QUrl::fromAce(host); - p += status; - if ((p - response) + 10 > responseLength) { + offset += status; + if (offset + RRFIXEDSZ > responseLength) { // probably just a truncated reply, return what we have return; } - const quint16 type = qFromBigEndian(p); - p += 2; // RR type - const qint16 rrclass = qFromBigEndian(p); - p += 2; // RR class - const quint32 ttl = qFromBigEndian(p); - p += 4; - const quint16 size = qFromBigEndian(p); - p += 2; - if ((p - response) + size > responseLength) + const quint16 type = qFromBigEndian(response + offset); + const qint16 rrclass = qFromBigEndian(response + offset + 2); + const quint32 ttl = qFromBigEndian(response + offset + 4); + const quint16 size = qFromBigEndian(response + offset + 8); + offset += RRFIXEDSZ; + if (offset + size > responseLength) return; // truncated if (rrclass != C_IN) continue; @@ -235,7 +232,7 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN reply->errorString = tr("Invalid IPv4 address record"); return; } - const quint32 addr = qFromBigEndian(p); + const quint32 addr = qFromBigEndian(response + offset); QDnsHostAddressRecord record; record.d->name = name; record.d->timeToLive = ttl; @@ -250,10 +247,10 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN QDnsHostAddressRecord record; record.d->name = name; record.d->timeToLive = ttl; - record.d->value = QHostAddress(p); + record.d->value = QHostAddress(response + offset); reply->hostAddressRecords.append(record); } else if (type == QDnsLookup::CNAME) { - status = dn_expand(response, response + responseLength, p, answer, sizeof(answer)); + status = dn_expand(response, response + responseLength, response + offset, answer, sizeof(answer)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Invalid canonical name record"); @@ -265,7 +262,7 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN record.d->value = QUrl::fromAce(answer); reply->canonicalNameRecords.append(record); } else if (type == QDnsLookup::NS) { - status = dn_expand(response, response + responseLength, p, answer, sizeof(answer)); + status = dn_expand(response, response + responseLength, response + offset, answer, sizeof(answer)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Invalid name server record"); @@ -277,7 +274,7 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN record.d->value = QUrl::fromAce(answer); reply->nameServerRecords.append(record); } else if (type == QDnsLookup::PTR) { - status = dn_expand(response, response + responseLength, p, answer, sizeof(answer)); + status = dn_expand(response, response + responseLength, response + offset, answer, sizeof(answer)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Invalid pointer record"); @@ -289,8 +286,8 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN record.d->value = QUrl::fromAce(answer); reply->pointerRecords.append(record); } else if (type == QDnsLookup::MX) { - const quint16 preference = qFromBigEndian(p); - status = dn_expand(response, response + responseLength, p + 2, answer, sizeof(answer)); + const quint16 preference = qFromBigEndian(response + offset); + status = dn_expand(response, response + responseLength, response + offset + 2, answer, sizeof(answer)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Invalid mail exchange record"); @@ -303,10 +300,10 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN record.d->timeToLive = ttl; reply->mailExchangeRecords.append(record); } else if (type == QDnsLookup::SRV) { - const quint16 priority = qFromBigEndian(p); - const quint16 weight = qFromBigEndian(p + 2); - const quint16 port = qFromBigEndian(p + 4); - status = dn_expand(response, response + responseLength, p + 6, answer, sizeof(answer)); + const quint16 priority = qFromBigEndian(response + offset); + const quint16 weight = qFromBigEndian(response + offset + 2); + const quint16 port = qFromBigEndian(response + offset + 4); + status = dn_expand(response, response + responseLength, response + offset + 6, answer, sizeof(answer)); if (status < 0) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Invalid service record"); @@ -321,24 +318,24 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN record.d->weight = weight; reply->serviceRecords.append(record); } else if (type == QDnsLookup::TXT) { - unsigned char *txt = p; QDnsTextRecord record; record.d->name = name; record.d->timeToLive = ttl; - while (txt < p + size) { - const unsigned char length = *txt; + qptrdiff txt = offset; + while (txt < offset + size) { + const unsigned char length = response[txt]; txt++; - if (txt + length > p + size) { + if (txt + length > offset + size) { reply->error = QDnsLookup::InvalidReplyError; reply->errorString = tr("Invalid text record"); return; } - record.d->values << QByteArray((char*)txt, length); + record.d->values << QByteArrayView(response + txt, length).toByteArray(); txt += length; } reply->textRecords.append(record); } - p += size; + offset += size; answerIndex++; } } -- cgit v1.2.1