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.

diff --git a/lib/dns/geoip2.c b/lib/dns/geoip2.c
index 098a8844d5022adce212a3f46202cb10a1c32180..e65d6fb5707ab09964f0a5bc4a4778c0dbcd553b 100644 (file)
--- 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 21a887bae2ad85296a81ae6d5940d7995088c9f2..bc581a01a27088e0a8b8c985600687eefaeb2ade 100644 (file)
--- a/lib/dns/tests/geoip_test.c
+++ b/lib/dns/tests/geoip_test.c
@@ -36,6 +36,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"
 
@@ -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),