Skip to content

Proper EDNS handling and cleaner NOERROR response in DNSSERVER #11411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/DNSServer/library.properties
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name=DNSServer
version=3.2.0
version=3.3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this version is updated with every new release. Please revert

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the change with new commit

author=Kristijan Novoselić
maintainer=Kristijan Novoselić, <kristijan.novoselic@gmail.com>
sentence=A simple DNS server for ESP32.
Expand Down
89 changes: 84 additions & 5 deletions libraries/DNSServer/src/DNSServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
DNSServer::DNSServer() : _port(DNS_DEFAULT_PORT), _ttl(htonl(DNS_DEFAULT_TTL)), _errorReplyCode(DNSReplyCode::NonExistentDomain) {}

DNSServer::DNSServer(const String &domainName)
: _port(DNS_DEFAULT_PORT), _ttl(htonl(DNS_DEFAULT_TTL)), _errorReplyCode(DNSReplyCode::NonExistentDomain), _domainName(domainName){};
: _port(DNS_DEFAULT_PORT), _ttl(htonl(DNS_DEFAULT_TTL)), _errorReplyCode(DNSReplyCode::NonExistentDomain), _domainName(domainName) {};

bool DNSServer::start() {
if (_resolvedIP.operator uint32_t() == 0) { // no address is set, try to obtain AP interface's IP
Expand Down Expand Up @@ -111,16 +111,22 @@ void DNSServer::_handleUDP(AsyncUDPPacket &pkt) {
// will reply with IP only to "*" or if domain matches without www. subdomain
if (dnsHeader.OPCode == DNS_OPCODE_QUERY && requestIncludesOnlyOneQuestion(dnsHeader)
&& (_domainName.isEmpty() || getDomainNameWithoutWwwPrefix(static_cast<const unsigned char *>(dnsQuestion.QName), dnsQuestion.QNameLength) == _domainName)) {
replyWithIP(pkt, dnsHeader, dnsQuestion);

// Qtype = A (1) or ANY (255): send an A record otherwise an empty response
if (ntohs(dnsQuestion.QType) == 1 || ntohs(dnsQuestion.QType) == 255) {
replyWithIP(pkt, dnsHeader, dnsQuestion);
} else {
replyWithNoAnsw(pkt, dnsHeader, dnsQuestion);
}
return;
}

// otherwise reply with custom code
replyWithCustomCode(pkt, dnsHeader);
}

bool DNSServer::requestIncludesOnlyOneQuestion(DNSHeader &dnsHeader) {
return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0 && dnsHeader.ARCount == 0;
dnsHeader.ARCount = 0; // We assume that if ARCount !=0 there is a EDNS OPT packet, just ignore
return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0; // && dnsHeader.ARCount == 0;
Copy link
Preview

Copilot AI Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating dnsHeader.ARCount directly can lead to unexpected side effects since the header is reused for reply. Consider checking ARCount in a local variable or on a copy instead of overwriting the original field.

Suggested change
dnsHeader.ARCount = 0; // We assume that if ARCount !=0 there is a EDNS OPT packet, just ignore
return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0; // && dnsHeader.ARCount == 0;
uint16_t arCount = dnsHeader.ARCount; // Store ARCount in a local variable
return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0 && arCount == 0;

Copilot uses AI. Check for mistakes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a bit of dead code here (line 129), I removed it.
Co pilot seemed to be hallucinating for its other suggestions

}

String DNSServer::getDomainNameWithoutWwwPrefix(const unsigned char *start, size_t len) {
Expand All @@ -139,7 +145,6 @@ String DNSServer::getDomainNameWithoutWwwPrefix(const unsigned char *start, size

void DNSServer::replyWithIP(AsyncUDPPacket &req, DNSHeader &dnsHeader, DNSQuestion &dnsQuestion) {
AsyncUDPMessage rpl;

// Change the type of message to a response and set the number of answers equal to
// the number of questions in the header
dnsHeader.QR = DNS_QR_RESPONSE;
Expand Down Expand Up @@ -187,3 +192,77 @@ void DNSServer::replyWithCustomCode(AsyncUDPPacket &req, DNSHeader &dnsHeader) {
rpl.write(reinterpret_cast<const uint8_t *>(&dnsHeader), sizeof(DNSHeader));
_udp.sendTo(rpl, req.remoteIP(), req.remotePort());
}

void DNSServer::replyWithNoAnsw(AsyncUDPPacket &req, DNSHeader &dnsHeader, DNSQuestion &dnsQuestion) {

dnsHeader.QR = DNS_QR_RESPONSE;
// dnsHeader.QDCount = 1;
dnsHeader.ANCount = 0;
dnsHeader.NSCount = htons(1);

AsyncUDPMessage rpl;
rpl.write(reinterpret_cast<const uint8_t *>(&dnsHeader), sizeof(DNSHeader));

// Write the question
rpl.write(dnsQuestion.QName, dnsQuestion.QNameLength);
rpl.write((uint8_t *)&dnsQuestion.QType, 2);
rpl.write((uint8_t *)&dnsQuestion.QClass, 2);

// An empty answer contains an authority section with a SOA,
// We take the name of the query as the root of the zone for which the SOA is generated
// and use a value of DNS_MINIMAL_TTL seconds in order to minimize negative caching
// Write the authority section:
// The SOA RR's ownername is set equal to the query name, and we use made up names for
// the MNAME and RNAME - it doesn't really matter from a protocol perspective - as for
// a no such QTYPE answer only the timing fields are used.
// a protocol perspective - it
// Use DNS name compression : instead of repeating the name in this RNAME occurrence,
// set the two MSB of the byte corresponding normally to the length to 1. The following
// 14 bits must be used to specify the offset of the domain name in the message
// (<255 here so the first byte has the 6 LSB at 0)
rpl.write((uint8_t)0xC0);
rpl.write((uint8_t)DNS_OFFSET_DOMAIN_NAME);

// DNS type A : host address, DNS class IN for INternet, returning an IPv4 address
uint16_t answerType = htons(DNS_TYPE_SOA), answerClass = htons(DNS_CLASS_IN);
uint32_t Serial = htonl(DNS_SOA_SERIAL); // Date type serial based on the date this piece of code was written
uint32_t Refresh = htonl(DNS_SOA_REFRESH); // These timers don't matter, we don't serve zone transfers
uint32_t Retry = htonl(DNS_SOA_RETRY);
uint32_t Expire = htonl(DNS_SOA_EXPIRE);
uint32_t MinTTL = htonl(DNS_MINIMAL_TTL); // See RFC2308 section 5
char MLabel[] = DNS_SOA_MNAME_LABEL;
char RLabel[] = DNS_SOA_RNAME_LABEL;
char PostFixLabel[] = DNS_SOA_POSTFIX_LABEL;

// 4 accounts for len fields and for both rname
// and lname and their postfix labels and there are 5 32 bit fields

uint16_t RdataLength = htons((uint16_t)(strlen(MLabel) + strlen(RLabel) + 2 * strlen(PostFixLabel) + 4 + 5 * sizeof(Serial)));

rpl.write((unsigned char *)&answerType, 2);
rpl.write((unsigned char *)&answerClass, 2);
rpl.write((unsigned char *)&MinTTL, 4); // DNS Time To Live

rpl.write((unsigned char *)&RdataLength, 2);

rpl.write((uint8_t)strlen(MLabel));
rpl.write((unsigned char *)&MLabel, strlen(MLabel));

rpl.write((unsigned char *)&PostFixLabel, strlen(PostFixLabel));
rpl.write((uint8_t)0);
// rpl.write((uint8_t)0xC0);
// rpl.write((uint8_t)DNS_OFFSET_DOMAIN_NAME);

rpl.write((uint8_t)strlen(RLabel));
rpl.write((unsigned char *)&RLabel, strlen(RLabel));
rpl.write((unsigned char *)&PostFixLabel, strlen(PostFixLabel));
rpl.write((uint8_t)0);

rpl.write((unsigned char *)&Serial, 4);
rpl.write((unsigned char *)&Refresh, 4);
rpl.write((unsigned char *)&Retry, 4);
rpl.write((unsigned char *)&Expire, 4);
rpl.write((unsigned char *)&MinTTL, 4);

_udp.sendTo(rpl, req.remoteIP(), req.remotePort());
}
21 changes: 20 additions & 1 deletion libraries/DNSServer/src/DNSServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,23 @@
#define DNS_OFFSET_DOMAIN_NAME DNS_HEADER_SIZE // Offset in bytes to reach the domain name labels in the DNS message
#define DNS_DEFAULT_PORT 53

#define DNS_SOA_MNAME_LABEL "ns"
#define DNS_SOA_RNAME_LABEL "esp32"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are MNAME and RNAME? How are their values chosen?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MNAME and RNAME are values defined in RFC1035 section 3.1.13.

Tne MNAME is the name of the primary nameserver the request came from - that value is only used in the context of dynamic DNS updates RFC 2136. In this contexts it is purely informational. I chose the 'ns' label as that is sort of customary for nameservers.

The RNAME is used to construct the email address of the administrator for the zone. The way things are coded now that is esp32@local. There nothing in the DNS protocol that uses that value, it is only for network administrator information siganling purpose.

// The POSTFIX_LABEL will be concatinated to the RName and MName Label label
// do not use a multilabel name here. "local" is a good choice as it is reserved for
// local use by IANA
// The postfix label is defined as an array of characters that follows the
// definition of RFC1035 3.1
// for instance, a postfix of example.com would be defined as:
// #define DNS_SOA_POSTFIX_LABEL {'\7', 'e', 'x', 'a', 'm', 'p', 'l', 'e', '\3', 'c', 'o', 'm', '\0'}
#define DNS_SOA_POSTFIX_LABEL {'\5', 'l', 'o', 'c', 'a', 'l', '\0'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.local is used by MDNS. Is that the best value?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent question, because it surfaces a dilemma that I had.

Here are the considerations:

  • The MNAME and RNAME are purely informational still you do not want something to be used on the real Internet.
  • But, suppose that somebody queries a DNSSERVER on an ESP and decides to complain to the admin who is indicated in the RNAME. Then you don't want that mail to end up with an existing person somewhere on the Internet.
  • The domains in the MNAME and RNAME could be based on the queryname, but that would be problematic for wildcard resolution, where the administrator email might become esp32@captive.apple.com, so I figure that is a bad choice.
  • One has to pick a value that is never going to appear in the real DNS. The IANA maintains a registry of those.
  • I think invalid might be a good choice. RFC 6761 says that trying to resolve invalid must always expected to fail. However - folk seeing 'invalid' in their soa record during troubleshooting might misinterpret what is happening.
  • .local demonstrates that something local is going on - which is the most likely place where ESP32s get deployed.

As said, there is nothing in the DNS (except for dynamic updates) that will use the M- and RNAME values, hence no interference with MDNS - the values are purely used in the context of local troubleshooting.

// From the following values only the MINIMAL_TTL has relevance
// in the context of client-server protocol interactions.
#define DNS_SOA_SERIAL 2025052900 // Arbitrary
#define DNS_SOA_REFRESH 100000 // Arbitrary
#define DNS_SOA_RETRY 10000 // Arbitrary
#define DNS_SOA_EXPIRE 1000000 // Arbitrary
#define DNS_MINIMAL_TTL 5 // Time to live for negative answers RFC2308
enum class DNSReplyCode : uint16_t {
NoError = 0,
FormError = 1,
Expand Down Expand Up @@ -82,7 +99,7 @@ class DNSServer {
* @param domainName - domain name to serve
*/
DNSServer(const String &domainName);
~DNSServer(){}; // default d-tor
~DNSServer() {}; // default d-tor

// Copy semantics not implemented (won't run on same UDP port anyway)
DNSServer(const DNSServer &) = delete;
Expand Down Expand Up @@ -179,5 +196,7 @@ class DNSServer {
inline bool requestIncludesOnlyOneQuestion(DNSHeader &dnsHeader);
void replyWithIP(AsyncUDPPacket &req, DNSHeader &dnsHeader, DNSQuestion &dnsQuestion);
inline void replyWithCustomCode(AsyncUDPPacket &req, DNSHeader &dnsHeader);
inline void replyWithNoAnsw(AsyncUDPPacket &req, DNSHeader &dnsHeader, DNSQuestion &dnsQuestion);

void _handleUDP(AsyncUDPPacket &pkt);
};