From 2f67539c0fb291f23a922855e627d90a1acab778 Mon Sep 17 00:00:00 2001
From: Benjamin Bannier <benjamin.bannier@corelight.com>
Date: Sun, 29 Jun 2025 11:14:07 +0200
Subject: [PATCH] Prevent SQLite storage backend from serving expired entries

The SQLite storage backend implements expiration by hand and garbage
collection is done in `DoExpire`. This previously relied exclusively on
gets not running within `Storage::expire_interval` of the put, otherwise
we would potentially serve expired entries.

With this patch we explictly check that entries are not expired before
serving them so that the SQLite backend should never serve expired
entries.
---
 src/storage/backend/sqlite/SQLite.cc          | 10 +++-
 .../output                                    |  3 ++
 .../storage/sqlite-expiration-implicit.zeek   | 50 +++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output
 create mode 100644 testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek

diff --git a/src/storage/backend/sqlite/SQLite.cc b/src/storage/backend/sqlite/SQLite.cc
index 4d7447b77a..bb3f4b6f8a 100644
--- a/src/storage/backend/sqlite/SQLite.cc
+++ b/src/storage/backend/sqlite/SQLite.cc
@@ -190,7 +190,10 @@ OperationResult SQLite::DoOpen(OpenResultCallback* cb, RecordValPtr options) {
                                 "DO UPDATE SET value_str=?, expire_time=?",
                                 table_name.c_str()),
                         db),
-         std::make_pair(util::fmt("select value_str from %s where key_str=?", table_name.c_str()), db),
+         std::make_pair(util::fmt("select value_str, expire_time from %s where key_str=? and "
+                                  "(expire_time > ? OR expire_time == 0.0)",
+                                  table_name.c_str()),
+                        db),
          std::make_pair(util::fmt("delete from %s where key_str=?", table_name.c_str()), db),
 
          std::make_pair(
@@ -348,6 +351,11 @@ OperationResult SQLite::DoGet(ResultCallback* cb, ValPtr key) {
         return res;
     }
 
+    if ( auto res = CheckError(sqlite3_bind_double(stmt.get(), 2, run_state::network_time));
+         res.code != ReturnCode::SUCCESS ) {
+        return res;
+    }
+
     return Step(stmt.get(), true);
 }
 
diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output
new file mode 100644
index 0000000000..0730873d8c
--- /dev/null
+++ b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output
@@ -0,0 +1,3 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+BEFORE, [code=Storage::SUCCESS, error_str=<uninitialized>, value=v]
+AFTER, [code=Storage::KEY_NOT_FOUND, error_str=<uninitialized>, value=<uninitialized>]
diff --git a/testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek b/testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek
new file mode 100644
index 0000000000..aa9e4d9a98
--- /dev/null
+++ b/testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek
@@ -0,0 +1,50 @@
+# @TEST-EXEC: zeek %INPUT 2>&1 >output
+# @TEST-EXEC: btest-diff output
+
+@load base/frameworks/storage/sync
+@load policy/frameworks/storage/backend/sqlite
+
+# Manually control the clock.
+redef allow_network_time_forward = F;
+
+redef Storage::expire_interval = 1secs;
+
+event zeek_init()
+	{
+	local opts = Storage::BackendOptions(
+		$serializer=Storage::STORAGE_SERIALIZER_JSON,
+		$sqlite=Storage::Backend::SQLite::Options(
+			$database_path="test.sqlite",
+			$table_name="testing"));
+
+	local open_res = Storage::Sync::open_backend(
+		Storage::STORAGE_BACKEND_SQLITE,
+		opts,
+		string,
+		string);
+	local h = open_res$value;
+
+	local key="k";
+	local value="v";
+
+	# Expire entries well within `Storage::expire_interval` so `DoExpire` does not kick in.
+	local expire = Storage::expire_interval / 10;
+	local expire_time = network_time() + expire;
+	Storage::Sync::put(
+		h,
+		Storage::PutArgs(
+			$key=key,
+			$value=value,
+			$expire_time=expire));
+
+	# The entry we just put in exists.
+	local get = Storage::Sync::get(h, key);
+	print "BEFORE", get;
+
+	# Advance time.
+	set_network_time(network_time() + expire * 2);
+
+	# An expired value does not exist.
+	get = Storage::Sync::get(h, key);
+	print "AFTER", get;
+	}