Parameterize offline-storage deletes and harden kill-switch response handling

lib/android_build/app/src/main/cpp/CMakeLists.txt

@@ -55,6 +55,7 @@ set(TESTS_SRCS
         ${SDK_ROOT}/tests/unittests/HttpDeflateCompressionTests.cpp
         ${SDK_ROOT}/tests/unittests/HttpRequestEncoderTests.cpp
         ${SDK_ROOT}/tests/unittests/HttpResponseDecoderTests.cpp
+        ${SDK_ROOT}/tests/unittests/KillSwitchManagerTests.cpp
         ${SDK_ROOT}/tests/unittests/LoggerTests.cpp
         ${SDK_ROOT}/tests/unittests/LogManagerImplTests.cpp
         ${SDK_ROOT}/tests/unittests/LogSessionDataTests.cpp

lib/offline/KillSwitchManager.hpp

@@ -7,9 +7,12 @@
 
 #include "pal/PAL.hpp"
 
+#include <list>
 #include <map>
-#include <string>
 #include <mutex>
+#include <stdexcept>
+#include <string>
+#include <vector>
 
 #include <atomic>
 
@@ -39,8 +42,8 @@ namespace MAT_NS_BEGIN {
             std::string timeStr = headers.get("Retry-After");
             if (!timeStr.empty())
             {
-                int64_t timeinSecs = std::stoi(timeStr);
-                if (timeinSecs > 0)
+                int64_t timeinSecs = 0;
+                if (tryParseSeconds(timeStr, timeinSecs) && timeinSecs > 0)
                 {
                     std::lock_guard<std::mutex> guard(m_lock);
                     m_retryAfterExpiryTime = PAL::getUtcSystemTime() + timeinSecs;
@@ -57,12 +60,22 @@ namespace MAT_NS_BEGIN {
 
                 for (std::multimap<std::string, std::string>::const_iterator it = ret.first; it != ret.second; ++it)
                 {
-                    std::string token = std::move(it->second);
+                    // it->second is a const reference (const_iterator), so this is a
+                    // copy; no std::move (moving from const silently copies anyway).
+                    std::string token = it->second;
                     size_t pos = token.find(':');
                     if (pos != std::string::npos)
                     {
                         // Strip suffix and assume ':all' events of that tenant are killed
-                        token.erase(pos, token.length() - pos);
+                        token.erase(pos);
+                    }
+                    // Reject kill-tokens with control characters (defensive
+                    // hygiene; see isValidTenantToken). Any other opaque value is
+                    // safe to act on and must remain killable -- over-restricting
+                    // would let that tenant escape the kill.
+                    if (!isValidTenantToken(token))
+                    {
+                        continue;
                     }
                     killtokensVector.push_back(token);
                 }
@@ -71,7 +84,7 @@ namespace MAT_NS_BEGIN {
                 std::string timeString = headers.get("kill-duration");
                 if (!timeString.empty())
                 {
-                    timeinSecs = std::stoi(timeString);
+                    tryParseSeconds(timeString, timeinSecs);
                 }
 
                 if (killtokensVector.size() > 0 && timeinSecs > 0)
@@ -156,6 +169,109 @@ namespace MAT_NS_BEGIN {
         }
 
     private:
+        // Parse a count of seconds from a response-header value (Retry-After /
+        // kill-duration). Returns false when the value is malformed or out of
+        // range instead of letting std::stoll throw: the worker thread that drives
+        // handleResponse has no exception guard, so a throw here would crash the
+        // process.
+        //
+        // RFC 7231 allows Retry-After to be either delay-seconds or an HTTP-date.
+        // We deliberately accept only delay-seconds and ignore the HTTP-date form:
+        //   - The collector this SDK targets sends Retry-After as delay-seconds, so
+        //     the date form does not occur in practice.
+        //   - An HTTP-date is absolute and would have to be converted to a delay
+        //     against the device clock, which a telemetry SDK cannot trust (it has
+        //     a separate clock-skew channel for exactly this reason); a skewed clock
+        //     would yield a wrong, negative, or huge back-off.
+        //   - Parsing it would add an error-prone three-format date parser on this
+        //     no-exception-guard worker thread for no practical benefit.
+        // Ignoring an unparseable value degrades safely: the SDK's own retry
+        // back-off still applies; only the server's exact instant is not honored.
+        //
+        // RFC 7231 delay-seconds is 1*DIGIT, optionally surrounded by HTTP optional
+        // whitespace (OWS = SP / HTAB). We trim only OWS and require the remaining
+        // value to be all digits. This rejects malformed inputs that std::stoll
+        // would otherwise accept -- a leading '+'/'-' sign, or leading CR/LF and
+        // other control whitespace that stoll silently skips -- and keeps leading
+        // and trailing handling symmetric.
+        static bool tryParseSeconds(const std::string& value, int64_t& outSeconds)
+        {
+            outSeconds = 0;
+            size_t begin = 0;
+            size_t end = value.size();
+            while (begin < end && (value[begin] == ' ' || value[begin] == '\t'))
+            {
+                ++begin;
+            }
+            while (end > begin && (value[end - 1] == ' ' || value[end - 1] == '\t'))
+            {
+                --end;
+            }
+            if (begin == end)
+            {
+                return false;
+            }
+            for (size_t i = begin; i < end; ++i)
+            {
+                if (value[i] < '0' || value[i] > '9')
+                {
+                    return false;
+                }
+            }
+            try
+            {
+                // The substring is all digits, so std::stoll itself can only throw
+                // std::out_of_range; substr() may also throw (e.g. std::bad_alloc).
+                // Either way the std::exception catch below ignores the value rather
+                // than crashing.
+                const long long parsed = std::stoll(value.substr(begin, end - begin));
+                // Clamp to a value that cannot overflow when later added to a current
+                // UTC timestamp (seconds) to compute an expiry time. No legitimate
+                // Retry-After / kill-duration approaches this; an absurd value is
+                // capped instead of wrapping the expiry into the past.
+                const int64_t kMaxSeconds = 100LL * 365 * 24 * 60 * 60; // ~100 years
+                outSeconds = (parsed > kMaxSeconds) ? kMaxSeconds : static_cast<int64_t>(parsed);
+                return true;
+            }
+            catch (const std::exception&)
+            {
+                return false;
+            }
+        }
+
+        // Tenant tokens are opaque (they may legitimately contain spaces, quotes,
+        // etc.). Every sink that consumes the token handles raw bytes safely -- the
+        // offline-storage DELETE is parameterized (SQLite bind / Room DAO) and the
+        // in-memory match is a plain string compare -- so any printable value must
+        // remain killable; over-restricting would let that tenant escape the kill.
+        // We reject empty tokens and control characters as defensive hygiene:
+        // a legitimate token never contains control characters, an embedded NUL would
+        // be truncated by the Room JNI NewStringUTF(c_str()) call (acting on the wrong
+        // token), and it keeps the value safe if it ever reaches a log/display sink.
+        // We also bound the length: real tenant tokens are a fixed, small size
+        // (~74 chars; see HttpRequestEncoder.cpp), so kMaxTenantTokenBytes (256) is a
+        // generous ceiling -- it caps memory growth from a malicious/oversized
+        // kill-token in an untrusted HTTP response, while staying far above any real
+        // token so a legitimate tenant can never be made unkillable by this check.
+        static constexpr size_t kMaxTenantTokenBytes = 256;
+        static bool isValidTenantToken(const std::string& token)
+        {
+            if (token.empty() || token.size() > kMaxTenantTokenBytes)
+            {
+                return false;
+            }
+            for (unsigned char c : token)
+            {
+                // Reject C0 control characters and DEL; allow any other byte
+                // (printable ASCII and UTF-8 sequences).
+                if (c < 0x20 || c == 0x7f)
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
         std::map<std::string, int64_t> m_tokenTime;
         std::mutex      m_lock;
         bool            m_isRetryAfterActive;

lib/offline/OfflineStorage_SQLite.cpp

@@ -12,6 +12,7 @@
 #include <algorithm>
 #include <numeric>
 #include <set>
+#include <stdexcept>
 
 namespace MAT_NS_BEGIN {
 
@@ -398,7 +399,6 @@ namespace MAT_NS_BEGIN {
 
     void OfflineStorage_SQLite::DeleteRecords(const std::map<std::string, std::string> & whereFilter)
     {
-        UNREFERENCED_PARAMETER(whereFilter);
         if (!isOpen()) {
             return;
         }
@@ -413,40 +413,100 @@ namespace MAT_NS_BEGIN {
                 return;
             }
 #endif
-            auto formatter = [&](const std::map<std::string, std::string> & whereFilter)
+            // SECURITY: build a parameterized statement. Column names are taken
+            // from a fixed whitelist (never from server/response data) and every
+            // value is bound via sqlite3_bind_* instead of being concatenated into
+            // the SQL text. This prevents untrusted values (for example kill-token
+            // tenant ids carried in a collector response) from altering the
+            // statement or appending additional "stacked" statements.
+            enum class ColumnType { Text, Integer };
+            static const std::map<std::string, ColumnType> kAllowedColumns = {
+                { "record_id",    ColumnType::Text },
+                { "tenant_token", ColumnType::Text },
+                { "latency",      ColumnType::Integer },
+                { "persistence",  ColumnType::Integer },
+                { "retry_count",  ColumnType::Integer },
+            };
+
+            std::string clause;
+            std::vector<std::map<std::string, std::string>::const_iterator> boundColumns;
+            for (auto it = whereFilter.begin(); it != whereFilter.end(); ++it)
             {
-                std::string clause;
-                for (const auto &kv : whereFilter)
+                if (kAllowedColumns.find(it->first) == kAllowedColumns.end())
                 {
-                    bool quotes = false;
-                    if ((kv.first == "record_id") ||
-                        (kv.first == "tenant_token"))
+                    // Fail closed: an unrecognized column cannot be honored, so
+                    // delete nothing rather than running a looser predicate. This
+                    // matches MemoryStorage, whose matcher treats an unknown column
+                    // as "no match".
+                    LOG_WARN("DeleteRecords: unrecognized filter column '%s'; nothing deleted", it->first.c_str());
+                    return;
+                }
+                clause += clause.empty() ? "" : " AND ";
+                clause += it->first;
+                clause += "=?";
+                boundColumns.push_back(it);
+            }
+
+            // Never run a DELETE with no predicate, which would erase the table.
+            if (clause.empty())
+            {
+                LOG_WARN("DeleteRecords: no recognized filter columns; nothing deleted");
+                return;
+            }
+
+            const std::string sql = "DELETE FROM " TABLE_NAME_EVENTS " WHERE " + clause;
+            SqliteStatement stmt(*m_db, sql.c_str());
+            if (stmt.handle() == nullptr)
+            {
+                LOG_ERROR("DeleteRecords: failed to prepare delete statement for table " TABLE_NAME_EVENTS ": %s",
+                    g_sqlite3Proxy->sqlite3_errmsg(*m_db));
+                return;
+            }
+            int idx = 1;
+            for (const auto& it : boundColumns)
+            {
+                const std::string& value = it->second;
+                int rc = SQLITE_OK;
+                if (kAllowedColumns.at(it->first) == ColumnType::Text)
+                {
+                    rc = g_sqlite3Proxy->sqlite3_bind_text(stmt.handle(), idx,
+                        value.data(), static_cast<int>(value.size()), SQLITE_STATIC);
+                }
+                else
+                {
+                    int64_t numeric = 0;
+                    size_t consumed = 0;
+                    try
                     {
-                        // string types
-                        quotes = true;
-                    } 
-                    else if (
-                        // integer types
-                        (kv.first == "latency") ||
-                        (kv.first == "persistence") ||
-                        (kv.first == "retry_count"))
+                        numeric = static_cast<int64_t>(std::stoll(value, &consumed));
+                    }
+                    catch (const std::exception&)
                     {
-                        quotes = false;
+                        consumed = 0;
                     }
-                    if (!clause.empty())
+                    // Treat a non-numeric value for an integer column as an invalid
+                    // filter and abort, rather than coercing to 0 and deleting rows
+                    // that happen to match 0.
+                    if (value.empty() || consumed != value.size())
                     {
-                        clause += " AND ";
+                        LOG_WARN("DeleteRecords: invalid numeric filter value for column '%s'; nothing deleted",
+                            it->first.c_str());
+                        return;
                     }
-                    clause += kv.first;
-                    clause += "=";
-                    clause += (quotes) ?
-                        ("\"" + kv.second + "\"") :
-                        kv.second;
+                    rc = g_sqlite3Proxy->sqlite3_bind_int64(stmt.handle(), idx, numeric);
                 }
-                return clause;
-            };
-            std::string sql = "DELETE FROM " TABLE_NAME_EVENTS " WHERE ";
-            Execute(sql + formatter(whereFilter));
+                if (rc != SQLITE_OK)
+                {
+                    LOG_ERROR("DeleteRecords: failed to bind filter column '%s': %d (%s)",
+                        it->first.c_str(), rc, g_sqlite3Proxy->sqlite3_errmsg(*m_db));
+                    return;
+                }
+                ++idx;
+            }
+            if (!stmt.execute())
+            {
+                LOG_ERROR("DeleteRecords: failed to execute delete for table " TABLE_NAME_EVENTS);
+            }
         }
     }
 

tests/unittests/CMakeLists.txt

@@ -27,6 +27,7 @@ set(SRCS
   HttpResponseDecoderTests.cpp
   HttpServerTests.cpp
   InformationProviderImplTests.cpp
+  KillSwitchManagerTests.cpp
   LoggerTests.cpp
   LogManagerImplTests.cpp
   LogSessionDataTests.cpp