From 987dee3fdea29dcfa0035747c43ee22886a707fb Mon Sep 17 00:00:00 2001 From: Adam Date: Sun, 19 Apr 2015 21:57:38 -0400 Subject: core_dns Move packet source address checking before packet processing --- src/coremods/core_dns.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'src/coremods') diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index de8dedd4a..61d524ada 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -571,6 +571,15 @@ class MyManager : public Manager, public Timer, public EventHandler if (length < Packet::HEADER_LENGTH) return; + if (myserver != from) + { + std::string server1 = from.str(); + std::string server2 = myserver.str(); + ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "Got a result from the wrong server! Bad NAT or DNS forging attempt? '%s' != '%s'", + server1.c_str(), server2.c_str()); + return; + } + Packet recv_packet; try @@ -583,15 +592,6 @@ class MyManager : public Manager, public Timer, public EventHandler return; } - if (myserver != from) - { - std::string server1 = from.str(); - std::string server2 = myserver.str(); - ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "Got a result from the wrong server! Bad NAT or DNS forging attempt? '%s' != '%s'", - server1.c_str(), server2.c_str()); - return; - } - DNS::Request* request = this->requests[recv_packet.id]; if (request == NULL) { -- cgit v1.2.3 From 8950aeda9a42ec795ce4edf87b40135047f55f6d Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:00:01 +0200 Subject: core_dns Allow usage of id 0 --- src/coremods/core_dns.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'src/coremods') diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 61d524ada..f4559c08f 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -473,32 +473,34 @@ class MyManager : public Manager, public Timer, public EventHandler /* Create an id */ unsigned int tries = 0; + int id; do { - req->id = ServerInstance->GenRandomInt(DNS::MAX_REQUEST_ID); + id = ServerInstance->GenRandomInt(DNS::MAX_REQUEST_ID); if (++tries == DNS::MAX_REQUEST_ID*5) { // If we couldn't find an empty slot this many times, do a sequential scan as a last // resort. If an empty slot is found that way, go on, otherwise throw an exception - req->id = 0; - for (int i = 1; i < DNS::MAX_REQUEST_ID; i++) + id = -1; + for (unsigned int i = 0; i < DNS::MAX_REQUEST_ID; i++) { if (!this->requests[i]) { - req->id = i; + id = i; break; } } - if (req->id == 0) + if (id == -1) throw Exception("DNS: All ids are in use"); break; } } - while (!req->id || this->requests[req->id]); + while (this->requests[id]); + req->id = id; this->requests[req->id] = req; Packet p; -- cgit v1.2.3 From 14d15d3d2a049b07e9cad2bc2970d1fa0d51af02 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:05:49 +0200 Subject: core_dns Allow usage of id 65535 --- src/coremods/core_dns.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/coremods') diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index f4559c08f..58c275c7c 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -442,18 +442,18 @@ class MyManager : public Manager, public Timer, public EventHandler } public: - DNS::Request* requests[MAX_REQUEST_ID]; + DNS::Request* requests[MAX_REQUEST_ID+1]; MyManager(Module* c) : Manager(c), Timer(3600, true) { - for (int i = 0; i < MAX_REQUEST_ID; ++i) + for (unsigned int i = 0; i <= MAX_REQUEST_ID; ++i) requests[i] = NULL; ServerInstance->Timers.AddTimer(this); } ~MyManager() { - for (int i = 0; i < MAX_REQUEST_ID; ++i) + for (unsigned int i = 0; i <= MAX_REQUEST_ID; ++i) { DNS::Request* request = requests[i]; if (!request) @@ -476,14 +476,14 @@ class MyManager : public Manager, public Timer, public EventHandler int id; do { - id = ServerInstance->GenRandomInt(DNS::MAX_REQUEST_ID); + id = ServerInstance->GenRandomInt(DNS::MAX_REQUEST_ID+1); if (++tries == DNS::MAX_REQUEST_ID*5) { // If we couldn't find an empty slot this many times, do a sequential scan as a last // resort. If an empty slot is found that way, go on, otherwise throw an exception id = -1; - for (unsigned int i = 0; i < DNS::MAX_REQUEST_ID; i++) + for (unsigned int i = 0; i <= DNS::MAX_REQUEST_ID; i++) { if (!this->requests[i]) { @@ -807,7 +807,7 @@ class ModuleDNS : public Module void OnUnloadModule(Module* mod) { - for (int i = 0; i < MAX_REQUEST_ID; ++i) + for (unsigned int i = 0; i <= MAX_REQUEST_ID; ++i) { DNS::Request* req = this->manager.requests[i]; if (!req) -- cgit v1.2.3 From 4e58282058fe9d57f0ef1d557e88ffdbdbf01166 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:08:10 +0200 Subject: core_dns Add typedef for request id, change it to uint16_t --- include/modules/dns.h | 4 +++- src/coremods/core_dns.cpp | 5 +---- 2 files changed, 4 insertions(+), 5 deletions(-) (limited to 'src/coremods') diff --git a/include/modules/dns.h b/include/modules/dns.h index 400d2085d..04e3df16c 100644 --- a/include/modules/dns.h +++ b/include/modules/dns.h @@ -68,6 +68,8 @@ namespace DNS ERROR_INVALIDTYPE }; + typedef uint16_t RequestId; + const int PORT = 53; /** @@ -147,7 +149,7 @@ namespace DNS /* Use result cache if available */ bool use_cache; /* Request id */ - unsigned short id; + RequestId id; /* Creator of this request */ Module* const creator; diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 58c275c7c..703dfdbdc 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -201,7 +201,7 @@ class Packet : public Query static const int HEADER_LENGTH = 12; /* ID for this packet */ - unsigned short id; + RequestId id; /* Flags on the packet */ unsigned short flags; @@ -219,9 +219,6 @@ class Packet : public Query this->id = (input[packet_pos] << 8) | input[packet_pos + 1]; packet_pos += 2; - if (this->id >= MAX_REQUEST_ID) - throw Exception("Query ID too large?"); - this->flags = (input[packet_pos] << 8) | input[packet_pos + 1]; packet_pos += 2; -- cgit v1.2.3 From 402779cc65f8e665c160a6902d585d7713e9b0a3 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:10:07 +0200 Subject: core_dns Remove ability to pack answers --- src/coremods/core_dns.cpp | 79 ++--------------------------------------------- 1 file changed, 2 insertions(+), 77 deletions(-) (limited to 'src/coremods') diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 703dfdbdc..891c6705a 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -256,8 +256,8 @@ class Packet : public Query output[pos++] = this->flags & 0xFF; output[pos++] = this->questions.size() >> 8; output[pos++] = this->questions.size() & 0xFF; - output[pos++] = this->answers.size() >> 8; - output[pos++] = this->answers.size() & 0xFF; + output[pos++] = 0; // Answer count, high byte + output[pos++] = 0; // Answer count, low byte output[pos++] = 0; output[pos++] = 0; output[pos++] = 0; @@ -312,81 +312,6 @@ class Packet : public Query pos += 2; } - for (unsigned int i = 0; i < answers.size(); i++) - { - ResourceRecord& rr = answers[i]; - - this->PackName(output, output_size, pos, rr.name); - - if (pos + 8 >= output_size) - throw Exception("Unable to pack packet"); - - short s = htons(rr.type); - memcpy(&output[pos], &s, 2); - pos += 2; - - s = htons(rr.qclass); - memcpy(&output[pos], &s, 2); - pos += 2; - - long l = htonl(rr.ttl); - memcpy(&output[pos], &l, 4); - pos += 4; - - switch (rr.type) - { - case QUERY_A: - { - if (pos + 6 > output_size) - throw Exception("Unable to pack packet"); - - irc::sockets::sockaddrs a; - irc::sockets::aptosa(rr.rdata, 0, a); - - s = htons(4); - memcpy(&output[pos], &s, 2); - pos += 2; - - memcpy(&output[pos], &a.in4.sin_addr, 4); - pos += 4; - break; - } - case QUERY_AAAA: - { - if (pos + 18 > output_size) - throw Exception("Unable to pack packet"); - - irc::sockets::sockaddrs a; - irc::sockets::aptosa(rr.rdata, 0, a); - - s = htons(16); - memcpy(&output[pos], &s, 2); - pos += 2; - - memcpy(&output[pos], &a.in6.sin6_addr, 16); - pos += 16; - break; - } - case QUERY_CNAME: - case QUERY_PTR: - { - if (pos + 2 >= output_size) - throw Exception("Unable to pack packet"); - - unsigned short packet_pos_save = pos; - pos += 2; - - this->PackName(output, output_size, pos, rr.rdata); - - s = htons(pos - packet_pos_save - 2); - memcpy(&output[packet_pos_save], &s, 2); - break; - } - default: - break; - } - } - return pos; } }; -- cgit v1.2.3 From a28f095db832e44aad66bfd73bfd8176a97c6de2 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:14:29 +0200 Subject: core_dns Don't store query class code in Question --- include/modules/dns.h | 9 ++++----- src/coremods/core_dns.cpp | 8 ++++---- 2 files changed, 8 insertions(+), 9 deletions(-) (limited to 'src/coremods') diff --git a/include/modules/dns.h b/include/modules/dns.h index 04e3df16c..f4071e399 100644 --- a/include/modules/dns.h +++ b/include/modules/dns.h @@ -88,11 +88,10 @@ namespace DNS { std::string name; QueryType type; - unsigned short qclass; - Question() : type(QUERY_NONE), qclass(0) { } - Question(const std::string& n, QueryType t, unsigned short c = 1) : name(n), type(t), qclass(c) { } - inline bool operator==(const Question& other) const { return name == other.name && type == other.type && qclass == other.qclass; } + Question() : type(QUERY_NONE) { } + Question(const std::string& n, QueryType t) : name(n), type(t) { } + bool operator==(const Question& other) const { return ((name == other.name) && (type == other.type)); } struct hash { @@ -109,7 +108,7 @@ namespace DNS std::string rdata; time_t created; - ResourceRecord(const std::string& n, QueryType t, unsigned short c = 1) : Question(n, t, c), ttl(0), created(ServerInstance->Time()) { } + ResourceRecord(const std::string& n, QueryType t) : Question(n, t), ttl(0), created(ServerInstance->Time()) { } ResourceRecord(const Question& question) : Question(question), ttl(0), created(ServerInstance->Time()) { } }; diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 891c6705a..999da8356 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -126,7 +126,7 @@ class Packet : public Query question.type = static_cast(input[pos] << 8 | input[pos + 1]); pos += 2; - question.qclass = input[pos] << 8 | input[pos + 1]; + // Skip over query class code pos += 2; return question; @@ -307,9 +307,9 @@ class Packet : public Query memcpy(&output[pos], &s, 2); pos += 2; - s = htons(q.qclass); - memcpy(&output[pos], &s, 2); - pos += 2; + // Query class, always IN + output[pos++] = 0; + output[pos++] = 1; } return pos; -- cgit v1.2.3 From 92959a48b05aa982d5b1331622cb28197d38e9da Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:17:02 +0200 Subject: core_dns Reject incoming packets with qdcount != 1 --- src/coremods/core_dns.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src/coremods') diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 999da8356..8911f5b55 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -236,6 +236,9 @@ class Packet : public Query ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "qdcount: " + ConvToStr(qdcount) + " ancount: " + ConvToStr(ancount) + " nscount: " + ConvToStr(nscount) + " arcount: " + ConvToStr(arcount)); + if (qdcount != 1) + throw Exception("Question count != 1 in incoming packet"); + for (unsigned i = 0; i < qdcount; ++i) this->questions.push_back(this->UnpackQuestion(input, len, packet_pos)); -- cgit v1.2.3 From edfe7f035ea83731b9316d40d64402411a531426 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Tue, 21 Apr 2015 15:24:10 +0200 Subject: core_dns Remove incomplete support for multiple questions per query --- include/modules/dns.h | 4 ++-- src/coremods/core_dns.cpp | 28 +++++++++++++--------------- 2 files changed, 15 insertions(+), 17 deletions(-) (limited to 'src/coremods') diff --git a/include/modules/dns.h b/include/modules/dns.h index f4071e399..4da31b805 100644 --- a/include/modules/dns.h +++ b/include/modules/dns.h @@ -114,13 +114,13 @@ namespace DNS struct Query { - std::vector questions; + Question question; std::vector answers; Error error; bool cached; Query() : error(ERROR_NONE), cached(false) { } - Query(const Question& question) : error(ERROR_NONE), cached(false) { questions.push_back(question); } + Query(const Question& q) : question(q), error(ERROR_NONE), cached(false) { } }; class ReplySocket; diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 8911f5b55..a7ed8d3ed 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -116,20 +116,20 @@ class Packet : public Query Question UnpackQuestion(const unsigned char* input, unsigned short input_size, unsigned short& pos) { - Question question; + Question q; - question.name = this->UnpackName(input, input_size, pos); + q.name = this->UnpackName(input, input_size, pos); if (pos + 4 > input_size) throw Exception("Unable to unpack question"); - question.type = static_cast(input[pos] << 8 | input[pos + 1]); + q.type = static_cast(input[pos] << 8 | input[pos + 1]); pos += 2; // Skip over query class code pos += 2; - return question; + return q; } ResourceRecord UnpackResourceRecord(const unsigned char* input, unsigned short input_size, unsigned short& pos) @@ -239,8 +239,7 @@ class Packet : public Query if (qdcount != 1) throw Exception("Question count != 1 in incoming packet"); - for (unsigned i = 0; i < qdcount; ++i) - this->questions.push_back(this->UnpackQuestion(input, len, packet_pos)); + this->question = this->UnpackQuestion(input, len, packet_pos); for (unsigned i = 0; i < ancount; ++i) this->answers.push_back(this->UnpackResourceRecord(input, len, packet_pos)); @@ -257,8 +256,8 @@ class Packet : public Query output[pos++] = this->id & 0xFF; output[pos++] = this->flags >> 8; output[pos++] = this->flags & 0xFF; - output[pos++] = this->questions.size() >> 8; - output[pos++] = this->questions.size() & 0xFF; + output[pos++] = 0; // Question count, high byte + output[pos++] = 1; // Question count, low byte output[pos++] = 0; // Answer count, high byte output[pos++] = 0; // Answer count, low byte output[pos++] = 0; @@ -266,9 +265,8 @@ class Packet : public Query output[pos++] = 0; output[pos++] = 0; - for (unsigned i = 0; i < this->questions.size(); ++i) { - Question& q = this->questions[i]; + Question& q = this->question; if (q.type == QUERY_PTR) { @@ -363,7 +361,7 @@ class MyManager : public Manager, public Timer, public EventHandler { const ResourceRecord& rr = r.answers[0]; ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "cache: added cache for " + rr.name + " -> " + rr.rdata + " ttl: " + ConvToStr(rr.ttl)); - this->cache[r.questions[0]] = r; + this->cache[r.question] = r; } public: @@ -431,15 +429,15 @@ class MyManager : public Manager, public Timer, public EventHandler Packet p; p.flags = QUERYFLAGS_RD; p.id = req->id; - p.questions.push_back(*req); + p.question = *req; unsigned char buffer[524]; unsigned short len = p.Pack(buffer, sizeof(buffer)); - /* Note that calling Pack() above can actually change the contents of p.questions[0].name, if the query is a PTR, + /* Note that calling Pack() above can actually change the contents of p.question.name, if the query is a PTR, * to contain the value that would be in the DNS cache, which is why this is here. */ - if (req->use_cache && this->CheckCache(req, p.questions[0])) + if (req->use_cache && this->CheckCache(req, p.question)) { ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "Using cached result"); delete req; @@ -567,7 +565,7 @@ class MyManager : public Manager, public Timer, public EventHandler recv_packet.error = error; request->OnError(&recv_packet); } - else if (recv_packet.questions.empty() || recv_packet.answers.empty()) + else if (recv_packet.answers.empty()) { ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "No resource records returned"); ServerInstance->stats.DnsBad++; -- cgit v1.2.3 From ee10c33eba66d20d6471d81b5f4e3484db30cfe6 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Mon, 27 Apr 2015 19:23:14 +0200 Subject: core_dns Update DNS::Request::name to be the same as in the packet --- src/coremods/core_dns.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src/coremods') diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index a7ed8d3ed..f4f85e253 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -444,6 +444,9 @@ class MyManager : public Manager, public Timer, public EventHandler return; } + // Update name in the original request so question checking works for PTR queries + req->name = p.question.name; + if (SocketEngine::SendTo(this, buffer, len, 0, &this->myserver.sa, this->myserver.sa_size()) != len) throw Exception("DNS: Unable to send query"); } -- cgit v1.2.3 From 3d32852e7e459d403179e8f79354e2e52bbc4a52 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Mon, 27 Apr 2015 19:28:00 +0200 Subject: core_dns Drop incoming packets containing a different question from what we asked --- include/modules/dns.h | 1 + src/coremods/core_dns.cpp | 7 +++++++ 2 files changed, 8 insertions(+) (limited to 'src/coremods') diff --git a/include/modules/dns.h b/include/modules/dns.h index 4da31b805..cfa048184 100644 --- a/include/modules/dns.h +++ b/include/modules/dns.h @@ -92,6 +92,7 @@ namespace DNS Question() : type(QUERY_NONE) { } Question(const std::string& n, QueryType t) : name(n), type(t) { } bool operator==(const Question& other) const { return ((name == other.name) && (type == other.type)); } + bool operator!=(const Question& other) const { return (!(*this == other)); } struct hash { diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index f4f85e253..d4214b9a5 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -527,6 +527,13 @@ class MyManager : public Manager, public Timer, public EventHandler return; } + if (static_cast(*request) != recv_packet.question) + { + // This can happen under high latency, drop it silently, do not fail the request + ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "Received an answer that isn't for a question we asked"); + return; + } + if (recv_packet.flags & QUERYFLAGS_OPCODE) { ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "Received a nonstandard query"); -- cgit v1.2.3