From 1f3e47a5f1cb0aba75343f50983b04bcd2e1dea4 Mon Sep 17 00:00:00 2001
From: Tom Sommer <tom.sommer@team.blue>
Date: Sat, 19 Sep 2026 16:22:41 +0200
Subject: [PATCH] Fix memory leak when SecGeoLookupDb loads a database more
 than once

GeoLookup is a process wide singleton and GeoLookup::setDataBase() used to
call MMDB_open() unconditionally, overwriting the MMDB_s member without
closing the database that was open before. Every additional evaluation of
SecGeoLookupDb therefore leaked the MMDB_s heap metadata plus the mmap()ed
copy of the whole database file.

This is not limited to a configuration that repeats the directive: under
nginx the master process re-parses all rules on every reload and on every
configuration test, and each server/location context that includes the
directive parses it again, so the leak accumulates for the lifetime of the
master process and is inherited by every worker.

cleanUp() already knows how to release either back end and resets m_version,
so calling it at the top of setDataBase() is enough. The GeoIP back end was
not leaking (GeoIP_open is guarded by m_version == NOT_LOADED), but it kept
the first database forever and ignored any later SecGeoLookupDb; it is now
replaced like the MaxMind one.

Behaviour change on the error path: when a SecGeoLookupDb fails to open after
a good database was already loaded, the previous database is now released and
m_version is left as NOT_LOADED, so a later @geoLookup logs "Database is not
open. Use: SecGeoLookupDb directive." Before this change m_version stayed as
VERSION_MAXMIND while MMDB_open() had already clobbered the MMDB_s holding
the good database, so @geoLookup set no GEO variable and reported the
confusing "MaxMind: Got an error from libmaxminddb: The MaxMind DB file's
search tree is corrupt" instead. In both cases the configuration carrying the
failing directive is rejected with a parser error.

Out of scope, and deliberately not changed: ~ModSecurity only calls
GeoLookup::cleanUp() under WITH_GEOIP. Calling it for MaxMind as well would
be wrong under nginx, where the old cycle's ModSecurity instance is destroyed
*after* the new cycle has already parsed its rules -- it would close the
database the new configuration is about to use. The same hazard already
exists for the GEOIP build.
---
 src/utils/geo_lookup.cc                       |  7 +++
 .../regression/config-secgeolookupdb.json     | 51 +++++++++++++++++++
 test/test-suite.in                            |  1 +
 3 files changed, 59 insertions(+)
 create mode 100644 test/test-cases/regression/config-secgeolookupdb.json

diff --git a/src/utils/geo_lookup.cc b/src/utils/geo_lookup.cc
index 4e06a1ec52..c7b27e3a5a 100644
--- a/src/utils/geo_lookup.cc
+++ b/src/utils/geo_lookup.cc
@@ -66,6 +66,13 @@ bool GeoLookup::setDataBase(const std::string& filePath,
     std::string intGeo;
 #endif
 
+    /* A database may have been loaded already -- GeoLookup is a process wide
+     * singleton and SecGeoLookupDb can be used more than once, including on
+     * every re-parse of the configuration. Release it before loading the new
+     * one, otherwise the previous handle (and the mmap'ed database file) is
+     * leaked. */
+    cleanUp();
+
 #ifdef WITH_MAXMIND
     int status = MMDB_open(filePath.c_str(), MMDB_MODE_MMAP, &mmdb);
     if (status != MMDB_SUCCESS) {
diff --git a/test/test-cases/regression/config-secgeolookupdb.json b/test/test-cases/regression/config-secgeolookupdb.json
new file mode 100644
index 0000000000..c8a15db14b
--- /dev/null
+++ b/test/test-cases/regression/config-secgeolookupdb.json
@@ -0,0 +1,51 @@
+[
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "title": "Testing SecGeoLookupDb :: database informed twice [maxmind]",
+    "resource": "maxmind",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 123
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "localhost",
+        "User-Agent": "curl/7.38.0",
+        "Accept": "*/*",
+        "Content-Length": "0"
+      },
+      "uri": "/?key=value&key=other_value",
+      "method": "GET",
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Date": "Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type": "text/html",
+        "Content-Length": "8"
+      },
+      "body": [
+        "no need."
+      ]
+    },
+    "expected": {
+      "debug_log": "Target value: \"Brazil\" \\(Variable: GEO:COUNTRY_NAME\\)",
+      "http_code": 200
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecGeoLookupDb test-cases/data/GeoIP2-City-Test.mmdb",
+      "SecGeoLookupDb test-cases/data/GeoIP2-City-Test.mmdb",
+      "SecRule REMOTE_ADDR \"@geoLookup\" \"id:1,pass,t:trim\"",
+      "SecRule GEO \"@contains test \" \"id:2,pass,t:trim\""
+    ]
+  }
+]
diff --git a/test/test-suite.in b/test/test-suite.in
index ebda49fb81..05fdba7115 100644
--- a/test/test-suite.in
+++ b/test/test-suite.in
@@ -41,6 +41,7 @@ TESTS+=test/test-cases/regression/config-remove_by_msg.json
 TESTS+=test/test-cases/regression/config-remove_by_tag.json
 TESTS+=test/test-cases/regression/config-response_type.json
 TESTS+=test/test-cases/regression/config-secdefaultaction.json
+TESTS+=test/test-cases/regression/config-secgeolookupdb.json
 TESTS+=test/test-cases/regression/config-secremoterules.json
 TESTS+=test/test-cases/regression/config-update-action-by-id.json
 TESTS+=test/test-cases/regression/config-update-target-by-id.json