Spicy SSL analyzer - address feedback

Minor proposed changes from https://github.com/zeek/zeek/pull/3765,
mostly cosmetic.

Changes CI to be part of an already existing spicy CI job.

.cirrus.yml

@@ -229,17 +229,6 @@ debian12_binary_task:
   env:
     ZEEK_CI_CONFIGURE_FLAGS: *BINARY_CONFIG
 
-debian12_spicy_ssl_task:
-  container:
-    # Just use a recent/common distro to run a test using spicy ssl.
-    # Debian 12 (bookworm) EOL: TBD
-    dockerfile: ci/debian-12/Dockerfile
-    << : *RESOURCES_TEMPLATE
-  << : *CI_TEMPLATE
-  << : *SKIP_TASK_ON_PR
-  env:
-    ZEEK_CI_CONFIGURE_FLAGS: *SPICY_SSL_CONFIG
-
 debian11_task:
   container:
     # Debian 11 EOL: June 2026
@@ -298,6 +287,7 @@ ubuntu22_task:
         $CIRRUS_BRANCH =~ 'release/.*' ||
         $CIRRUS_CRON == 'benchmark-nightly' )
 
+# Also enable Spicy SSL for this
 ubuntu22_spicy_task:
   container:
     # Ubuntu 22.04 EOL: April 2027
@@ -306,7 +296,7 @@ ubuntu22_spicy_task:
   << : *CI_TEMPLATE
   env:
     ZEEK_CI_CREATE_ARTIFACT: 1
-  test_script: true  # Don't run tests, these are redundant.
+    ZEEK_CI_CONFIGURE_FLAGS: *SPICY_SSL_CONFIG
   spicy_install_analyzers_script: ./ci/spicy-install-analyzers.sh
   upload_binary_artifacts:
     path: build.tgz

src/analyzer/protocol/rdp/RDP.h

@@ -1,6 +1,5 @@
 #pragma once
 
-#include "zeek/analyzer/protocol/pia/PIA.h"
 #include "zeek/analyzer/protocol/rdp/events.bif.h"
 #include "zeek/analyzer/protocol/rdp/rdp_pac.h"
 #include "zeek/analyzer/protocol/tcp/TCP.h"

src/analyzer/protocol/ssl/CMakeLists.txt

@@ -33,6 +33,20 @@ if (NOT ENABLE_SPICY_SSL)
         ssl-dtls-protocol.pac
         dtls-protocol.pac
         ssl-defs.pac)
+
+    if (NOT DISABLE_SPICY)
+        # Even if we are using the binpac SSL analyzer make sure the Spicy
+        # analyzer builds successfully. We use a debug build (`-d`) to perform
+        # a faster build.
+        add_custom_command(
+            OUTPUT spicy-ssl.hlto
+            COMMENT "Compiling Spicy SSL analyzer"
+            COMMAND spicyz -d spicy/SSL.spicy spicy/SSL.evt spicy/support.cc -o
+                    ${CMAKE_CURRENT_BINARY_DIR}/spicy-ssl.hlto
+            WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+            DEPENDS spicyz spicy/SSL.spicy spicy/SSL.evt spicy/support.cc)
+        add_custom_target(spicy-ssl ALL DEPENDS spicy-ssl.hlto)
+    endif ()
 else ()
     add_subdirectory(spicy)
     zeek_add_plugin(Zeek SSL SOURCES Plugin.cc BIFS functions.bif)

src/analyzer/protocol/ssl/functions.bif

@@ -18,7 +18,7 @@
 function set_ssl_established%(c: connection%): bool
 	%{
 #ifndef ENABLE_SPICY_SSL
-/* not implemented for spicy ssl */
+/* not implemented for Spicy ssl */
 	zeek::analyzer::Analyzer* sa = c->FindAnalyzer("SSL");
 
 	if ( sa )
@@ -42,7 +42,7 @@ function set_ssl_established%(c: connection%): bool
 function set_secret%(c: connection, secret: string%): bool
 	%{
 #ifndef ENABLE_SPICY_SSL
-/* not implemented for spicy ssl */
+/* not implemented for Spicy ssl */
 	analyzer::Analyzer* sa = c->FindAnalyzer("SSL");
 
 	if ( sa )
@@ -66,7 +66,7 @@ function set_secret%(c: connection, secret: string%): bool
 function set_keys%(c: connection, keys: string%): bool
 	%{
 #ifndef ENABLE_SPICY_SSL
-/* not implemented for spicy ssl */
+/* not implemented for Spicy ssl */
 	analyzer::Analyzer* sa = c->FindAnalyzer("SSL");
 
 	if ( sa )

src/analyzer/protocol/ssl/spicy/SSL.evt

@@ -1,3 +1,5 @@
+# Copyright (c) 2024 by the Zeek Project. See LICENSE for details.
+
 protocol analyzer SSL over TCP:
 	parse with SSL::Message;
 
@@ -84,4 +86,4 @@ on SSL::CertificateStatus -> event ssl_stapled_ocsp($conn, $is_orig, self.respon
 on SSL::CertificateRequest if ( SSL::uses_signature_and_hashalgorithm(sh) ) -> event ssl_certificate_request($conn, SSL::get_direction(sh), self.certificate_types, self.supported_signature_algorithms.supported_signature_algorithms_converted, self.certificate_authorities);
 on SSL::CertificateRequest if ( ! SSL::uses_signature_and_hashalgorithm(sh) ) -> event ssl_certificate_request($conn, SSL::get_direction(sh), self.certificate_types, SSL::create_empty_sigmature_algorithms(), self.certificate_authorities);
 
-on SSL::DirectionCheck::%done if ( self.was_flipped ) -> event ssl_connection_flipped($conn);
+on SSL::DirectionCheck if ( self.was_flipped ) -> event ssl_connection_flipped($conn);

src/analyzer/protocol/ssl/spicy/SSL.spicy

@@ -1,3 +1,5 @@
+# Copyright (c) 2024 by the Zeek Project. See LICENSE for details.
+
 module SSL;
 
 import spicy;
@@ -578,21 +580,6 @@ type Share = unit {
     var established: bool;
     var client_certificate_depth: uint32;
     var server_certificate_depth: uint32;
-
-    on %init {
-        self.ccs_seen = 0;
-        self.invalid_dtls_version_count = 0;
-        self.tls_13 = False;
-        self.negotiated_version = UNKNOWN_VERSION;
-        self.flipped = False;
-        self.flip_already_alerted = False;
-        self.server_encrypted = False;
-        self.client_encrypted = False;
-        self.both_sides_encrypted_first_time = False;
-        self.established = False;
-        self.client_certificate_depth = 0;
-        self.server_certificate_depth = 0;
-    }
 };
 
 function get_encrypted(sh: Share): bool {
@@ -781,8 +768,10 @@ type DTLSRecordFragment = unit(content_type: uint8, handshakesink: sink&, alerts
 };
 
 type PlaintextRecord = unit(content_type: uint8, handshakesink: sink&, alertsink: sink&, inout msg: Message, inout sh: Share) {
-    length: uint16;
     var encrypted: bool;
+    length: uint16 {
+        self.encrypted = determine_encryption_on(self, content_type, handshakesink, alertsink, sh);
+    }
     # convenient triggers to hang stuff in the evt file from. Two of them for event ordering :)
     trigger_zero: void;
     trigger_one: void;
@@ -801,10 +790,6 @@ type PlaintextRecord = unit(content_type: uint8, handshakesink: sink&, alertsink
     };
     trigger_two: void;
 
-    on length {
-        self.encrypted = determine_encryption_on(self, content_type, handshakesink, alertsink, sh);
-    }
-
     on ccs {
         # I know this looks a bit weird. Basically - in TLS 1.3, CCS is meaningless
         # fluff that just is used to pretend to TLS 1.2 devices listening in that
@@ -1058,17 +1043,7 @@ type ServerHelloChoice = unit(len: uint64, msg: Message, inout sh: Share) {
         server_version0: 0..7;
         server_version1: 8..15;
         server_version: 0..15;
-    };
+    } {
-    var negotiated_version: uint16;
-
-    switch (self.negotiated_version) {
-        TLSv13,
-        TLSv13_draft,
-        0x7F00 -> sh_one_three: ServerHelloOneThree(len, msg, sh, self.sv.server_version);
-        * -> sh_normal: ServerHello(len, msg, sh, self.sv.server_version);
-    };
-
-    on sv {
         # print "Got server version", self.sv.server_version0, self.sv.server_version1, self.sv.server_version;
         sh.chosen_version_sh_outer = self.sv.server_version;
         set_version(self.sv.server_version, sh);
@@ -1082,6 +1057,14 @@ type ServerHelloChoice = unit(len: uint64, msg: Message, inout sh: Share) {
             self.negotiated_version = self.sv.server_version;
         }
     }
+    var negotiated_version: uint16;
+
+    switch (self.negotiated_version) {
+        TLSv13,
+        TLSv13_draft,
+        0x7F00 -> sh_one_three: ServerHelloOneThree(len, msg, sh, self.sv.server_version);
+        * -> sh_normal: ServerHello(len, msg, sh, self.sv.server_version);
+    };
 };
 
 # Draft versions of TLS 1.3 had a different server hello.

src/analyzer/protocol/ssl/spicy/support.cc

@@ -1,6 +1,7 @@
 // Copyright (c) 2023 by the Zeek Project. See COPYING for details.
 
 #include <hilti/rt/libhilti.h>
+#include <cassert>
 
 #include "zeek/Desc.h"
 #include "zeek/file_analysis/Manager.h"
@@ -21,7 +22,7 @@ std::string ssl_get_certificate_fuid(const hilti::rt::Bool& is_client, const hil
     file_handle.AddRaw(is_client ? "T" : "F", 1);
     c->analyzer->Conn()->IDString(&file_handle);
 
-    file_handle.Add((uint32_t)pos);
+    file_handle.Add(pos.Ref());
     std::string file_id = zeek::file_mgr->HashHandle(file_handle.Description());
     return file_id;
 }
@@ -44,6 +45,7 @@ std::string ssl_get_ocsp_fuid() {
     return file_id;
 }
 
+// TODO: it would make sense to make this available for all users of Spicy
 bool ssl_is_partial_tcp() {
     auto cookie = static_cast<zeek::spicy::rt::Cookie*>(hilti::rt::context::cookie());
     assert(cookie);

testing/btest/coverage/bare-load-baseline.test

@@ -9,7 +9,7 @@
 # below does. Don't ask. :-)
 
 # @TEST-REQUIRES: $SCRIPTS/have-spicy  # This test logs loaded scripts, so disable it if Spicy and it associated plugin is unavailable.
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # Enabling Spicy SSL changes the loaded scripts, skip in this case
 # @TEST-EXEC: zeek -b misc/loaded-scripts
 # @TEST-EXEC: test -e loaded_scripts.log
 # @TEST-EXEC: cat loaded_scripts.log | grep -E -v '#' | awk 'NR>0{print $1}' | sed -e ':a' -e '$!N' -e 's/^\(.*\).*\n\1.*/\1/' -e 'ta' >prefix

testing/btest/coverage/default-load-baseline.test

@@ -8,7 +8,7 @@
 # below does. Don't ask. :-)
 
 # @TEST-REQUIRES: ${SCRIPTS}/have-spicy
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # Enabling Spicy SSL changes the loaded scripts, skip in this case
 # @TEST-EXEC: zeek misc/loaded-scripts
 # @TEST-EXEC: test -e loaded_scripts.log
 # @TEST-EXEC: cat loaded_scripts.log | grep -E -v '#' | sed 's/ //g' | sed -e ':a' -e '$!N' -e 's/^\(.*\).*\n\1.*/\1/' -e 'ta' >prefix

testing/btest/plugins/hooks.zeek

@@ -1,6 +1,6 @@
 # @TEST-REQUIRES: test "${ZEEK_ZAM}" != "1"
 # @TEST-REQUIRES: ${SCRIPTS}/have-spicy  # This test logs loaded scripts, so disable it if Spicy and the associated plugin are unavailable.
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # Enabling Spicy SSL changes baselines and thus changes raised events. Skip in this case.
 # @TEST-EXEC: ${DIST}/auxil/zeek-aux/plugin-support/init-plugin -u . Demo Hooks
 # @TEST-EXEC: cp -r %DIR/hooks-plugin/* .
 # @TEST-EXEC: ./configure --zeek-dist=${DIST} && make

testing/btest/scripts/base/protocols/ssl/dtls-13.test

@@ -1,6 +1,6 @@
 # This tests a normal SSL connection and the log it outputs.
 
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # DTLS not supported in Spicy SSL
 # @TEST-EXEC: zeek -C -r $TRACES/tls/dtls13-wolfssl.pcap %INPUT
 # @TEST-EXEC: cp ssl.log ssl-all.log
 # @TEST-EXEC: echo "start CID test"

testing/btest/scripts/base/protocols/ssl/dtls-stun-dpd.test

@@ -1,4 +1,4 @@
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # DTLS is not supported in Spicy SSL yet
 # @TEST-EXEC: zeek -b -r $TRACES/tls/webrtc-stun.pcap %INPUT
 # @TEST-EXEC: btest-diff ssl.log
 # @TEST-EXEC: touch dpd.log

testing/btest/scripts/base/protocols/ssl/dtls.test

@@ -1,6 +1,6 @@
 # This tests a normal SSL connection and the log it outputs.
 
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # DTLS is not supported in Spicy SSL yet
 # @TEST-EXEC: zeek -b -r $TRACES/tls/dtls1_0.pcap %INPUT
 # @TEST-EXEC: btest-diff ssl.log
 # @TEST-EXEC: btest-diff x509.log

testing/btest/scripts/policy/protocols/ssl/decryption.zeek

@@ -1,5 +1,5 @@
 # @TEST-REQUIRES: grep -q "#define OPENSSL_HAVE_KDF_H" $BUILD/zeek-config.h
-# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h
+# @TEST-REQUIRES: ! grep -q "#define ENABLE_SPICY_SSL" $BUILD/zeek-config.h # Decryption is not supported in Spicy SSL
 
 # @TEST-EXEC: zeek -B dpd -C -r $TRACES/tls/tls12-decryption.pcap %INPUT
 # @TEST-EXEC: btest-diff http.log