From: Remi Gacogne Date: Thu, 13 Jun 2019 13:49:29 +0000 (+0200) Subject: dnsdist: Proper HTTP response for timeouts over DoH X-Git-Tag: dnsdist-1.4.0-rc1~38^2~5 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0956c5c52b49e33e4d179173e1d3b24eb08d6165;p=pdns dnsdist: Proper HTTP response for timeouts over DoH --- diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index babcc9972..b8ab70b9b 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -546,9 +546,10 @@ try { IDState* ids = &dss->idStates[queryId]; int origFD = ids->origFD; - if(origFD < 0 && ids->du == nullptr) // duplicate + if(origFD < 0) // duplicate continue; + auto du = ids->du; /* setting age to 0 to prevent the maintainer thread from cleaning this IDS while we process the response. We have already a copy of the origFD, so it would @@ -568,7 +569,10 @@ try { and the other thread has not incremented the outstanding counter, so we don't want it to be decremented twice. */ --dss->outstanding; // you'd think an attacker could game this, but we're using connected socket + } else { + du = nullptr; } + ids->du = nullptr; if(dh->tc && g_truncateTC) { truncateTC(response, &responseLen, responseSize, consumed); @@ -588,15 +592,15 @@ try { } if (ids->cs && !ids->cs->muted) { - if (ids->du) { + if (du) { #ifdef HAVE_DNS_OVER_HTTPS // DoH query - ids->du->query = std::string(response, responseLen); - if (send(ids->du->rsock, &ids->du, sizeof(ids->du), 0) != sizeof(ids->du)) { - delete ids->du; + du->query = std::string(response, responseLen); + if (send(du->rsock, &du, sizeof(du), 0) != sizeof(du)) { + delete du; } #endif /* HAVE_DNS_OVER_HTTPS */ - ids->du = nullptr; + du = nullptr; } else { ComboAddress empty; @@ -1564,16 +1568,25 @@ static void processUDPQuery(ClientState& cs, LocalHolders& holders, const struct unsigned int idOffset = (ss->idOffset++) % ss->idStates.size(); IDState* ids = &ss->idStates[idOffset]; ids->age = 0; - ids->du = nullptr; + DOHUnit* du = nullptr; + + if (ids->origFD != -1) { + du = ids->du; + } int oldFD = ids->origFD.exchange(cs.udpFD); if(oldFD < 0) { - // if we are reusing, no change in outstanding ++ss->outstanding; + /* either it was already -1 so no DOH unit to handle, + or someone handled it before us */ + du = nullptr; } else { + // if we are reusing, no change in outstanding ++ss->reuseds; ++g_stats.downstreamTimeouts; + ids->du = nullptr; + handleDOHTimeout(du); } ids->cs = &cs; @@ -2064,11 +2077,14 @@ static void healthChecksThread() so the sooner the better any way since we _will_ decrement it. */ + auto oldDU = ids.du; + if (ids.origFD.exchange(-1) != origFD) { /* this state has been altered in the meantime, don't go anywhere near it */ continue; } + handleDOHTimeout(oldDU); ids.du = nullptr; ids.age = 0; dss->reuseds++; diff --git a/pdns/dnsdistdist/Makefile.am b/pdns/dnsdistdist/Makefile.am index 2112afa53..71bd409c5 100644 --- a/pdns/dnsdistdist/Makefile.am +++ b/pdns/dnsdistdist/Makefile.am @@ -134,7 +134,7 @@ dnsdist_SOURCES = \ dnsname.cc dnsname.hh \ dnsparser.hh dnsparser.cc \ dnswriter.cc dnswriter.hh \ - doh.hh \ + doh.hh doh.cc \ dolog.hh \ ednsoptions.cc ednsoptions.hh \ ednscookies.cc ednscookies.hh \ @@ -207,7 +207,6 @@ endif endif if HAVE_DNS_OVER_HTTPS -dnsdist_SOURCES += doh.cc if HAVE_LIBH2OEVLOOP dnsdist_LDADD += $(LIBH2OEVLOOP_LIBS) diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc index a8cf90706..8608922b9 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -1,3 +1,7 @@ +#include "config.h" +#include "doh.hh" + +#ifdef HAVE_DNS_OVER_HTTPS #define H2O_USE_EPOLL 1 #include @@ -122,6 +126,22 @@ struct DOHServerConfig int dohresponsepair[2]{-1,-1}; }; +void handleDOHTimeout(DOHUnit* oldDU) +{ + if (oldDU == nullptr) { + return; + } + +/* we are about to erase an existing DU */ + oldDU->error = true; + oldDU->status_code = 502; + + if (send(oldDU->rsock, &oldDU, sizeof(oldDU), 0) != sizeof(oldDU)) { + delete oldDU; + oldDU = nullptr; + } +} + static void on_socketclose(void *data) { DOHAcceptContext* ctx = reinterpret_cast(data); @@ -213,21 +233,31 @@ static int processDOHQuery(DOHUnit* du) return -1; } + ComboAddress dest = du->dest; unsigned int idOffset = (ss->idOffset++) % ss->idStates.size(); IDState* ids = &ss->idStates[idOffset]; ids->age = 0; - ids->du = du; + DOHUnit* oldDU = nullptr; + if (ids->origFD != -1) { + oldDU = ids->du; + } - int oldFD = ids->origFD.exchange(cs.udpFD); - if(oldFD < 0) { + int oldFD = ids->origFD.exchange(0); + if (oldFD < 0) { // if we are reusing, no change in outstanding ++ss->outstanding; } else { ++ss->reuseds; ++g_stats.downstreamTimeouts; + /* origFD is not -1 anymore, we can't touch it */ + oldDU = nullptr; } + handleDOHTimeout(oldDU); + + ids->du = du; + ids->cs = &cs; ids->origID = dh->id; setIDStateFromDNSQuestion(*ids, dq, std::move(qname)); @@ -238,8 +268,8 @@ static int processDOHQuery(DOHUnit* du) We need to keep track of which one it is since we may want to use the real but not the listening addr to reply. */ - if (du->dest.sin4.sin_family != 0) { - ids->origDest = du->dest; + if (dest.sin4.sin_family != 0) { + ids->origDest = dest; ids->destHarvested = true; } else { @@ -588,6 +618,7 @@ static void on_dnsdist(h2o_socket_t *listener, const char *err) ++dsc->df->d_errorresponses; } + delete du; } @@ -771,3 +802,11 @@ catch(const std::exception& e) { catch(...) { throw runtime_error("DOH thread failed to launch"); } + +#else /* HAVE_DNS_OVER_HTTPS */ + +void handleDOHTimeout(DOHUnit* oldDU) +{ +} + +#endif /* HAVE_DNS_OVER_HTTPS */ diff --git a/pdns/doh.hh b/pdns/doh.hh index 29f4adcc9..7d411d25f 100644 --- a/pdns/doh.hh +++ b/pdns/doh.hh @@ -74,3 +74,5 @@ struct DOHUnit }; #endif /* HAVE_DNS_OVER_HTTPS */ + +void handleDOHTimeout(DOHUnit* oldDU);