From da495400ca6e99d7ace28cff7b10de39fb845b14 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 29 Aug 2024 10:44:23 -0700 Subject: [PATCH 01/10] Update c-ares submodule to v1.34.2 --- auxil/c-ares | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auxil/c-ares b/auxil/c-ares index 0ad09d251b..a57ff692ee 160000 --- a/auxil/c-ares +++ b/auxil/c-ares @@ -1 +1 @@ -Subproject commit 0ad09d251bf01cc2b7860950527e33e22cd64256 +Subproject commit a57ff692eeab8d21c853dc1ddaf0164f517074c3 From b52a8ed9e2e86e1225a10807714690f34f9a0ec1 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 30 Aug 2024 12:44:31 -0700 Subject: [PATCH 02/10] Update vcpkg submodule to pick up c-ares v1.34.2 --- auxil/vcpkg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auxil/vcpkg b/auxil/vcpkg index 66b4b34d99..fb5e0ed3b3 160000 --- a/auxil/vcpkg +++ b/auxil/vcpkg @@ -1 +1 @@ -Subproject commit 66b4b34d99ab272fcf21f2bd12b616e371c6bb31 +Subproject commit fb5e0ed3b3632abbd889ccd8579b76cf980d88c1 From 65a59419b0d7ab50d29534dc3ea61c967141f0f1 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 29 Aug 2024 11:06:50 -0700 Subject: [PATCH 03/10] DNS_Mgr: Use ares_dns_record methods for queries --- src/DNS_Mgr.cc | 147 +++++++++++++++++++++++++++++-------------------- 1 file changed, 88 insertions(+), 59 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index ba019a589e..792355f28d 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -127,11 +127,12 @@ struct ares_deleter { void operator()(ares_addrinfo* s) const { ares_freeaddrinfo(s); } void operator()(struct hostent* h) const { ares_free_hostent(h); } void operator()(struct ares_txt_reply* h) const { ares_free_data(h); } + void operator()(ares_dns_record_t* r) const { ares_dns_record_destroy(r); } }; namespace zeek::detail { static void addrinfo_cb(void* arg, int status, int timeouts, struct ares_addrinfo* result); -static void query_cb(void* arg, int status, int timeouts, unsigned char* buf, int len); +static void query_cb(void* arg, ares_status_t status, size_t timeouts, const ares_dns_record* dnsrec); static void sock_cb(void* data, int s, int read, int write); struct CallbackArgs { @@ -158,7 +159,7 @@ private: IPAddr addr; int request_type = 0; // Query type bool async = false; - std::unique_ptr query; + std::unique_ptr query_rec; static uint16_t request_id; }; @@ -200,17 +201,23 @@ void DNS_Request::MakeRequest(ares_channel channel, DNS_Mgr* mgr) { else query_host = host; - std::unique_ptr query_str; + std::unique_ptr dnsrec; int len = 0; - int status = ares_create_query(query_host.c_str(), C_IN, request_type, DNS_Request::request_id, 1, - out_ptr(query_str), &len, MAX_UDP_BUFFER_SIZE); + ares_status_t status = ares_dns_record_create(out_ptr(dnsrec), DNS_Request::request_id, + ARES_FLAG_RD, ARES_OPCODE_QUERY, ARES_RCODE_NOERROR); - if ( status != ARES_SUCCESS || query_str == nullptr ) + if ( status != ARES_SUCCESS || dnsrec == nullptr ) + return; + + status = ares_dns_record_query_add(dnsrec.get(), query_host.c_str(), + static_cast(request_type), ARES_CLASS_IN); + + if ( status != ARES_SUCCESS ) return; // Store this so it can be destroyed when the request is destroyed. - this->query = std::move(query_str); - ares_send(channel, this->query.get(), len, query_cb, req_data.release()); + this->query_rec = std::move(dnsrec); + ares_send_dnsrec(channel, query_rec.get(), query_cb, req_data.release(), NULL); } } @@ -347,7 +354,7 @@ static void addrinfo_cb(void* arg, int status, int timeouts, struct ares_addrinf delete arg_data; } -static void query_cb(void* arg, int status, int timeouts, unsigned char* buf, int len) { +static void query_cb(void* arg, ares_status_t status, size_t timeouts, const ares_dns_record_t* dnsrec) { auto arg_data = reinterpret_cast(arg); const auto [req, mgr] = *arg_data; @@ -365,65 +372,87 @@ static void query_cb(void* arg, int status, int timeouts, unsigned char* buf, in } } else { - // We don't really care that we couldn't properly parse the TTL here, since the - // later parsing will fail with better error messages. In that case, it's ok - // that we throw away the status value. - int ttl; - get_ttl(buf, len, &ttl); + struct hostent he {}; - switch ( req->RequestType() ) { - case T_PTR: { - std::unique_ptr he; - if ( req->Addr().GetFamily() == IPv4 ) { - struct in_addr addr; - req->Addr().CopyIPv4(&addr); - status = ares_parse_ptr_reply(buf, len, &addr, sizeof(addr), AF_INET, out_ptr(he)); - } - else { - struct in6_addr addr; - req->Addr().CopyIPv6(&addr); - status = - ares_parse_ptr_reply(buf, len, &addr, sizeof(addr), AF_INET6, out_ptr(he)); + uint32_t ttl = 0; + size_t rr_cnt = ares_dns_record_rr_cnt(dnsrec, ARES_SECTION_ANSWER); + bool error = false; + + for ( size_t idx = 0; idx < rr_cnt; idx++ ) { + const ares_dns_rr_t* rr = ares_dns_record_rr_get_const(dnsrec, ARES_SECTION_ANSWER, 0); + + // Use the ttl from the first record, since we don't keep track of the TTLs for each + // individually. + if ( idx == 0 ) + ttl = ares_dns_rr_get_ttl(rr); + + ares_dns_rec_type_t type = ares_dns_rr_get_type(rr); + if ( req->RequestType() != type ) + continue; + + if ( type == ARES_REC_TYPE_PTR ) { + const char* txt = ares_dns_rr_get_str(rr, ARES_RR_PTR_DNAME); + if ( txt == NULL ) { + // According to the c-ares docs, this can happen but only in cases of "misuse". We + // still need to check for it though. + error = true; + break; } - if ( status == ARES_SUCCESS ) - mgr->AddResult(req, he.get(), ttl); - else { - // See above for why DNS_TIMEOUT here. - mgr->AddResult(req, nullptr, DNS_TIMEOUT); + if ( idx == 0 ) { + he.h_name = util::copy_string(txt); + he.h_aliases = new char*[rr_cnt]; + he.h_aliases[0] = NULL; } + else { + he.h_aliases[idx - 1] = util::copy_string(txt); + } + } + else if ( type == ARES_REC_TYPE_TXT ) { + size_t abin_cnt = ares_dns_rr_get_abin_cnt(rr, ARES_RR_TXT_DATA); + if ( abin_cnt == 0 ) + break; + + // TODO: We only process the first abin in the response. There might be more. + size_t abin_len; + const unsigned char* abin = ares_dns_rr_get_abin(rr, ARES_RR_TXT_DATA, 0, &abin_len); + if ( abin == NULL ) { + // According to the c-ares docs, this can happen but only in cases of "misuse". We + // still need to check for it though. + error = true; + break; + } + + he.h_name = new char[abin_len + 1]; + strncpy(he.h_name, reinterpret_cast(abin), abin_len); + he.h_name[abin_len] = 0; + he.h_aliases = new char*[1]; + he.h_aliases[0] = NULL; + + // TODO: We only process the first RR for a TXT query, even if there are more of them. break; } - case T_TXT: { - std::unique_ptr reply; - int r = ares_parse_txt_reply(buf, len, out_ptr(reply)); - if ( r == ARES_SUCCESS ) { - // Use a hostent to send the data into AddResult(). We only care about - // setting the host field, but everything else should be zero just for - // safety. - - // We don't currently handle more than the first response, and throw the - // rest away. There really isn't a good reason for this, we just haven't - // ever done so. It would likely require some changes to the output from - // Lookup(), since right now it only returns one value. - struct hostent he {}; - he.h_name = util::copy_string(reinterpret_cast(reply->txt)); - mgr->AddResult(req, &he, ttl); - - delete[] he.h_name; - } - else { - // See above for why DNS_TIMEOUT here. - mgr->AddResult(req, nullptr, DNS_TIMEOUT); - } - + else { + error = true; + reporter->Error("Requests of type %d (%s) are unsupported", type, ares_dns_rec_type_tostr(type)); break; } + } - default: - reporter->Error("Requests of type %d (%s) are unsupported", req->RequestType(), - request_type_string(req->RequestType())); - break; + if ( rr_cnt != 0 && ! error ) + mgr->AddResult(req, &he, ttl); + else + // See above for why DNS_TIMEOUT here. + mgr->AddResult(req, nullptr, DNS_TIMEOUT); + + delete[] he.h_name; + + if ( he.h_aliases ) { + for ( size_t idx = 0; he.h_aliases[idx] != NULL; idx++ ) { + delete[] he.h_aliases; + } + + delete[] he.h_aliases; } } From 16474ed77f20904acd4960ce4a6cd7690d01a1e8 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 29 Aug 2024 13:17:12 -0700 Subject: [PATCH 04/10] DNS_Mgr: Switch to ares_set_servers_csv --- src/DNS_Mgr.cc | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 792355f28d..4e17e5c26f 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -550,25 +550,7 @@ void DNS_Mgr::InitSource() { // the lookup. auto dns_resolver = getenv("ZEEK_DNS_RESOLVER"); if ( dns_resolver ) { - ares_addr_node servers; - servers.next = NULL; - - auto dns_resolver_addr = IPAddr(dns_resolver); - - if ( dns_resolver_addr.GetFamily() == IPv4 ) { - servers.family = AF_INET; - dns_resolver_addr.CopyIPv4(&(servers.addr.addr4)); - } - else { - struct sockaddr_in6 sa = {0}; - sa.sin6_family = AF_INET6; - dns_resolver_addr.CopyIPv6(&sa.sin6_addr); - - servers.family = AF_INET6; - memcpy(&(servers.addr.addr6), &sa.sin6_addr, sizeof(ares_in6_addr)); - } - - ares_set_servers(channel, &servers); + ares_set_servers_csv(channel, dns_resolver); } did_init = true; From 6739fca645cba0f93f7082310f39e380461774c7 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 30 Aug 2024 10:24:45 -0700 Subject: [PATCH 05/10] DNS_Mgr: Remove usage of ares_getsock from GetNextTimeout --- src/DNS_Mgr.cc | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 4e17e5c26f..2cd157e443 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -1315,23 +1315,19 @@ double DNS_Mgr::GetNextTimeout() { if ( asyncs_pending == 0 ) return -1; - int nfds = 0; - ares_socket_t socks[ARES_GETSOCK_MAXNUM]; - int bitmap = ares_getsock(channel, socks, ARES_GETSOCK_MAXNUM); - for ( int i = 0; i < ARES_GETSOCK_MAXNUM; i++ ) { - if ( ARES_GETSOCK_READABLE(bitmap, i) || ARES_GETSOCK_WRITABLE(bitmap, i) ) - ++nfds; - } + struct timeval tv; + struct timeval* tvp = ares_timeout(channel, NULL, &tv); - // Do we have any sockets that are read or writable? - if ( nfds == 0 ) + // If you pass NULL as the max time argument to ares_timeout, it will return null if there + // isn't anything waiting to be processed. + if ( ! tvp ) return -1; - struct timeval tv; - tv.tv_sec = DNS_TIMEOUT; - tv.tv_usec = 0; - - struct timeval* tvp = ares_timeout(channel, &tv, &tv); + // Clamp the timeout to our desired max, since we passed NULl to ares_timeout. + if ( tvp->tv_sec > DNS_TIMEOUT ) { + tvp->tv_sec = DNS_TIMEOUT; + tvp->tv_usec = 0; + } return static_cast(tvp->tv_sec) + (static_cast(tvp->tv_usec) / 1e6); } From d95057d61832578dfcc66b4b4ec67bac0a25c8ae Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 30 Aug 2024 10:58:51 -0700 Subject: [PATCH 06/10] DNS_Mgr: Remove usage of ares_getsock from Lookup --- src/DNS_Mgr.cc | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 2cd157e443..1228515459 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -887,30 +887,34 @@ void DNS_Mgr::Lookup(const std::string& name, int request_type, LookupCallback* void DNS_Mgr::Resolve() { int nfds = 0; struct timeval *tvp, tv; - struct pollfd pollfds[ARES_GETSOCK_MAXNUM]; - ares_socket_t socks[ARES_GETSOCK_MAXNUM]; + struct pollfd pollfds[1024]; tv.tv_sec = DNS_TIMEOUT; tv.tv_usec = 0; for ( int i = 0; i < MAX_PENDING_REQUESTS; i++ ) { - int nfds = 0; - int bitmap = ares_getsock(channel, socks, ARES_GETSOCK_MAXNUM); + if ( socket_fds.empty() && write_socket_fds.empty() ) + break; - for ( int i = 0; i < ARES_GETSOCK_MAXNUM; i++ ) { - bool rd = ARES_GETSOCK_READABLE(bitmap, i); - bool wr = ARES_GETSOCK_WRITABLE(bitmap, i); - if ( rd || wr ) { - pollfds[nfds].fd = socks[i]; - pollfds[nfds].events = rd ? POLLIN : 0; - pollfds[nfds].events |= wr ? POLLOUT : 0; - ++nfds; - } + memset(pollfds, 0, sizeof(pollfd) * 1024); + + for ( int fd : socket_fds ) { + if ( nfds == 1024 ) + break; + + pollfds[nfds].fd = fd; + pollfds[nfds].events = POLLIN; + ++nfds; } - // Do we have any sockets that are read or writable? - if ( nfds == 0 ) - break; + for ( int fd : write_socket_fds ) { + if ( nfds == 1024 ) + break; + + pollfds[nfds].fd = fd; + pollfds[nfds].events = POLLIN | POLLOUT; + ++nfds; + } // poll() timeout is in milliseconds. tvp = ares_timeout(channel, &tv, &tv); From f3fbe45c4c32b450f5b7948ad6a654c21f573b49 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 8 Nov 2024 11:17:48 +0100 Subject: [PATCH 07/10] btest: Add integration test for DNS_Mgr This makes use of an ephemeral dnsmasq instance --- .../btest/Baseline/dns_mgr.lookup_addr/out | 2 + .../Baseline/dns_mgr.lookup_hostname/out | 7 ++++ .../dns_mgr.lookup_hostname_cname/out | 3 ++ .../Baseline/dns_mgr.lookup_hostname_txt/out | 2 + testing/btest/btest.cfg | 2 +- testing/btest/dns_mgr/lookup_addr.zeek | 26 ++++++++++++ testing/btest/dns_mgr/lookup_hostname.zeek | 28 +++++++++++++ .../btest/dns_mgr/lookup_hostname_cname.zeek | 42 +++++++++++++++++++ .../btest/dns_mgr/lookup_hostname_txt.zeek | 32 ++++++++++++++ testing/scripts/run-dnsmasq | 34 +++++++++++++++ 10 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/dns_mgr.lookup_addr/out create mode 100644 testing/btest/Baseline/dns_mgr.lookup_hostname/out create mode 100644 testing/btest/Baseline/dns_mgr.lookup_hostname_cname/out create mode 100644 testing/btest/Baseline/dns_mgr.lookup_hostname_txt/out create mode 100644 testing/btest/dns_mgr/lookup_addr.zeek create mode 100644 testing/btest/dns_mgr/lookup_hostname.zeek create mode 100644 testing/btest/dns_mgr/lookup_hostname_cname.zeek create mode 100644 testing/btest/dns_mgr/lookup_hostname_txt.zeek create mode 100755 testing/scripts/run-dnsmasq diff --git a/testing/btest/Baseline/dns_mgr.lookup_addr/out b/testing/btest/Baseline/dns_mgr.lookup_addr/out new file mode 100644 index 0000000000..4b156fd76d --- /dev/null +++ b/testing/btest/Baseline/dns_mgr.lookup_addr/out @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +dns.example.com diff --git a/testing/btest/Baseline/dns_mgr.lookup_hostname/out b/testing/btest/Baseline/dns_mgr.lookup_hostname/out new file mode 100644 index 0000000000..dbcd56e47a --- /dev/null +++ b/testing/btest/Baseline/dns_mgr.lookup_hostname/out @@ -0,0 +1,7 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +addrs, 5 +10.0.0.3 +10.0.0.2 +10.0.0.1 +fe80::6990:df6e:618:c096 +10.0.0.4 diff --git a/testing/btest/Baseline/dns_mgr.lookup_hostname_cname/out b/testing/btest/Baseline/dns_mgr.lookup_hostname_cname/out new file mode 100644 index 0000000000..5711a95433 --- /dev/null +++ b/testing/btest/Baseline/dns_mgr.lookup_hostname_cname/out @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +lookup_hostname addrs, 0 +lookup_hostname_txt, 15, www.example.com diff --git a/testing/btest/Baseline/dns_mgr.lookup_hostname_txt/out b/testing/btest/Baseline/dns_mgr.lookup_hostname_txt/out new file mode 100644 index 0000000000..52f266be5d --- /dev/null +++ b/testing/btest/Baseline/dns_mgr.lookup_hostname_txt/out @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +TXT, more-network-monitor diff --git a/testing/btest/btest.cfg b/testing/btest/btest.cfg index b47ff2dddf..9b459a0a05 100644 --- a/testing/btest/btest.cfg +++ b/testing/btest/btest.cfg @@ -4,7 +4,7 @@ build_dir = build [btest] -TestDirs = af_packet doc bifs language core scripts coverage signatures plugins broker spicy supervisor telemetry javascript misc opt +TestDirs = af_packet doc bifs language core scripts coverage signatures plugins broker spicy supervisor telemetry javascript misc opt dns_mgr TmpDir = %(testbase)s/.tmp BaselineDir = %(testbase)s/Baseline IgnoreDirs = .svn CVS .tmp diff --git a/testing/btest/dns_mgr/lookup_addr.zeek b/testing/btest/dns_mgr/lookup_addr.zeek new file mode 100644 index 0000000000..1c35cb2a9c --- /dev/null +++ b/testing/btest/dns_mgr/lookup_addr.zeek @@ -0,0 +1,26 @@ +# @TEST-GROUP: dns_mgr +# +# @TEST-REQUIRES: dnsmasq --version +# @TEST-PORT: DNSMASQ_PORT + +# @TEST-EXEC: btest-bg-run dnsmasq run-dnsmasq 127.0.0.1 ${DNSMASQ_PORT%/tcp} +# @TEST-EXEC: unset ZEEK_DNS_FAKE; ZEEK_DNS_RESOLVER=127.0.0.1:${DNSMASQ_PORT%/tcp} zeek -b %INPUT >out +# @TEST-EXEC: btest-bg-wait -k 0 + +# @TEST-EXEC: btest-diff out + +redef exit_only_after_terminate = T; + +event zeek_init() + { + when ( local host = lookup_addr(10.0.0.99) ) + { + print cat(host); + terminate(); + } + timeout 5sec + { + print "ERROR timeout"; + terminate(); + } + } diff --git a/testing/btest/dns_mgr/lookup_hostname.zeek b/testing/btest/dns_mgr/lookup_hostname.zeek new file mode 100644 index 0000000000..fcb328369e --- /dev/null +++ b/testing/btest/dns_mgr/lookup_hostname.zeek @@ -0,0 +1,28 @@ +# @TEST-GROUP: dns_mgr +# +# @TEST-REQUIRES: dnsmasq --version +# @TEST-PORT: DNSMASQ_PORT + +# @TEST-EXEC: btest-bg-run dnsmasq run-dnsmasq 127.0.0.1 ${DNSMASQ_PORT%/tcp} +# @TEST-EXEC: unset ZEEK_DNS_FAKE; ZEEK_DNS_RESOLVER=127.0.0.1:${DNSMASQ_PORT%/tcp} zeek -b %INPUT >out +# @TEST-EXEC: btest-bg-wait -k 0 + +# @TEST-EXEC: btest-diff out + +redef exit_only_after_terminate = T; + +event zeek_init() + { + when ( local addrs = lookup_hostname("example.com") ) + { + print "addrs", |addrs|; + for ( a in addrs ) + print a; + terminate(); + } + timeout 5sec + { + print "ERROR timeout"; + terminate(); + } + } diff --git a/testing/btest/dns_mgr/lookup_hostname_cname.zeek b/testing/btest/dns_mgr/lookup_hostname_cname.zeek new file mode 100644 index 0000000000..0c66df4043 --- /dev/null +++ b/testing/btest/dns_mgr/lookup_hostname_cname.zeek @@ -0,0 +1,42 @@ +# @TEST-GROUP: dns_mgr +# +# @TEST-REQUIRES: dnsmasq --version +# @TEST-PORT: DNSMASQ_PORT + +# @TEST-EXEC: btest-bg-run dnsmasq run-dnsmasq 127.0.0.1 ${DNSMASQ_PORT%/tcp} +# @TEST-EXEC: unset ZEEK_DNS_FAKE; ZEEK_DNS_RESOLVER=127.0.0.1:${DNSMASQ_PORT%/tcp} zeek -b %INPUT >out +# @TEST-EXEC: btest-bg-wait -k 0 + +# @TEST-EXEC: btest-diff out + +redef exit_only_after_terminate = T; + +event zeek_init() + { + # www.example.com is a CNAME for example.com and this + # results in nothing :-/ + when ( local addrs = lookup_hostname("www.example.com") ) + { + print "lookup_hostname addrs", |addrs|; + for ( a in addrs ) + print a; + + # Example.com is a CNAME for www.example.com and a + # TXT lookup yields example.com. Weird. + when ( local txt = lookup_hostname_txt("www.example.com") ) + { + print "lookup_hostname_txt", |txt|, txt; + terminate(); + } + timeout 5sec + { + print "ERROR lookup_hostname_txt timeout"; + terminate(); + } + } + timeout 5sec + { + print "ERROR lookup_hostname timeout"; + terminate(); + } + } diff --git a/testing/btest/dns_mgr/lookup_hostname_txt.zeek b/testing/btest/dns_mgr/lookup_hostname_txt.zeek new file mode 100644 index 0000000000..29b70f6c69 --- /dev/null +++ b/testing/btest/dns_mgr/lookup_hostname_txt.zeek @@ -0,0 +1,32 @@ +# @TEST-GROUP: dns_mgr +# +# @TEST-REQUIRES: dnsmasq --version +# @TEST-PORT: DNSMASQ_PORT + +# @TEST-EXEC: btest-bg-run dnsmasq run-dnsmasq 127.0.0.1 ${DNSMASQ_PORT%/tcp} +# @TEST-EXEC: unset ZEEK_DNS_FAKE; ZEEK_DNS_RESOLVER=127.0.0.1:${DNSMASQ_PORT%/tcp} zeek -b %INPUT >out +# @TEST-EXEC: btest-bg-wait -k 0 + +# @TEST-EXEC: btest-diff out + +redef exit_only_after_terminate = T; + +event zeek_init() + { + when ( local txt = lookup_hostname_txt("example.com") ) + { + # www.example.com has much more TXT entries, we + # only return "more-network-monitor", however. + # + # ;; ANSWER SECTION: + # www.example.com. 0 IN TXT "more-network-monitor" "bro" + # www.example.com. 0 IN TXT "network-monitor" "open-source" "zeek" + print "TXT", txt; + terminate(); + } + timeout 5sec + { + print "ERROR timeout"; + terminate(); + } + } diff --git a/testing/scripts/run-dnsmasq b/testing/scripts/run-dnsmasq new file mode 100755 index 0000000000..edada1b6e5 --- /dev/null +++ b/testing/scripts/run-dnsmasq @@ -0,0 +1,34 @@ +#!/usr/bin/env bash +set -eux + +if ! dnsmasq --version; then + exit 1 +fi + +if [ $# -ne 2 ]; then + echo "Usage $0 " >2 + exit 1 +fi + +listen_addr=$1 +listen_port=$2 + +exec dnsmasq \ + --no-resolv \ + --no-hosts \ + --no-daemon \ + --listen-addr="${listen_addr}" \ + --port="${listen_port}" \ + --address /example.com/10.0.0.1 \ + --address /example.com/10.0.0.2 \ + --address /example.com/10.0.0.3 \ + --address /example.com/10.0.0.4 \ + --address /example.com/10.0.0.4 \ + --address /example.com/fe80::6990:df6e:618:c096 \ + --address /mx.example.com/10.0.0.99 \ + --address /dns.example.com/10.0.0.99 \ + --ptr-record=99.0.0.10.in-addr.arpa,mx.example.com \ + --ptr-record=99.0.0.10.in-addr.arpa,dns.example.com \ + --txt-record=example.com,network-monitor,open-source,zeek \ + --txt-record=example.com,more-network-monitor,bro \ + --cname=www.example.com,example.com From 5859a7e28c65fd71cadc7d30818b846fb2b72c66 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 8 Nov 2024 11:18:34 +0100 Subject: [PATCH 08/10] DNS_Mgr: Fix aliases memory issues --- src/DNS_Mgr.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 1228515459..a14345ed2c 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -406,6 +406,7 @@ static void query_cb(void* arg, ares_status_t status, size_t timeouts, const are } else { he.h_aliases[idx - 1] = util::copy_string(txt); + he.h_aliases[idx] = nullptr; } } else if ( type == ARES_REC_TYPE_TXT ) { @@ -449,7 +450,7 @@ static void query_cb(void* arg, ares_status_t status, size_t timeouts, const are if ( he.h_aliases ) { for ( size_t idx = 0; he.h_aliases[idx] != NULL; idx++ ) { - delete[] he.h_aliases; + delete[] he.h_aliases[idx]; } delete[] he.h_aliases; From 3f4de778ae11661bcce92ad845401a144d492da4 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 8 Nov 2024 11:23:06 +0100 Subject: [PATCH 09/10] ci: Add dnsmasq to a few platforms for testing --- ci/alpine/Dockerfile | 1 + ci/debian-12/Dockerfile | 1 + ci/fedora-40/Dockerfile | 1 + ci/freebsd/prepare.sh | 5 ++++- ci/macos/prepare.sh | 2 +- ci/opensuse-leap-15.6/Dockerfile | 1 + ci/opensuse-tumbleweed/Dockerfile | 1 + ci/ubuntu-24.04/Dockerfile | 1 + 8 files changed, 11 insertions(+), 2 deletions(-) diff --git a/ci/alpine/Dockerfile b/ci/alpine/Dockerfile index 30676c3c4d..ce82ab708f 100644 --- a/ci/alpine/Dockerfile +++ b/ci/alpine/Dockerfile @@ -12,6 +12,7 @@ RUN apk add --no-cache \ cmake \ curl \ diffutils \ + dnsmasq \ flex-dev \ musl-fts-dev \ g++ \ diff --git a/ci/debian-12/Dockerfile b/ci/debian-12/Dockerfile index f989f4e28a..c3ae4339b7 100644 --- a/ci/debian-12/Dockerfile +++ b/ci/debian-12/Dockerfile @@ -12,6 +12,7 @@ RUN apt-get update && apt-get -y install \ ccache \ cmake \ curl \ + dnsmasq \ flex \ g++ \ gcc \ diff --git a/ci/fedora-40/Dockerfile b/ci/fedora-40/Dockerfile index f292e11ece..45f209dbec 100644 --- a/ci/fedora-40/Dockerfile +++ b/ci/fedora-40/Dockerfile @@ -9,6 +9,7 @@ RUN dnf -y install \ ccache \ cmake \ diffutils \ + dnsmasq \ flex \ gcc \ gcc-c++ \ diff --git a/ci/freebsd/prepare.sh b/ci/freebsd/prepare.sh index 629ed3c513..5051084ac3 100755 --- a/ci/freebsd/prepare.sh +++ b/ci/freebsd/prepare.sh @@ -6,7 +6,7 @@ set -e set -x env ASSUME_ALWAYS_YES=YES pkg bootstrap -pkg install -y bash git cmake swig bison python3 base64 flex ccache jq +pkg install -y bash git cmake swig bison python3 base64 flex ccache jq dnsmasq pkg upgrade -y curl pyver=$(python3 -c 'import sys; print(f"py{sys.version_info[0]}{sys.version_info[1]}")') pkg install -y $pyver-sqlite3 @@ -17,3 +17,6 @@ python -m pip install websockets junit2html # Spicy detects whether it is run from build directory via `/proc`. echo "proc /proc procfs rw,noauto 0 0" >>/etc/fstab mount /proc + +# dnsmasq is in /usr/local/sbin and that's not in the PATH by default +ln -s /usr/local/sbin/dnsmasq /usr/local/bin/dnsmasq diff --git a/ci/macos/prepare.sh b/ci/macos/prepare.sh index a96833f75f..7cc3e69cac 100755 --- a/ci/macos/prepare.sh +++ b/ci/macos/prepare.sh @@ -7,7 +7,7 @@ set -x brew update brew upgrade cmake -brew install openssl@3 swig bison flex ccache libmaxminddb +brew install openssl@3 swig bison flex ccache libmaxminddb dnsmasq if [ $(sw_vers -productVersion | cut -d '.' -f 1) -lt 14 ]; then python3 -m pip install --upgrade pip diff --git a/ci/opensuse-leap-15.6/Dockerfile b/ci/opensuse-leap-15.6/Dockerfile index 359c282f01..a40405e855 100644 --- a/ci/opensuse-leap-15.6/Dockerfile +++ b/ci/opensuse-leap-15.6/Dockerfile @@ -11,6 +11,7 @@ RUN zypper addrepo https://download.opensuse.org/repositories/openSUSE:Leap:15.6 ccache \ cmake \ curl \ + dnsmasq \ flex \ gcc12 \ gcc12-c++ \ diff --git a/ci/opensuse-tumbleweed/Dockerfile b/ci/opensuse-tumbleweed/Dockerfile index 911f7af96b..c35a80205a 100644 --- a/ci/opensuse-tumbleweed/Dockerfile +++ b/ci/opensuse-tumbleweed/Dockerfile @@ -16,6 +16,7 @@ RUN zypper refresh \ cmake \ curl \ diffutils \ + dnsmasq \ findutils \ flex \ gcc \ diff --git a/ci/ubuntu-24.04/Dockerfile b/ci/ubuntu-24.04/Dockerfile index 13dbac92cf..89d999d19c 100644 --- a/ci/ubuntu-24.04/Dockerfile +++ b/ci/ubuntu-24.04/Dockerfile @@ -15,6 +15,7 @@ RUN apt-get update && apt-get -y install \ clang++-18 \ cmake \ curl \ + dnsmasq \ flex \ g++ \ gcc \ From e3763df0654acca2cc1b914c313962f717e6500e Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 8 Nov 2024 12:45:51 -0700 Subject: [PATCH 10/10] DNS_Mgr: Remove processing of dns aliases in general --- src/DNS_Mgr.cc | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index a14345ed2c..6c0409d64b 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -123,10 +123,7 @@ static const char* request_type_string(int request_type) { struct ares_deleter { void operator()(char* s) const { ares_free_string(s); } - void operator()(unsigned char* s) const { ares_free_string(s); } void operator()(ares_addrinfo* s) const { ares_freeaddrinfo(s); } - void operator()(struct hostent* h) const { ares_free_hostent(h); } - void operator()(struct ares_txt_reply* h) const { ares_free_data(h); } void operator()(ares_dns_record_t* r) const { ares_dns_record_destroy(r); } }; @@ -399,15 +396,12 @@ static void query_cb(void* arg, ares_status_t status, size_t timeouts, const are break; } - if ( idx == 0 ) { - he.h_name = util::copy_string(txt); - he.h_aliases = new char*[rr_cnt]; - he.h_aliases[0] = NULL; - } - else { - he.h_aliases[idx - 1] = util::copy_string(txt); - he.h_aliases[idx] = nullptr; - } + // TODO: it's possible that a response has multiple aliases. We + // don't handle those so we can just break here after setting + // h_aliases to null. + he.h_name = util::copy_string(txt); + he.h_aliases = nullptr; + break; } else if ( type == ARES_REC_TYPE_TXT ) { size_t abin_cnt = ares_dns_rr_get_abin_cnt(rr, ARES_RR_TXT_DATA); @@ -427,8 +421,7 @@ static void query_cb(void* arg, ares_status_t status, size_t timeouts, const are he.h_name = new char[abin_len + 1]; strncpy(he.h_name, reinterpret_cast(abin), abin_len); he.h_name[abin_len] = 0; - he.h_aliases = new char*[1]; - he.h_aliases[0] = NULL; + he.h_aliases = nullptr; // TODO: We only process the first RR for a TXT query, even if there are more of them. break; @@ -447,14 +440,6 @@ static void query_cb(void* arg, ares_status_t status, size_t timeouts, const are mgr->AddResult(req, nullptr, DNS_TIMEOUT); delete[] he.h_name; - - if ( he.h_aliases ) { - for ( size_t idx = 0; he.h_aliases[idx] != NULL; idx++ ) { - delete[] he.h_aliases[idx]; - } - - delete[] he.h_aliases; - } } req->ProcessAsyncResult(timeouts > 0, mgr); @@ -972,7 +957,7 @@ RecordValPtr DNS_Mgr::BuildMappingVal(const DNS_MappingPtr& dm) { } void DNS_Mgr::AddResult(DNS_Request* dr, struct hostent* h, uint32_t ttl, bool merge) { - // TODO: the existing code doesn't handle hostname aliases at all. Should we? + // TODO: Should we handle hostname aliases here somehow? DNS_MappingPtr new_mapping = nullptr; DNS_MappingPtr prev_mapping = nullptr;