From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 25 Jun 2019 14:08:48 +0000 (+0200)
Subject: dnsdist: Use a separate string for the DoH query and response
X-Git-Tag: dnsdist-1.4.0-rc1~38^2~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=246ff31f279b5dde43e9300e96145e062210cf70;p=thirdparty%2Fpdns.git

dnsdist: Use a separate string for the DoH query and response

In rare cases, the responder thread might get a response _before_
the send() call of the DoH client thread has returned, resulting
in ASAN reporting a use-after-free since the response was written
into the now useless query object.
---

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index b8ab70b9b3..0866425413 100644
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -564,6 +564,7 @@ try {
 
         int oldFD = ids->origFD.exchange(-1);
         if (oldFD == origFD) {
+          ids->du = nullptr;
           /* we only decrement the outstanding counter if the value was not
              altered in the meantime, which would mean that the state has been actively reused
              and the other thread has not incremented the outstanding counter, so we don't
@@ -572,7 +573,6 @@ try {
         } else {
           du = nullptr;
         }
-        ids->du = nullptr;
 
         if(dh->tc && g_truncateTC) {
           truncateTC(response, &responseLen, responseSize, consumed);
@@ -595,7 +595,7 @@ try {
           if (du) {
 #ifdef HAVE_DNS_OVER_HTTPS
             // DoH query
-            du->query = std::string(response, responseLen);
+            du->response = std::string(response, responseLen);
             if (send(du->rsock, &du, sizeof(du), 0) != sizeof(du)) {
               delete du;
             }
diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc
index 8608922b91..c3c93511fa 100644
--- a/pdns/dnsdistdist/doh.cc
+++ b/pdns/dnsdistdist/doh.cc
@@ -218,7 +218,7 @@ static int processDOHQuery(DOHUnit* du)
     }
 
     if (result == ProcessQueryResult::SendAnswer) {
-      du->query = std::string(reinterpret_cast<char*>(dq.dh), dq.len);
+      du->response = std::string(reinterpret_cast<char*>(dq.dh), dq.len);
       send(du->rsock, &du, sizeof(du), 0);
       return 0;
     }
@@ -595,8 +595,8 @@ static void on_dnsdist(h2o_socket_t *listener, const char *err)
     //    struct dnsheader* dh = (struct dnsheader*)du->query.c_str();
     //    cout<<"Attempt to send out "<<du->query.size()<<" bytes over https, TC="<<dh->tc<<", RCODE="<<dh->rcode<<", qtype="<<du->qtype<<", req="<<(void*)du->req<<endl;
 
-    du->req->res.content_length = du->query.size();
-    h2o_send_inline(du->req, du->query.c_str(), du->query.size());
+    du->req->res.content_length = du->response.size();
+    h2o_send_inline(du->req, du->response.c_str(), du->response.size());
   }
   else {
     switch(du->status_code) {
diff --git a/pdns/doh.hh b/pdns/doh.hh
index 7d411d25f8..f7c61c6335 100644
--- a/pdns/doh.hh
+++ b/pdns/doh.hh
@@ -55,6 +55,7 @@ struct st_h2o_req_t;
 struct DOHUnit
 {
   std::string query;
+  std::string response;
   ComboAddress remote;
   ComboAddress dest;
   st_h2o_req_t* req{nullptr};