Merge remote-tracking branch 'origin/topic/bbannier/spicy-cookie-nullptr-deref'

CHANGES

@@ -1,3 +1,21 @@
+7.1.0-dev.464 | 2024-11-05 17:38:41 +0100
+
+  * Fix potential nullptr deref in Spicy plugin runtime (Benjamin Bannier, Corelight)
+
+    If we added a file but the other side of the connection had already run
+    into a protocol violation and shut down we could previously have
+    dereferenced a null cookie. This patch fixes the code so it now throws
+    in such scenarios.
+
+  * Assume no Spicy cookie in most places (Benjamin Bannier, Corelight)
+
+    We would previously assert that it was available which could have lead
+    to aborts since when the analyzer for either side of a connection shuts
+    down the connection cookie could get cleared and become nil. This patch
+    reworks the code slightly so we now never assume it is available. We do
+    this by either throwing or by making the whole operation requesting the
+    cookie a noop.
+
 7.1.0-dev.461 | 2024-11-04 19:35:21 +0100
 
   * btest: Add detect-sqli test script (Arne Welzel, Corelight)

VERSION

@@ -1 +1 @@
-7.1.0-dev.461
+7.1.0-dev.464

src/spicy/runtime-support.cc

@@ -223,9 +223,8 @@ TypePtr rt::event_arg_type(const EventHandlerPtr& handler, const hilti::rt::inte
 
 ValPtr& rt::current_conn() {
     auto _ = hilti::rt::profiler::start("zeek/rt/current_conn");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( cookie->cache.conn )
             return cookie->cache.conn;
 
@@ -233,15 +232,15 @@ ValPtr& rt::current_conn() {
             cookie->cache.conn = x->analyzer->Conn()->GetVal();
             return cookie->cache.conn;
         }
-    else
+    }
+
     throw ValueUnavailable("$conn not available");
 }
 
 ValPtr& rt::current_is_orig() {
     auto _ = hilti::rt::profiler::start("zeek/rt/current_is_orig");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( cookie->cache.is_orig )
             return cookie->cache.is_orig;
 
@@ -249,21 +248,22 @@ ValPtr& rt::current_is_orig() {
             cookie->cache.is_orig = val_mgr->Bool(x->is_orig);
             return cookie->cache.is_orig;
         }
-    else
+    }
+
     throw ValueUnavailable("$is_orig not available");
 }
 
 void rt::debug(const std::string& msg) {
     auto _ = hilti::rt::profiler::start("zeek/rt/debug");
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
+    if ( ! cookie )
+        return SPICY_DEBUG(msg);
+
     rt::debug(*cookie, msg);
 }
 
 void rt::debug(const Cookie& cookie, const std::string& msg) {
     auto _ = hilti::rt::profiler::start("zeek/rt/debug");
-    std::string name;
-    std::string id;
 
     if ( const auto p = cookie.protocol ) {
         auto name = p->analyzer->GetAnalyzerName();
@@ -285,11 +285,13 @@ void rt::debug(const Cookie& cookie, const std::string& msg) {
 inline rt::cookie::FileStateStack* _file_state_stack(rt::Cookie* cookie) {
     auto _ = hilti::rt::profiler::start("zeek/rt/file_state_stack");
 
+    if ( cookie ) {
         if ( auto c = cookie->protocol )
             return c->is_orig ? &c->fstate_orig : &c->fstate_resp;
         else if ( auto f = cookie->file )
             return &f->fstate;
-    else
+    }
+
     throw rt::ValueUnavailable("no current connection or file available");
 }
 
@@ -313,24 +315,23 @@ inline const rt::cookie::FileState* _file_state(rt::Cookie* cookie, std::optiona
 
 ValPtr rt::current_file() {
     auto _ = hilti::rt::profiler::start("zeek/rt/current_file");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto x = cookie->file )
             return x->analyzer->GetFile()->ToVal();
         else if ( auto* fstate = _file_state(cookie, {}) ) {
             if ( auto* f = file_mgr->LookupFile(fstate->fid) )
                 return f->ToVal();
         }
+    }
 
     throw ValueUnavailable("$file not available");
 }
 
 ValPtr rt::current_packet() {
     auto _ = hilti::rt::profiler::start("zeek/rt/current_packet");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto c = cookie->packet ) {
             if ( ! c->packet_val )
                 // We cache the built value in case we need it multiple times.
@@ -338,32 +339,33 @@ ValPtr rt::current_packet() {
 
             return c->packet_val;
         }
-    else
+    }
+
     throw ValueUnavailable("$packet not available");
 }
 
 hilti::rt::Bool rt::is_orig() {
     auto _ = hilti::rt::profiler::start("zeek/rt/is_orig");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto x = cookie->protocol )
             return x->is_orig;
-    else
+    }
+
     throw ValueUnavailable("is_orig() not available in current context");
 }
 
 std::string rt::uid() {
     auto _ = hilti::rt::profiler::start("zeek/rt/uid");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto c = cookie->protocol ) {
             // Retrieve the ConnVal() so that we ensure the UID has been set.
             c->analyzer->ConnVal();
             return c->analyzer->Conn()->GetUID().Base62("C");
         }
-    else
+    }
+
     throw ValueUnavailable("uid() not available in current context");
 }
 
@@ -395,49 +397,48 @@ std::tuple<hilti::rt::Address, hilti::rt::Port, hilti::rt::Address, hilti::rt::P
         hilti::rt::cannot_be_reached();
     };
 
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
-    assert(cookie);
-
         if ( auto c = cookie->protocol ) {
             const auto* conn = c->analyzer->Conn();
-        return std::make_tuple(convert_address(conn->OrigAddr()), convert_port(conn->OrigPort(), conn->ConnTransport()),
+            return std::make_tuple(convert_address(conn->OrigAddr()),
+                                   convert_port(conn->OrigPort(), conn->ConnTransport()),
                                    convert_address(conn->RespAddr()),
                                    convert_port(conn->RespPort(), conn->ConnTransport()));
         }
-    else
+    }
+
     throw ValueUnavailable("conn_id() not available in current context");
 }
 
 void rt::flip_roles() {
     auto _ = hilti::rt::profiler::start("zeek/rt/flip_roles");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         rt::debug(*cookie, "flipping roles");
 
         if ( auto x = cookie->protocol )
-        x->analyzer->Conn()->FlipRoles();
+            return x->analyzer->Conn()->FlipRoles();
-    else
+    }
+
     throw ValueUnavailable("flip_roles() not available in current context");
 }
 
 hilti::rt::integer::safe<uint64_t> rt::number_packets() {
     auto _ = hilti::rt::profiler::start("zeek/rt/number_packets");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto x = cookie->protocol ) {
             return x->num_packets;
         }
-    else
+    }
+
     throw ValueUnavailable("number_packets() not available in current context");
 }
 
 void rt::confirm_protocol() {
     auto _ = hilti::rt::profiler::start("zeek/rt/confirm_protocol");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( cookie->cache.confirmed )
             return;
 
@@ -447,6 +448,8 @@ void rt::confirm_protocol() {
             cookie->cache.confirmed = true;
             return x->analyzer->AnalyzerConfirmation(tag);
         }
+    }
+
     throw ValueUnavailable("no current connection available");
 }
 
@@ -471,17 +474,16 @@ void rt::reject_protocol(const std::string& reason) {
 
 void rt::weird(const std::string& id, const std::string& addl) {
     auto _ = hilti::rt::profiler::start("zeek/rt/weird");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( const auto x = cookie->protocol )
-        x->analyzer->Weird(id.c_str(), addl.data());
+            return x->analyzer->Weird(id.c_str(), addl.data());
         else if ( const auto x = cookie->file )
-        zeek::reporter->Weird(x->analyzer->GetFile(), id.c_str(), addl.data());
+            return zeek::reporter->Weird(x->analyzer->GetFile(), id.c_str(), addl.data());
-    else if ( const auto x = cookie->packet ) {
+        else if ( const auto x = cookie->packet )
-        x->analyzer->Weird(id.c_str(), x->packet, addl.c_str());
+            return x->analyzer->Weird(id.c_str(), x->packet, addl.c_str());
     }
-    else
+
     throw ValueUnavailable("none of $conn, $file, or $packet available for weird reporting");
 }
 
@@ -499,11 +501,11 @@ void rt::protocol_begin(const std::optional<std::string>& analyzer, const ::hilt
     // doesn't need to track what the other side already did.
 
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
+
+    if ( ! cookie || ! cookie->protocol )
+        throw ValueUnavailable("no current connection available");
 
     auto c = cookie->protocol;
-    if ( ! c )
-        throw ValueUnavailable("no current connection available");
 
     switch ( proto.value() ) {
         case ::hilti::rt::Protocol::TCP: {
@@ -547,12 +549,12 @@ void rt::protocol_begin(const ::hilti::rt::Protocol& proto) { return protocol_be
 
 rt::ProtocolHandle rt::protocol_handle_get_or_create(const std::string& analyzer, const ::hilti::rt::Protocol& proto) {
     auto _ = hilti::rt::profiler::start("zeek/rt/protocol_handle_get_or_create");
+
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
+    if ( ! cookie || ! cookie->protocol )
+        throw ValueUnavailable("no current connection available");
 
     auto c = cookie->protocol;
-    if ( ! c )
-        throw ValueUnavailable("no current connection available");
 
     switch ( proto.value() ) {
         case ::hilti::rt::Protocol::TCP: {
@@ -623,11 +625,11 @@ static void protocol_data_in(const hilti::rt::Bool& is_orig, const hilti::rt::By
                              const std::optional<rt::ProtocolHandle>& h) {
     auto _ = hilti::rt::profiler::start("zeek/rt/protocol_data_in");
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
+
+    if ( ! cookie || ! cookie->protocol )
+        throw ValueUnavailable("no current connection available");
 
     auto c = cookie->protocol;
-    if ( ! c )
-        throw ValueUnavailable("no current connection available");
 
     // We need to copy the data here to be on the safe side: the streaming
     // input methods expect the data to stay around until they return. At first
@@ -719,11 +721,11 @@ void rt::protocol_gap(const hilti::rt::Bool& is_orig, const hilti::rt::integer::
                       const hilti::rt::integer::safe<uint64_t>& len, const std::optional<rt::ProtocolHandle>& h) {
     auto _ = hilti::rt::profiler::start("zeek/rt/protocol_gap");
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
+
+    if ( ! cookie || ! cookie->protocol )
+        throw ValueUnavailable("no current connection available");
 
     auto c = cookie->protocol;
-    if ( ! c )
-        throw ValueUnavailable("no current connection available");
 
     switch ( h->protocol().value() ) {
         case ::hilti::rt::Protocol::TCP: {
@@ -761,25 +763,25 @@ void rt::protocol_gap(const hilti::rt::Bool& is_orig, const hilti::rt::integer::
 
 void rt::protocol_end() {
     auto _ = hilti::rt::profiler::start("zeek/rt/protocol_end");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         auto c = cookie->protocol;
         if ( ! c )
             throw ValueUnavailable("no current connection available");
 
         for ( const auto& i : c->analyzer->GetChildren() )
             c->analyzer->RemoveChildAnalyzer(i);
+    }
 }
 
 void rt::protocol_handle_close(const ProtocolHandle& handle) {
     auto _ = hilti::rt::profiler::start("zeek/rt/protocol_handle_close");
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
+
+    if ( ! cookie || ! cookie->protocol )
+        throw ValueUnavailable("no current connection available");
 
     auto c = cookie->protocol;
-    if ( ! c )
-        throw ValueUnavailable("no current connection available");
 
     switch ( handle.protocol().value() ) {
         case ::hilti::rt::Protocol::TCP: {
@@ -827,7 +829,8 @@ rt::cookie::FileState* rt::cookie::FileStateStack::push(std::optional<std::strin
         fid = *fid_provided;
     else {
         auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-        assert(cookie);
+        if ( ! cookie )
+            throw ValueUnavailable("no current connection available");
 
         if ( auto c = cookie->protocol ) {
             auto tag = spicy_mgr->tagForProtocolAnalyzer(c->analyzer->GetAnalyzerTag());
@@ -899,39 +902,39 @@ static void _data_in(const char* data, uint64_t len, std::optional<uint64_t> off
 
 void rt::terminate_session() {
     auto _ = hilti::rt::profiler::start("zeek/rt/terminate_session");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto c = cookie->protocol ) {
             assert(session_mgr);
-        session_mgr->Remove(c->analyzer->Conn());
+            return session_mgr->Remove(c->analyzer->Conn());
         }
-    else
+    }
+
     throw spicy::rt::ValueUnavailable("terminate_session() not available in the current context");
 }
 
 void rt::skip_input() {
     auto _ = hilti::rt::profiler::start("zeek/rt/skip_input");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto p = cookie->protocol )
-        p->analyzer->SetSkip(true);
+            return p->analyzer->SetSkip(true);
         else if ( auto f = cookie->file )
-        f->analyzer->SetSkip(true);
+            return f->analyzer->SetSkip(true);
-    else
+    }
+
     throw spicy::rt::ValueUnavailable("skip() not available in the current context");
 }
 
 std::string rt::fuid() {
     auto _ = hilti::rt::profiler::start("zeek/rt/fuid");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
         if ( auto f = cookie->file ) {
             if ( auto file = f->analyzer->GetFile() )
                 return file->GetID();
         }
+    }
 
     throw ValueUnavailable("fuid() not available in current context");
 }
@@ -1003,6 +1006,9 @@ void rt::file_gap(const hilti::rt::integer::safe<uint64_t>& offset, const hilti:
                   const std::optional<std::string>& fid) {
     auto _ = hilti::rt::profiler::start("zeek/rt/file_gap");
     auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
+    if ( ! cookie )
+        throw spicy::rt::ValueUnavailable("file_gap() not available in the current context");
+
     auto* fstate = _file_state(cookie, fid);
 
     if ( auto c = cookie->protocol ) {
@@ -1024,12 +1030,14 @@ void rt::file_end(const std::optional<std::string>& fid) {
 
 void rt::forward_packet(const hilti::rt::integer::safe<uint32_t>& identifier) {
     auto _ = hilti::rt::profiler::start("zeek/rt/forward_packet");
-    auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie());
-    assert(cookie);
 
-    if ( auto c = cookie->packet )
+    if ( auto cookie = static_cast<Cookie*>(hilti::rt::context::cookie()) ) {
+        if ( auto c = cookie->packet ) {
             c->next_analyzer = identifier;
-    else
+            return;
+        }
+    }
+
     throw ValueUnavailable("no current packet analyzer available");
 }