Rework DNSServer to be more robust #5573

SimonWilkinson · 2019-01-02T12:44:16Z

These changes rework DNSServer so that it's more robust in the face of real-world DNS queries. They should resolve issues #5534 #5529 and #3357

devyte

Overall this looks very good, I can clearly see additional sanity checks that are currently missing, and the code does loo a bit cleaner.
My comments are minor, mostly about readability and style.

libraries/DNSServer/src/DNSServer.h

libraries/DNSServer/src/DNSServer.cpp

devyte · 2019-01-03T19:18:19Z

libraries/DNSServer/src/DNSServer.cpp

-    {
-      replyWithCustomCode(buffer, packetSize);
-    }
+  buffer = (unsigned char *)malloc(currentPacketSize);


instead of malloc to a C-style ptr, and then manually freeing below, consider using a smart pointer. E.g.:
std::unique_ptr<uint8_t*>(new uint8_t[currentPacketSize]);
Notice the use of uint8_t* instead of unsigned char*. Although it's the same underlying type, we use this distinction to indicate that the ptr is meant for binary data and not for chars.

Done, I hope. I'm not particularly familiar with smart pointers (spend most of my life in C), so please check this bit carefully!

Not bad for a first try, but it's not quite right 😄

Keep in mind that in C++ you don't need to declare all vars at the top. In the case of smart pointers, there are reasons to not do so, and declare and construct in one go. My suggestion was this:

std::unique_ptr<uint8_t[]> buffer(new uint8_t(std::nothrow) [currentPacketSize]);

Then pass the internal buffer to respondToRequest like so:

respondToRequest(*buffer, currentPacketSize)

or so:

respondToRequest(buffer.get(), currentPacketSize)

With that, respondToRequest can receive a uint8_t * as before (no need to change the arg type to be unique_ptr).

I've changed this as suggested

libraries/DNSServer/src/DNSServer.cpp

Just replacing 'www.' with the empty string when we assign the domainname will remove all occurrences of 'www.', not just those at the start of the string. Change this to a startsWith check so that only "www." at the beginning of the string is removed.

SimonWilkinson · 2019-01-04T12:13:59Z

I've updated this PR with all of the requested changes

devyte · 2019-01-04T14:58:08Z

libraries/DNSServer/src/DNSServer.cpp

+  respondToRequest(buffer, currentPacketSize);
+}
+
+void DNSServer::writeNBOShort(uint16_t value)


I like this factoring out, but my previous comment about the literal 2 in the write() below is still not addressed.
I suggest the following:
_udp.write((usigned char *)&value, sizeof(value));

I'll change this. Writing 2 octets of data here is a property of the protocol, though, and not of the C++ representation that's feeding it.

I understand, and that's why I like the factoring out into a function: it makes it clear that it's a protocol property. My comment is more for maintainability and readability. If the function ever gets copied for e.g.: 4 octets or whatever, or templated for the arg type, with sizeof() there's no need to change anything => less error-prone. And when glancing at the code, sizeof() makes what is going on evident.
It's just good habits and programming guidelines, not that there's anything with your original code.

Also, I'll be porting these changes to my async dnsserver 😆

libraries/DNSServer/src/DNSServer.h

devyte · 2019-01-04T15:01:58Z

libraries/DNSServer/src/DNSServer.h

@@ -1,12 +1,20 @@
 #ifndef DNSServer_h
 #define DNSServer_h
 #include <WiFiUdp.h>
+#include <memory>


As I explained in my reply, this include shouldn't be necessary, because you can keep the signature for respondToRequest as it was before.

devyte · 2019-01-04T15:02:40Z

libraries/DNSServer/src/DNSServer.h

+			size_t queryLength);
+    void replyWithError(DNSHeader *dnsHeader,
+			DNSReplyCode rcode);
+    void respondToRequest(std::unique_ptr<uint8_t []> & buffer, size_t length);


As I explained in my reply, this change in signature shouldn't be necessary, because you won't be passing the unique_ptr, but rather its internal buffer.

Changed it back

Rewrite the request handling in the DNSServer code to address the following issues: Compatibility with EDNS esp8266#1: RFC6891 says that "Responders that choose not to implement the protocol extensions defined in this document MUST respond with a return code (RCODE) of FORMERR to messages containing an OPT record in the additional section and MUST NOT include an OPT record in the response" If we have any additional records in the request, then we need to return a FORMERR, and not whatever custom error code the user may have set. Compatibility with EDNS esp8266#2: If we're returning an error, we need to explicitly zero all of the record counters. In the existing code, if there is an additional record present in the request, we return an ARCOUNT of 1 in the response, despite including no additional records in the payload. Don't answer non-A requests If we receive an AAAA request (or any other non-A record) requests, we shouldn't respond to it with an A record. Don't answer non-IN requests If we receive a request for a non-IN type, don't answer it (it's unlikely that we'd see this in the real world) Don't read off the end of malformed packets If a packet claims to have a query, but then doesn't include one, or includes a query with malformed labels, don't read off the end of the allocated data structure.

Modify the code used to write the answer record back to the server so that it is clearer that we are writing network byte order 16-bit quantities, and to clarify what's happening with the pointer used at the start of the answer.

devyte

I'm ok with this. Is there anything further you want to add?

SimonWilkinson · 2019-01-05T15:44:45Z

No, I think that’s it for now. It might be interesting to actually add proper EDNS support at some point in the future, but that would be a separate PR.

devyte · 2019-01-05T15:46:38Z

If you're up for it later on, I'll take a look.
In the mean time, thank you for this PR!

devyte self-requested a review January 2, 2019 19:50

devyte requested changes Jan 4, 2019

View reviewed changes

SimonWilkinson force-pushed the dnsserver-edns branch from 64fef43 to ec481d3 Compare January 4, 2019 12:12

devyte requested changes Jan 4, 2019

View reviewed changes

SimonWilkinson added 2 commits January 5, 2019 11:17

DNSServer: Clarify and tidy writing the answer record

ed919eb

Modify the code used to write the answer record back to the server so that it is clearer that we are writing network byte order 16-bit quantities, and to clarify what's happening with the pointer used at the start of the answer.

SimonWilkinson force-pushed the dnsserver-edns branch from ec481d3 to ed919eb Compare January 5, 2019 11:19

devyte approved these changes Jan 5, 2019

View reviewed changes

devyte merged commit eaac1e8 into esp8266:master Jan 5, 2019

devyte self-assigned this Jan 5, 2019

devyte added type: bug component: libraries labels Jan 5, 2019

devyte added this to the 2.5.0 milestone Jan 5, 2019

devyte mentioned this pull request Jan 5, 2019

DNSServer does not work with dig unix tool #5534

Closed

6 tasks

earlephilhower mentioned this pull request Feb 24, 2020

DNSServer does not Answer request from Ubuntu 17.04 #3357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework DNSServer to be more robust #5573

Rework DNSServer to be more robust #5573

SimonWilkinson commented Jan 2, 2019

devyte left a comment

devyte Jan 3, 2019

SimonWilkinson Jan 4, 2019

devyte Jan 4, 2019 •

edited

Loading

SimonWilkinson Jan 4, 2019

SimonWilkinson commented Jan 4, 2019

devyte Jan 4, 2019

SimonWilkinson Jan 4, 2019

devyte Jan 4, 2019

devyte Jan 4, 2019

SimonWilkinson Jan 4, 2019

devyte Jan 4, 2019

SimonWilkinson Jan 4, 2019

devyte left a comment

SimonWilkinson commented Jan 5, 2019

devyte commented Jan 5, 2019

Rework DNSServer to be more robust #5573

Rework DNSServer to be more robust #5573

Conversation

SimonWilkinson commented Jan 2, 2019

devyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devyte Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonWilkinson commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devyte left a comment

Choose a reason for hiding this comment

SimonWilkinson commented Jan 5, 2019

devyte commented Jan 5, 2019

devyte Jan 4, 2019 •

edited

Loading