1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QDnsLookup/Unix: rework the buffer-size check code

Browse Source 
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 <marten.nordheim@qt.io>
This commit is contained in:
Thiago Macieira 2023-05-15 08:07:32 -07:00
parent 7ca633d9a8
commit eb51454b90
1 changed files with 31 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
src/network/kernel/qdnslookup_unix.cpp
View File

@ -177,21 +177,21 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN
} }
char host[PACKETSZ], answer[PACKETSZ]; char host[PACKETSZ], answer[PACKETSZ];
unsigned char *p = response + sizeof(HEADER); qptrdiff offset = sizeof(HEADER);
int status; int status;
if (ntohs(header->qdcount) == 1) { if (ntohs(header->qdcount) == 1) {
// Skip the query host, type (2 bytes) and class (2 bytes). // 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) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Could not expand domain name"); reply->errorString = tr("Could not expand domain name");
return; return;
} }
if ((p - response) + status + 4 >= responseLength) if (offset + status + 4 >= responseLength)
header->qdcount = 0xffff; // invalid reply below header->qdcount = 0xffff; // invalid reply below
else else
p += status + 4; offset += status + 4;
} }
if (ntohs(header->qdcount) > 1) { if (ntohs(header->qdcount) > 1) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
@ -202,8 +202,8 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN
// Extract results. // Extract results.
const int answerCount = ntohs(header->ancount); const int answerCount = ntohs(header->ancount);
int answerIndex = 0; int answerIndex = 0;
while ((p < response + responseLength) && (answerIndex < answerCount)) { while ((offset < responseLength) && (answerIndex < answerCount)) {
status = dn_expand(response, response + responseLength, p, host, sizeof(host)); status = dn_expand(response, response + responseLength, response + offset, host, sizeof(host));
if (status < 0) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Could not expand domain name"); 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); const QString name = QUrl::fromAce(host);
p += status; offset += status;
if ((p - response) + 10 > responseLength) { if (offset + RRFIXEDSZ > responseLength) {
// probably just a truncated reply, return what we have // probably just a truncated reply, return what we have
return; return;
} }
const quint16 type = qFromBigEndian<quint16>(p); const quint16 type = qFromBigEndian<quint16>(response + offset);
p += 2; // RR type const qint16 rrclass = qFromBigEndian<quint16>(response + offset + 2);
const qint16 rrclass = qFromBigEndian<quint16>(p); const quint32 ttl = qFromBigEndian<quint32>(response + offset + 4);
p += 2; // RR class const quint16 size = qFromBigEndian<quint16>(response + offset + 8);
const quint32 ttl = qFromBigEndian<quint32>(p); offset += RRFIXEDSZ;
p += 4; if (offset + size > responseLength)
const quint16 size = qFromBigEndian<quint16>(p);
p += 2;
if ((p - response) + size > responseLength)
return; // truncated return; // truncated
if (rrclass != C_IN) if (rrclass != C_IN)
continue; continue;
@ -235,7 +232,7 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN
reply->errorString = tr("Invalid IPv4 address record"); reply->errorString = tr("Invalid IPv4 address record");
return; return;
} }
const quint32 addr = qFromBigEndian<quint32>(p); const quint32 addr = qFromBigEndian<quint32>(response + offset);
QDnsHostAddressRecord record; QDnsHostAddressRecord record;
record.d->name = name; record.d->name = name;
record.d->timeToLive = ttl; record.d->timeToLive = ttl;
@ -250,10 +247,10 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN
QDnsHostAddressRecord record; QDnsHostAddressRecord record;
record.d->name = name; record.d->name = name;
record.d->timeToLive = ttl; record.d->timeToLive = ttl;
record.d->value = QHostAddress(p); record.d->value = QHostAddress(response + offset);
reply->hostAddressRecords.append(record); reply->hostAddressRecords.append(record);
} else if (type == QDnsLookup::CNAME) { } 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) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Invalid canonical name record"); 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); record.d->value = QUrl::fromAce(answer);
reply->canonicalNameRecords.append(record); reply->canonicalNameRecords.append(record);
} else if (type == QDnsLookup::NS) { } 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) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Invalid name server record"); 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); record.d->value = QUrl::fromAce(answer);
reply->nameServerRecords.append(record); reply->nameServerRecords.append(record);
} else if (type == QDnsLookup::PTR) { } 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) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Invalid pointer record"); 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); record.d->value = QUrl::fromAce(answer);
reply->pointerRecords.append(record); reply->pointerRecords.append(record);
} else if (type == QDnsLookup::MX) { } else if (type == QDnsLookup::MX) {
const quint16 preference = qFromBigEndian<quint16>(p); const quint16 preference = qFromBigEndian<quint16>(response + offset);
status = dn_expand(response, response + responseLength, p + 2, answer, sizeof(answer)); status = dn_expand(response, response + responseLength, response + offset + 2, answer, sizeof(answer));
if (status < 0) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Invalid mail exchange record"); reply->errorString = tr("Invalid mail exchange record");
@ -303,10 +300,10 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN
record.d->timeToLive = ttl; record.d->timeToLive = ttl;
reply->mailExchangeRecords.append(record); reply->mailExchangeRecords.append(record);
} else if (type == QDnsLookup::SRV) { } else if (type == QDnsLookup::SRV) {
const quint16 priority = qFromBigEndian<quint16>(p); const quint16 priority = qFromBigEndian<quint16>(response + offset);
const quint16 weight = qFromBigEndian<quint16>(p + 2); const quint16 weight = qFromBigEndian<quint16>(response + offset + 2);
const quint16 port = qFromBigEndian<quint16>(p + 4); const quint16 port = qFromBigEndian<quint16>(response + offset + 4);
status = dn_expand(response, response + responseLength, p + 6, answer, sizeof(answer)); status = dn_expand(response, response + responseLength, response + offset + 6, answer, sizeof(answer));
if (status < 0) { if (status < 0) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Invalid service record"); reply->errorString = tr("Invalid service record");
@ -321,24 +318,24 @@ void QDnsLookupRunnable::query(const int requestType, const QByteArray &requestN
record.d->weight = weight; record.d->weight = weight;
reply->serviceRecords.append(record); reply->serviceRecords.append(record);
} else if (type == QDnsLookup::TXT) { } else if (type == QDnsLookup::TXT) {
unsigned char *txt = p;
QDnsTextRecord record; QDnsTextRecord record;
record.d->name = name; record.d->name = name;
record.d->timeToLive = ttl; record.d->timeToLive = ttl;
while (txt < p + size) { qptrdiff txt = offset;
const unsigned char length = *txt; while (txt < offset + size) {
const unsigned char length = response[txt];
txt++; txt++;
if (txt + length > p + size) { if (txt + length > offset + size) {
reply->error = QDnsLookup::InvalidReplyError; reply->error = QDnsLookup::InvalidReplyError;
reply->errorString = tr("Invalid text record"); reply->errorString = tr("Invalid text record");
return; return;
} }
record.d->values << QByteArray((char*)txt, length); record.d->values << QByteArrayView(response + txt, length).toByteArray();
txt += length; txt += length;
} }
reply->textRecords.append(record); reply->textRecords.append(record);
} }
p += size; offset += size;
answerIndex++; answerIndex++;
} }
} }