From be9f170561e1bf10fae0bad2c0c322e1c61fbbef Mon Sep 17 00:00:00 2001
From: Arne Welzel <arne.welzel@corelight.com>
Date: Wed, 21 Aug 2024 23:58:41 +0200
Subject: [PATCH] Analyzer: Do not add child analyzers when finished

Depending on an analyzer's implementation, its Done() method may
attempt to access analyzer or connection state when executing.
When this happens in the destructor of the parent analyzer during
the process of destructing a connection, this state may have been
deleted, resulting in use-after-free crashes or worse memory
corruption.

The following cases have been observed in the wild for when this happens.

  * PIA matching during Done() for undelivered TCP data enables a Spicy
    based analyzer which in turn attempts to raise an analyzer violation
    during Done()->EndOfData().

  * Spicy analyzers attaching new analyzers during their Done() processing
    which in turn attempt to use TCP() (to call FindChild()) during Done()
    while the analyzer tree / connection is being destructed.

The second scenario was previously found to happen in the HTTP analyzer
and fixed with 6ef9423f3cff13e6c73f97eb6a3a27d6f64cc320.

Plug these scenarios by short-circuiting AddChildAnalyzer() if the analyzer
or connection have finished or are being finished.
---
 src/Conn.h               |  3 +++
 src/analyzer/Analyzer.cc | 38 +++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/src/Conn.h b/src/Conn.h
index 67f41dc68d..c1d16f54bd 100644
--- a/src/Conn.h
+++ b/src/Conn.h
@@ -195,6 +195,9 @@ public:
 
     bool PermitWeird(const char* name, uint64_t threshold, uint64_t rate, double duration);
 
+    // Returns true once Done() is called.
+    bool IsFinished() { return finished; }
+
 private:
     friend class session::detail::Timer;
 
diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc
index e8f2b65d28..5db65b44a2 100644
--- a/src/analyzer/Analyzer.cc
+++ b/src/analyzer/Analyzer.cc
@@ -113,19 +113,7 @@ void Analyzer::CtorInit(const zeek::Tag& arg_tag, Connection* arg_conn) {
 
 Analyzer::~Analyzer() {
     assert(finished);
-
-    // Make sure any late entries into the analyzer tree are handled (e.g.
-    // from some Done() implementation).
-    LOOP_OVER_GIVEN_CHILDREN(i, new_children) {
-        if ( ! (*i)->finished )
-            (*i)->Done();
-    }
-
-    // Deletion of new_children done in separate loop in case a Done()
-    // implementation tries to inspect analyzer tree w/ assumption that
-    // all analyzers are still valid.
-    LOOP_OVER_GIVEN_CHILDREN(i, new_children)
-    delete *i;
+    assert(new_children.empty());
 
     LOOP_OVER_CHILDREN(i)
     delete *i;
@@ -330,6 +318,30 @@ void Analyzer::ForwardEndOfData(bool orig) {
 bool Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init) {
     auto t = analyzer->GetAnalyzerTag();
 
+    // Prevent attaching child analyzers to analyzer subtrees where
+    // either the parent has finished or is being removed. Further,
+    // don't attach analyzers when the connection has finished or is
+    // currently being finished (executing Done()).
+    //
+    // Scenarios in which analyzers have been observed that late in
+    // analyzer / connection lifetime are:
+    //
+    // * A DPD signature match on undelivered TCP data that is flushed
+    //   during Connection::Done(). The PIA analyzer activates a new
+    //   analyzer adding it to the TCP analyzer.
+    //
+    // * Analyzers flushing buffered state during Done(), resulting
+    //   in new analyzers being created.
+    //
+    // Analyzers added during Done() are problematic as calling Done()
+    // within the parent's destructor isn't safe, so we prevent these
+    // situations.
+    if ( Removing() || IsFinished() || Conn()->IsFinished() ) {
+        analyzer->Done();
+        delete analyzer;
+        return false;
+    }
+
     if ( HasChildAnalyzer(t) || IsPreventedChildAnalyzer(t) ) {
         analyzer->Done();
         delete analyzer;