https://github.com/c-ares/c-ares/issues/683 https://github.com/c-ares/c-ares/commit/626dcb155b4daf3360e4251c64ce052e7e520b34 From 626dcb155b4daf3360e4251c64ce052e7e520b34 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 12 Jan 2024 09:55:42 -0500 Subject: [PATCH] Do not sanity check RR Name vs Question (#685) It appears as though we should never sanity check the RR name vs the question name as some DNS servers may return results for alias records. Fixes Bug: #683 Fix By: Brad House (@bradh352) --- a/src/lib/ares__parse_into_addrinfo.c +++ b/src/lib/ares__parse_into_addrinfo.c @@ -81,7 +81,6 @@ ares_status_t ares__parse_into_addrinfo(const unsigned char *abuf, size_t alen, } for (i = 0; i < ancount; i++) { - const char *rname = NULL; ares_dns_rec_type_t rtype; const ares_dns_rr_t *rr = ares_dns_record_rr_get(dnsrec, ARES_SECTION_ANSWER, i); @@ -91,13 +90,18 @@ ares_status_t ares__parse_into_addrinfo(const unsigned char *abuf, size_t alen, } rtype = ares_dns_rr_get_type(rr); - rname = ares_dns_rr_get_name(rr); - /* Old code did this hostname sanity check */ - if ((rtype == ARES_REC_TYPE_A || rtype == ARES_REC_TYPE_AAAA) && - strcasecmp(rname, hostname) != 0) { - continue; - } + /* Issue #683 + * Old code did this hostname sanity check, however it appears this is + * flawed logic. Other resolvers don't do this sanity check. Leaving + * this code commented out for future reference. + * + * rname = ares_dns_rr_get_name(rr); + * if ((rtype == ARES_REC_TYPE_A || rtype == ARES_REC_TYPE_AAAA) && + * strcasecmp(rname, hostname) != 0) { + * continue; + * } + */ if (rtype == ARES_REC_TYPE_CNAME) { struct ares_addrinfo_cname *cname; --- a/src/lib/ares_parse_ptr_reply.c +++ b/src/lib/ares_parse_ptr_reply.c @@ -113,7 +113,6 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen_int, /* Cycle through answers */ for (i = 0; i < ancount; i++) { - const char *rname = NULL; const ares_dns_rr_t *rr = ares_dns_record_rr_get(dnsrec, ARES_SECTION_ANSWER, i); @@ -141,17 +140,20 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen_int, continue; } - /* Old code compared the name in the rr to the ptrname, so we'll do that - * check here, but I'm not sure its necessary */ - rname = ares_dns_rr_get_name(rr); - if (rname == NULL) { - /* Shouldn't be possible */ - status = ARES_EBADRESP; - goto done; - } - if (strcasecmp(ptrname, rname) != 0) { - continue; - } + /* Issue #683 + * Old code compared the name in the rr to the ptrname, but I think this + * is wrong since it was proven wrong for A & AAAA records. Leaving + * this code commented out for future reference + * + * rname = ares_dns_rr_get_name(rr); + * if (rname == NULL) { + * status = ARES_EBADRESP; + * goto done; + * } + * if (strcasecmp(ptrname, rname) != 0) { + * continue; + * } + */ /* Save most recent PTR record as the hostname */ hostname = ares_dns_rr_get_str(rr, ARES_RR_PTR_DNAME); --- a/test/ares-test-parse-a.cc +++ b/test/ares-test-parse-a.cc @@ -312,13 +312,19 @@ TEST_F(LibraryTest, ParseAReplyErrors) { EXPECT_EQ(nullptr, host); pkt.add_question(new DNSQuestion("example.com", T_A)); - // Question != answer + // Question != answer, this is ok as of Issue #683 pkt.questions_.clear(); pkt.add_question(new DNSQuestion("Axample.com", T_A)); data = pkt.data(); - EXPECT_EQ(ARES_ENODATA, ares_parse_a_reply(data.data(), (int)data.size(), + EXPECT_EQ(ARES_SUCCESS, ares_parse_a_reply(data.data(), (int)data.size(), &host, info, &count)); - EXPECT_EQ(nullptr, host); + ASSERT_NE(nullptr, host); + std::stringstream ss; + ss << HostEnt(host); + EXPECT_EQ("{'Axample.com' aliases=[] addrs=[2.3.4.5]}", ss.str()); + ares_free_hostent(host); + host = nullptr; + pkt.questions_.clear(); pkt.add_question(new DNSQuestion("example.com", T_A)); --- a/test/ares-test-parse-aaaa.cc +++ b/test/ares-test-parse-aaaa.cc @@ -139,13 +139,19 @@ TEST_F(LibraryTest, ParseAaaaReplyErrors) { EXPECT_EQ(nullptr, host); pkt.add_question(new DNSQuestion("example.com", T_AAAA)); - // Question != answer + // Question != answer, this is ok as of Issue #683 pkt.questions_.clear(); pkt.add_question(new DNSQuestion("Axample.com", T_AAAA)); data = pkt.data(); - EXPECT_EQ(ARES_ENODATA, ares_parse_aaaa_reply(data.data(), (int)data.size(), + EXPECT_EQ(ARES_SUCCESS, ares_parse_aaaa_reply(data.data(), (int)data.size(), &host, info, &count)); - EXPECT_EQ(nullptr, host); + ASSERT_NE(nullptr, host); + std::stringstream ss; + ss << HostEnt(host); + EXPECT_EQ("{'Axample.com' aliases=[] addrs=[0101:0101:0202:0202:0303:0303:0404:0404]}", ss.str()); + ares_free_hostent(host); + + host = nullptr; pkt.questions_.clear(); pkt.add_question(new DNSQuestion("example.com", T_AAAA)); --- a/test/ares-test-parse-ptr.cc +++ b/test/ares-test-parse-ptr.cc @@ -163,13 +163,20 @@ TEST_F(LibraryTest, ParsePtrReplyErrors) { addrv4, sizeof(addrv4), AF_INET, &host)); pkt.add_question(new DNSQuestion("64.48.32.16.in-addr.arpa", T_PTR)); - // Question != answer + // Question != answer, ok after #683 + host = nullptr; pkt.questions_.clear(); pkt.add_question(new DNSQuestion("99.48.32.16.in-addr.arpa", T_PTR)); data = pkt.data(); - EXPECT_EQ(ARES_ENODATA, ares_parse_ptr_reply(data.data(), (int)data.size(), + EXPECT_EQ(ARES_SUCCESS, ares_parse_ptr_reply(data.data(), (int)data.size(), addrv4, sizeof(addrv4), AF_INET, &host)); - EXPECT_EQ(nullptr, host); + ASSERT_NE(nullptr, host); + std::stringstream ss; + ss << HostEnt(host); + EXPECT_EQ("{'other.com' aliases=[other.com] addrs=[16.32.48.64]}", ss.str()); + ares_free_hostent(host); + + host = nullptr; pkt.questions_.clear(); pkt.add_question(new DNSQuestion("64.48.32.16.in-addr.arpa", T_PTR));