From ec8334fb743ecd8ae29f2751dab0fd86b7334327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 13 Jan 2020 14:32:19 +0100 Subject: [PATCH 1/2] Properly detect MMDB lookup failures Only comparing the value of the integer passed as the last argument to MMDB_lookup_sockaddr() against MMDB_SUCCESS is not enough to ensure that an MMDB lookup was successful - the 'found_entry' field of the MMDB_lookup_result_s structure returned by that function also needs to be true or else the remaining contents of that structure should be ignored as the lookup failed. Extend the relevant logical condition in get_entry_for() to ensure the latter does not return incorrect MMDB entries for IP addresses which do not belong to any subnet defined in a given GeoIP2 database. --- lib/dns/geoip2.c | 2 +- lib/dns/tests/geoip_test.c | 74 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/lib/dns/geoip2.c b/lib/dns/geoip2.c index 098a8844d5..e65d6fb570 100644 --- a/lib/dns/geoip2.c +++ b/lib/dns/geoip2.c @@ -97,7 +97,7 @@ get_entry_for(MMDB_s * const db, const isc_netaddr_t *addr) { isc_sockaddr_fromnetaddr(&sa, addr, 0); match = MMDB_lookup_sockaddr(db, &sa.type.sa, &err); - if (err != MMDB_SUCCESS) { + if (err != MMDB_SUCCESS || !match.found_entry) { return (NULL); } diff --git a/lib/dns/tests/geoip_test.c b/lib/dns/tests/geoip_test.c index 21a887bae2..bc581a01a2 100644 --- a/lib/dns/tests/geoip_test.c +++ b/lib/dns/tests/geoip_test.c @@ -36,6 +36,8 @@ #if defined(HAVE_GEOIP2) #include +#include "../geoip2.c" + /* Use GeoIP2 databases from the 'geoip2' system test */ #define TEST_GEOIP_DATA "../../../bin/tests/system/geoip2/data" @@ -105,6 +107,77 @@ close_geoip(void) { MMDB_close(&geoip_domain); } +static bool +/* Check if an MMDB entry of a given subtype exists for the given IP */ +entry_exists(dns_geoip_subtype_t subtype, const char *addr) { + struct in6_addr in6; + struct in_addr in4; + isc_netaddr_t na; + MMDB_s *db; + + if (inet_pton(AF_INET6, addr, &in6) == 1) { + isc_netaddr_fromin6(&na, &in6); + } else if (inet_pton(AF_INET, addr, &in4) == 1) { + isc_netaddr_fromin(&na, &in4); + } else { + INSIST(0); + ISC_UNREACHABLE(); + } + + db = geoip2_database(&geoip, fix_subtype(&geoip, subtype)); + + return (db != NULL && get_entry_for(db, &na) != NULL); +} + +/* + * Baseline test - check if get_entry_for() works as expected, i.e. that its + * return values are consistent with the contents of the test MMDBs found in + * bin/tests/system/geoip2/data/ (10.53.0.1 and fd92:7065:b8e:ffff::1 should be + * present in all databases, 192.0.2.128 should only be present in the country + * database, ::1 should be absent from all databases). + */ +static void +baseline(void **state) { + dns_geoip_subtype_t subtype; + + UNUSED(state); + + subtype = dns_geoip_city_name; + + assert_true(entry_exists(subtype, "10.53.0.1")); + assert_false(entry_exists(subtype, "192.0.2.128")); + assert_true(entry_exists(subtype, "fd92:7065:b8e:ffff::1")); + assert_false(entry_exists(subtype, "::1")); + + subtype = dns_geoip_country_name; + + assert_true(entry_exists(subtype, "10.53.0.1")); + assert_true(entry_exists(subtype, "192.0.2.128")); + assert_true(entry_exists(subtype, "fd92:7065:b8e:ffff::1")); + assert_false(entry_exists(subtype, "::1")); + + subtype = dns_geoip_domain_name; + + assert_true(entry_exists(subtype, "10.53.0.1")); + assert_false(entry_exists(subtype, "192.0.2.128")); + assert_true(entry_exists(subtype, "fd92:7065:b8e:ffff::1")); + assert_false(entry_exists(subtype, "::1")); + + subtype = dns_geoip_isp_name; + + assert_true(entry_exists(subtype, "10.53.0.1")); + assert_false(entry_exists(subtype, "192.0.2.128")); + assert_true(entry_exists(subtype, "fd92:7065:b8e:ffff::1")); + assert_false(entry_exists(subtype, "::1")); + + subtype = dns_geoip_as_asnum; + + assert_true(entry_exists(subtype, "10.53.0.1")); + assert_false(entry_exists(subtype, "192.0.2.128")); + assert_true(entry_exists(subtype, "fd92:7065:b8e:ffff::1")); + assert_false(entry_exists(subtype, "::1")); +} + static bool do_lookup_string(const char *addr, dns_geoip_subtype_t subtype, const char *string) @@ -334,6 +407,7 @@ int main(void) { #if defined(HAVE_GEOIP2) const struct CMUnitTest tests[] = { + cmocka_unit_test(baseline), cmocka_unit_test(country), cmocka_unit_test(country_v6), cmocka_unit_test(city), From aa96ec25c8db58c6d00be4df1adea55165735206 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 13 Jan 2020 14:32:19 +0100 Subject: [PATCH 2/2] Add CHANGES entry 5339. [bug] With some libmaxminddb versions, named could erroneously match an IP address not belonging to any subnet defined in a given GeoIP2 database to one of the existing entries in that database. [GL #1552] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 1434df9a5b..23e465e6af 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +5339. [bug] With some libmaxminddb versions, named could erroneously + match an IP address not belonging to any subnet defined + in a given GeoIP2 database to one of the existing + entries in that database. [GL #1552] + 5338. [bug] Fix line spacing in `rndc secroots`. Thanks to Tony Finch. [GL !2478]