From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 19 Oct 2021 07:24:52 +0000 (+0200)
Subject: dnsdist: Prevent UB by not accessing the DNS header via a (potentially) misaligned... 
X-Git-Tag: rec-4.6.0-beta1~28^2~11
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=98797bcbeb21fdee8368060ee1c6c0e9d3710f09;p=thirdparty%2Fpdns.git

dnsdist: Prevent UB by not accessing the DNS header via a (potentially) misaligned address
---

diff --git a/pdns/dnsdistdist/dnsdist-tcp-downstream.cc b/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
index 8bb84145e0..b6bdb75b95 100644
--- a/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
@@ -138,12 +138,27 @@ void TCPConnectionToBackend::release()
   }
 }
 
+static void editPayloadID(PacketBuffer& payload, uint16_t newId, size_t proxyProtocolPayloadSize, bool sizePrepended)
+{
+  /* we cannot do a direct cast as the alignment might be off (the size of the payload might have been prepended, which is bad enough,
+     but we might also have a proxy protocol payload */
+  size_t startOfHeaderOffset = (sizePrepended ? sizeof(uint16_t) : 0) + proxyProtocolPayloadSize;
+  if (payload.size() < startOfHeaderOffset + sizeof(dnsheader)) {
+    throw std::runtime_error("Invalid buffer for outgoing TCP query (size " + std::to_string(payload.size()));
+  }
+  dnsheader dh;
+  memcpy(&dh, &payload.at(startOfHeaderOffset), sizeof(dh));
+  dh.id = htons(newId);
+  memcpy(&payload.at(startOfHeaderOffset), &dh, sizeof(dh));
+}
+
 IOState TCPConnectionToBackend::queueNextQuery(std::shared_ptr<TCPConnectionToBackend>& conn)
 {
   conn->d_currentQuery = std::move(conn->d_pendingQueries.front());
-  dnsheader* dh = reinterpret_cast<dnsheader*>(&conn->d_currentQuery.d_query.d_buffer.at(sizeof(uint16_t) + (conn->d_currentQuery.d_query.d_proxyProtocolPayloadAdded ? conn->d_currentQuery.d_query.d_proxyProtocolPayload.size() : 0)));
+
   uint16_t id = conn->d_highestStreamID;
-  dh->id = htons(id);
+  editPayloadID(conn->d_currentQuery.d_query.d_buffer, id, conn->d_currentQuery.d_query.d_proxyProtocolPayloadAdded ? conn->d_currentQuery.d_query.d_proxyProtocolPayload.size() : 0, true);
+
   conn->d_pendingQueries.pop_front();
   conn->d_state = State::sendingQueryToBackend;
   conn->d_currentPos = 0;
@@ -303,9 +318,9 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
             if (conn->d_state == State::sendingQueryToBackend) {
               /* we need to edit this query so it has the correct ID */
               auto query = std::move(conn->d_currentQuery);
-              dnsheader* dh = reinterpret_cast<dnsheader*>(&query.d_query.d_buffer.at(sizeof(uint16_t) + (query.d_query.d_proxyProtocolPayloadAdded ? query.d_query.d_proxyProtocolPayload.size() : 0)));
+
               uint16_t id = conn->d_highestStreamID;
-              dh->id = htons(id);
+              editPayloadID(query.d_query.d_buffer, id, query.d_query.d_proxyProtocolPayloadAdded ? query.d_query.d_proxyProtocolPayload.size() : 0, true);
               conn->d_currentQuery = std::move(query);
             }
 
@@ -417,9 +432,10 @@ void TCPConnectionToBackend::queueQuery(std::shared_ptr<TCPQuerySender>& sender,
     DEBUGLOG("Sending new query to backend right away, with ID "<<d_highestStreamID);
     d_state = State::sendingQueryToBackend;
     d_currentPos = 0;
-    dnsheader* dh = reinterpret_cast<dnsheader*>(&query.d_buffer.at(sizeof(uint16_t) + (query.d_proxyProtocolPayloadAdded ? query.d_proxyProtocolPayload.size() : 0)));
+
     uint16_t id = d_highestStreamID;
-    dh->id = htons(id);
+    editPayloadID(query.d_buffer, id, query.d_proxyProtocolPayloadAdded ? query.d_proxyProtocolPayload.size() : 0, true);
+
     d_currentQuery = PendingRequest({sender, std::move(query)});
 
     if (needProxyProtocolPayload() && !d_currentQuery.d_query.d_proxyProtocolPayloadAdded && !d_currentQuery.d_query.d_proxyProtocolPayload.empty()) {
@@ -573,8 +589,7 @@ IOState TCPConnectionToBackend::handleResponse(std::shared_ptr<TCPConnectionToBa
     return IOState::Done;
   }
 
-  auto dh = reinterpret_cast<dnsheader*>(d_responseBuffer.data());
-  dh->id = it->second.d_query.d_idstate.origID;
+  editPayloadID(d_responseBuffer, ntohs(it->second.d_query.d_idstate.origID), 0, false);
 
   auto sender = it->second.d_sender;
 
diff --git a/pdns/dnsdistdist/test-dnsdisttcp_cc.cc b/pdns/dnsdistdist/test-dnsdisttcp_cc.cc
index 4d6224149c..2a52e73ecd 100644
--- a/pdns/dnsdistdist/test-dnsdisttcp_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdisttcp_cc.cc
@@ -422,16 +422,20 @@ static ComboAddress getBackendAddress(const std::string& lastDigit, uint16_t por
 static void appendPayloadEditingID(PacketBuffer& buffer, const PacketBuffer& payload, uint16_t newID)
 {
   PacketBuffer newPayload(payload);
-  auto dh = reinterpret_cast<dnsheader*>(&newPayload.at(sizeof(uint16_t)));
-  dh->id = htons(newID);
+  dnsheader dh;
+  memcpy(&dh, &newPayload.at(sizeof(uint16_t)), sizeof(dh));
+  dh.id = htons(newID);
+  memcpy(&newPayload.at(sizeof(uint16_t)), &dh, sizeof(dh));
   buffer.insert(buffer.end(), newPayload.begin(), newPayload.end());
 }
 
 static void prependPayloadEditingID(PacketBuffer& buffer, const PacketBuffer& payload, uint16_t newID)
 {
   PacketBuffer newPayload(payload);
-  auto dh = reinterpret_cast<dnsheader*>(&newPayload.at(sizeof(uint16_t)));
-  dh->id = htons(newID);
+  dnsheader dh;
+  memcpy(&dh, &newPayload.at(sizeof(uint16_t)), sizeof(dh));
+  dh.id = htons(newID);
+  memcpy(&newPayload.at(sizeof(uint16_t)), &dh, sizeof(dh));
   buffer.insert(buffer.begin(), newPayload.begin(), newPayload.end());
 }