From c960d279a212d9f07d50f8ade44a8a604132ed60 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Wed, 18 Oct 2023 18:43:33 +0200 Subject: [PATCH 1/2] 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; + %} }; From 560f8a4a848007388446af10a09c15f109a18586 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Thu, 19 Oct 2023 16:32:12 +0200 Subject: [PATCH 2/2] ssl: Prevent unbounded ssl_history growth The ssl_history field may grow unbounded (e.g., ssl_alert event). Prevent this by capping using a configurable limit (default 100) and raise a weird once reached. --- NEWS | 5 +++++ scripts/base/protocols/ssl/main.zeek | 10 ++++++++++ .../ssl-max-history-length-3.log | 2 ++ .../ssl-max-history-length-default.log | 2 ++ .../weird-max-history-length-3.log | 2 ++ .../base/protocols/ssl/max-history-length.test | 12 ++++++++++++ 6 files changed, 33 insertions(+) create mode 100644 testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-3.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-default.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/weird-max-history-length-3.log create mode 100644 testing/btest/scripts/base/protocols/ssl/max-history-length.test diff --git a/NEWS b/NEWS index 0d3444d94f..ec55f2328f 100644 --- a/NEWS +++ b/NEWS @@ -192,6 +192,11 @@ Changed Functionality 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 ------------------------ diff --git a/scripts/base/protocols/ssl/main.zeek b/scripts/base/protocols/ssl/main.zeek index 5b71ebf8b7..eadc4e20fb 100644 --- a/scripts/base/protocols/ssl/main.zeek +++ b/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) diff --git a/testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-3.log b/testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-3.log new file mode 100644 index 0000000000..b96373319b --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-default.log b/testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/ssl-max-history-length-default.log new file mode 100644 index 0000000000..262141044d --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/weird-max-history-length-3.log b/testing/btest/Baseline/scripts.base.protocols.ssl.max-history-length/weird-max-history-length-3.log new file mode 100644 index 0000000000..155cffe1cc --- /dev/null +++ b/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 diff --git a/testing/btest/scripts/base/protocols/ssl/max-history-length.test b/testing/btest/scripts/base/protocols/ssl/max-history-length.test new file mode 100644 index 0000000000..41e2ac0889 --- /dev/null +++ b/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