From c960d279a212d9f07d50f8ade44a8a604132ed60 Mon Sep 17 00:00:00 2001
From: Arne Welzel <arne.welzel@corelight.com>
Date: Wed, 18 Oct 2023 18:43:33 +0200
Subject: [PATCH] ssl: Cap number of alerts parsed from SSL record

Limit the number of events raised from an SSL record with content_type
alert (21) to a configurable maximum number (default 10). For TLS 1.3,
the limit is set to 1 as specified in the RFC. Add a new weird cases
where the limit is exceeded.

OSS-Fuzz managed to generate a reproducer that raised ~660k ssl_plaintext
and ssl_alert events given ~810kb of input data. This change prevents this
with hopefully no negative side-effect in the real-world.
---
 NEWS                                          |  5 +++
 scripts/base/init-bare.zeek                   |  5 +++
 src/analyzer/protocol/ssl/consts.bif          |  1 +
 .../protocol/ssl/ssl-dtls-protocol.pac        | 36 ++++++++++++++++++-
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index ab95d420c0..0d3444d94f 100644
--- a/NEWS
+++ b/NEWS
@@ -187,6 +187,11 @@ Changed Functionality
 - The input framework now provides better information in error messages when
   encountering missing non-optional field while loading data.
 
+- The SSL analyzer will now parse a configurable maximum of 10 SSL Alerts per
+  SSL message. For TLS 1.3, the maximum is implicitly 1 as defined by RFC 8446.
+  If there are more alerts, a new weird "SSL_excessive_alerts_in_record" is raised.
+  For non-TLS 1.3, the maximum can be redefined via ``SSL::max_alerts_per_record``.
+
 Deprecated Functionality
 ------------------------
 
diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek
index c71faa7979..04b69762da 100644
--- a/scripts/base/init-bare.zeek
+++ b/scripts/base/init-bare.zeek
@@ -4490,6 +4490,11 @@ const SSL::dtls_max_version_errors = 10 &redef;
 ## Maximum number of invalid version errors to report in one DTLS connection.
 const SSL::dtls_max_reported_version_errors = 1 &redef;
 
+## Maximum number of Alert messages parsed from an SSL record with
+## content_type alert (21). The remaining alerts are discarded. For
+## TLS 1.3 connections, this is implicitly 1 as defined by RFC 8446.
+const SSL::max_alerts_per_record = 10 &redef;
+
 }
 
 module GLOBAL;
diff --git a/src/analyzer/protocol/ssl/consts.bif b/src/analyzer/protocol/ssl/consts.bif
index 9dcbaa65d5..405b2e2cab 100644
--- a/src/analyzer/protocol/ssl/consts.bif
+++ b/src/analyzer/protocol/ssl/consts.bif
@@ -1,2 +1,3 @@
 const SSL::dtls_max_version_errors: count;
 const SSL::dtls_max_reported_version_errors: count;
+const SSL::max_alerts_per_record: count;
diff --git a/src/analyzer/protocol/ssl/ssl-dtls-protocol.pac b/src/analyzer/protocol/ssl/ssl-dtls-protocol.pac
index 26bc760b29..4516f4084c 100644
--- a/src/analyzer/protocol/ssl/ssl-dtls-protocol.pac
+++ b/src/analyzer/protocol/ssl/ssl-dtls-protocol.pac
@@ -5,7 +5,7 @@
 
 type PlaintextRecord(rec: SSLRecord) = case rec.content_type of {
 	CHANGE_CIPHER_SPEC	-> ch_cipher : ChangeCipherSpec(rec);
-	ALERT			-> alert : Alert(rec);
+	ALERT			-> alerts : Alerts(rec);
 	HEARTBEAT -> heartbeat: Heartbeat(rec);
 	APPLICATION_DATA	-> app_data : ApplicationData(rec);
 	default			-> unknown_record : UnknownRecord(rec);
@@ -44,6 +44,11 @@ type ChangeCipherSpec(rec: SSLRecord) = record {
 # Alert Protocol (7.2.)
 ######################################################################
 
+type Alerts(rec: SSLRecord) = record {
+	alerts: Alert(rec)[] &length=$context.connection.cap_alert_messages_length(rec.length);
+	rest: bytestring &restofdata &transient;
+} &length=rec.length;
+
 type Alert(rec: SSLRecord) = record {
 	level : uint8;
 	description: uint8;
@@ -87,6 +92,10 @@ type CiphertextRecord(rec: SSLRecord) = record {
 # binpac analyzer for SSL including
 ######################################################################
 
+%extern{
+#include "zeek/analyzer/protocol/ssl/consts.bif.h"
+%}
+
 refine connection SSL_Conn += {
 
 	%member{
@@ -131,4 +140,29 @@ refine connection SSL_Conn += {
 
 		return 0;
 		%}
+
+	function cap_alert_messages_length(record_length: int) : int
+		%{
+		int alert_length = record_length;
+		int max_length = zeek::BifConst::SSL::max_alerts_per_record * 2;
+
+		// With TLS 1.3, enforce a single alert.
+		//
+		// From https://datatracker.ietf.org/doc/html/rfc8446//section-5.1
+		//
+		//    Alert messages (Section 6) MUST NOT be fragmented across records, and
+		//    multiple alert messages MUST NOT be coalesced into a single
+		//    record.  In other words, a record with an Alert type MUST contain
+		//    exactly one message.
+		if ( determine_tls13() )
+			max_length = 2;
+
+		if ( alert_length > max_length )
+			{
+			zeek_analyzer()->Weird("SSL_excessive_alerts_in_record", zeek::util::fmt("%d", alert_length / 2));
+			alert_length = max_length;
+			}
+
+		return alert_length;
+		%}
 };