Merge remote-tracking branch 'security/topic/awelzel/200-pop-fuzzer-timeout'

* security/topic/awelzel/200-pop-fuzzer-timeout:
  ssl: Prevent unbounded ssl_history growth
  ssl: Cap number of alerts parsed from SSL record

NEWS

@@ -187,6 +187,16 @@ 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``.
+
+- The ``ssl_history`` field in the ssl.log is now capped at a configurable
+  limit of 100 characters prevent unbounded growth. The limit can be changed
+  via the option ``SSL::max_ssl_history_length``. When reached, a new weird
+  named "SSL_max_ssl_history_length_reached" is raised.
+
 Deprecated Functionality
 ------------------------
 

scripts/base/init-bare.zeek

@@ -4495,6 +4495,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;

scripts/base/protocols/ssl/main.zeek

@@ -143,6 +143,10 @@ export {
 	## (especially with large file transfers).
 	option disable_analyzer_after_detection = T;
 
+	## Maximum length of the ssl_history field to prevent unbounded
+	## growth when the parser is running into unexpected situations.
+	option max_ssl_history_length = 100;
+
 	## Delays an SSL record for a specific token: the record will not be
 	## logged as long as the token exists or until 15 seconds elapses.
 	global delay_log: function(info: Info, token: string);
@@ -208,10 +212,16 @@ function set_session(c: connection)
 
 function add_to_history(c: connection, is_client: bool, char: string)
 	{
+	if ( |c$ssl$ssl_history| == max_ssl_history_length )
+		return;
+
 	if ( is_client )
 		c$ssl$ssl_history = c$ssl$ssl_history+to_upper(char);
 	else
 		c$ssl$ssl_history = c$ssl$ssl_history+to_lower(char);
+
+	if ( |c$ssl$ssl_history| == max_ssl_history_length )
+		Reporter::conn_weird("SSL_max_ssl_history_length_reached", c);
 	}
 
 function delay_log(info: Info, token: string)

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;

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;
+		%}
 };

testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-3.log

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+CHhAvVGS1DHFjwGM9	Csx

testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-default.log

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+CHhAvVGS1DHFjwGM9	CsxknGIti

testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/weird-max-history-length-3.log

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+CHhAvVGS1DHFjwGM9	SSL_max_ssl_history_length_reached

testing/btest/scripts/base/protocols/ssl/max-history-length.test

@@ -0,0 +1,12 @@
+# Test max history length functionality
+
+# @TEST-EXEC: zeek -r $TRACES/tls/tls-conn-with-extensions.trace
+# @TEST-EXEC: zeek-cut uid ssl_history < ssl.log > ssl-max-history-length-default.log
+# @TEST-EXEC: btest-diff ssl-max-history-length-default.log
+# @TEST-EXEC: ! test -f weird.log
+#
+# @TEST-EXEC: zeek -r $TRACES/tls/tls-conn-with-extensions.trace SSL::max_ssl_history_length=3
+# @TEST-EXEC: zeek-cut uid ssl_history < ssl.log > ssl-max-history-length-3.log
+# @TEST-EXEC: zeek-cut uid name < weird.log > weird-max-history-length-3.log
+# @TEST-EXEC: btest-diff ssl-max-history-length-3.log
+# @TEST-EXEC: btest-diff weird-max-history-length-3.log