Some upstream DNS servers are non-compliant with EDNS options

Some DNS servers don't properly ignore unknown EDNS options as the spec says they must, and instead will return EFORMERR.

See discussion roughly starting here: alpinelinux/docker-alpine#366 (comment)

In this case the DNS server is known to support EDNS in general (as version prior to c-ares 1.33 worked which used EDNS), but when adding the EDNS DNS Cookie extension, they return EFORMERR.  This is in violation of [RFC6891 6.1.2](https://datatracker.ietf.org/doc/html/rfc6891#section-6.1.2):
> Any OPTION-CODE values not understood by a responder or requestor MUST be ignored.

The server in this example actual echo's back the EDNS record further causing confusion that makes you think they might understand the record.

We need to catch an EFORMERR and re-attempt the query without EDNS completely since they are really non-compliant with EDNS.  We may support additional EDNS extensions in the future and don't want to have to probe each individual extension with a braindead server.

Fixes c-ares#911
Authored-By: Brad House (@bradh352)

src/lib/ares_process.c

@@ -650,6 +650,51 @@ static ares_status_t rewrite_without_edns(ares_query_t *query)
   return status;
 }
 
+static ares_bool_t issue_might_be_edns(const ares_dns_record_t *req,
+                                       const ares_dns_record_t *rsp)
+{
+  const ares_dns_rr_t *rr;
+
+  /* If we use EDNS and server answers with FORMERR without an OPT RR, the
+   * protocol extension is not understood by the responder. We must retry the
+   * query without EDNS enabled. */
+  if (ares_dns_record_get_rcode(rsp) != ARES_RCODE_FORMERR) {
+    return ARES_FALSE;
+  }
+
+  rr = ares_dns_get_opt_rr_const(req);
+  if (rr == NULL) {
+    /* We didn't send EDNS */
+    return ARES_FALSE;
+  }
+
+  if (ares_dns_get_opt_rr_const(rsp) == NULL) {
+    /* Spec says EDNS won't be echo'd back on non-supporting servers, so
+     * retry without EDNS */
+    return ARES_TRUE;
+  }
+
+  /* As per issue #911 some non-compliant servers that do indeed support EDNS
+   * but don't support unrecognized option codes exist.  At this point we
+   * expect them to have also returned an EDNS opt record, but we may remove
+   * that check in the future. Lets detect this situation if we're sending
+   * option codes */
+  if (ares_dns_rr_get_opt_cnt(rr, ARES_RR_OPT_OPTIONS) == 0) {
+    /* We didn't send any option codes */
+    return ARES_FALSE;
+  }
+
+  if (ares_dns_get_opt_rr_const(rsp) != NULL) {
+    /* At this time we're requiring the server to respond with EDNS opt
+     * records since that's what has been observed in the field.  We might
+     * find in the future we have to remove this, who knows. Lets go
+     * ahead and force a retry without EDNS*/
+    return ARES_TRUE;
+  }
+
+  return ARES_FALSE;
+}
+
 /* Handle an answer from a server. This must NEVER cleanup the
  * server connection! Return something other than ARES_SUCCESS to cause
  * the connection to be terminated after this call. */
@@ -713,12 +758,10 @@ static ares_status_t process_answer(ares_channel_t      *channel,
   ares_llist_node_destroy(query->node_queries_to_conn);
   query->node_queries_to_conn = NULL;
 
-  /* If we use EDNS and server answers with FORMERR without an OPT RR, the
-   * protocol extension is not understood by the responder. We must retry the
-   * query without EDNS enabled. */
-  if (ares_dns_record_get_rcode(rdnsrec) == ARES_RCODE_FORMERR &&
-      ares_dns_get_opt_rr_const(query->query) != NULL &&
-      ares_dns_get_opt_rr_const(rdnsrec) == NULL) {
+  /* There are old servers that don't understand EDNS at all, then some servers
+   * that have non-compliant implementations.  Lets try to detect this sort
+   * of thing. */
+  if (issue_might_be_edns(query->query, rdnsrec)) {
     status = rewrite_without_edns(query);
     if (status != ARES_SUCCESS) {
       end_query(channel, server, query, status, NULL);

test/ares-test-mock.cc

@@ -790,6 +790,29 @@ TEST_P(MockEDNSChannelTest, RetryWithoutEDNS) {
   EXPECT_EQ("{'www.google.com' aliases=[] addrs=[1.2.3.4]}", ss.str());
 }
 
+
+// Issue #911
+TEST_P(MockUDPChannelTest, RetryWithoutEDNSNonCompliant) {
+  DNSPacket rspfail;
+  rspfail.set_response().set_aa().set_rcode(FORMERR)
+    .add_question(new DNSQuestion("www.google.com", T_A))
+    .add_additional(new DNSOptRR(0, 0, 0, 1280, { }, { }, false));
+  DNSPacket rspok;
+  rspok.set_response()
+    .add_question(new DNSQuestion("www.google.com", T_A))
+    .add_answer(new DNSARR("www.google.com", 100, {1, 2, 3, 4}));
+  EXPECT_CALL(server_, OnRequest("www.google.com", T_A))
+    .WillOnce(SetReply(&server_, &rspfail))
+    .WillOnce(SetReply(&server_, &rspok));
+  HostResult result;
+  ares_gethostbyname(channel_, "www.google.com.", AF_INET, HostCallback, &result);
+  Process();
+  EXPECT_TRUE(result.done_);
+  std::stringstream ss;
+  ss << result.host_;
+  EXPECT_EQ("{'www.google.com' aliases=[] addrs=[1.2.3.4]}", ss.str());
+}
+
 TEST_P(MockChannelTest, SearchDomains) {
   DNSPacket nofirst;
   nofirst.set_response().set_aa().set_rcode(NXDOMAIN)