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.

(cherry picked from commit ec8334fb743ecd8ae29f2751dab0fd86b7334327)

diff --git a/lib/dns/geoip2.c b/lib/dns/geoip2.c
index 7854e6db96a37453d72360a242e24ef427c86fa0..68a28c677bbb6fcee236ca3d33266cd275d4639d 100644 (file)
--- a/lib/dns/geoip2.c
+++ b/lib/dns/geoip2.c
@@ -194,7 +194,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 605ebae27effd313784872da90abaf753bb0f6d7..470d8b668b3333d1a7228d38db426d1bc3b0b9c4 100644 (file)
--- a/lib/dns/tests/geoip_test.c
+++ b/lib/dns/tests/geoip_test.c
@@ -38,6 +38,8 @@
 #if defined(HAVE_GEOIP2)
 #include <maxminddb.h>
 
+#include "../geoip2.c"
+
 /* Use GeoIP2 databases from the 'geoip2' system test */
 #define TEST_GEOIP_DATA "../../../bin/tests/system/geoip2/data"
 #elif defined(HAVE_GEOIP)
@@ -219,6 +221,79 @@ do_lookup_int(const char *addr, dns_geoip_subtype_t subtype, int id) {
 
 #endif /* HAVE_GEOIP */
 
+#ifdef HAVE_GEOIP2
+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"));
+}
+#endif /* HAVE_GEOIP2 */
+
 #if defined(HAVE_GEOIP) || defined(HAVE_GEOIP2)
 static bool
 do_lookup_string(const char *addr, dns_geoip_subtype_t subtype,
@@ -617,6 +692,9 @@ int
 main(void) {
 #if defined(HAVE_GEOIP) || defined(HAVE_GEOIP2)
        const struct CMUnitTest tests[] = {
+#ifdef HAVE_GEOIP2
+               cmocka_unit_test_setup_teardown(baseline, _setup, _teardown),
+#endif /* HAVE_GEOIP2 */
                cmocka_unit_test_setup_teardown(country, _setup, _teardown),
                cmocka_unit_test_setup_teardown(country_v6, _setup, _teardown),
                cmocka_unit_test_setup_teardown(city, _setup, _teardown),