From 529668670a56b9aeb54916fb0cd4fb0ac11a50c0 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 25 Nov 2014 14:57:10 -0800 Subject: [PATCH 1/2] make the SSL analyzer skip further processing once encountering situations which are very probably non-recoverable. Current behavior could lead to us jumping in in the middle of an old 443 stream and interpreting some data as ssl before failing again. --- src/analyzer/protocol/ssl/ssl-analyzer.pac | 19 +++++++++++++++---- src/analyzer/protocol/ssl/ssl-protocol.pac | 5 +++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/analyzer/protocol/ssl/ssl-analyzer.pac b/src/analyzer/protocol/ssl/ssl-analyzer.pac index 64d5d78df6..e74236b923 100644 --- a/src/analyzer/protocol/ssl/ssl-analyzer.pac +++ b/src/analyzer/protocol/ssl/ssl-analyzer.pac @@ -112,7 +112,10 @@ refine connection SSL_Conn += { cipher_suites24 : uint24[]) : bool %{ if ( ! version_ok(version) ) + { bro_analyzer()->ProtocolViolation(fmt("unsupported client SSL version 0x%04x", version)); + bro_analyzer()->SetSkip(true); + } else bro_analyzer()->ProtocolConfirmation(); @@ -152,7 +155,10 @@ refine connection SSL_Conn += { comp_method : uint8) : bool %{ if ( ! version_ok(version) ) + { bro_analyzer()->ProtocolViolation(fmt("unsupported server SSL version 0x%04x", version)); + bro_analyzer()->SetSkip(true); + } if ( ssl_server_hello ) { @@ -202,6 +208,7 @@ refine connection SSL_Conn += { // This should be impossible due to the binpac parser // and protocol description bro_analyzer()->ProtocolViolation(fmt("Impossible extension length: %lu", length)); + bro_analyzer()->SetSkip(true); return true; } @@ -392,7 +399,11 @@ refine connection SSL_Conn += { function proc_check_v2_server_hello_version(version: uint16) : bool %{ if ( version != SSLv20 ) + { bro_analyzer()->ProtocolViolation(fmt("Invalid version in SSL server hello. Version: %d", version)); + bro_analyzer()->SetSkip(true); + return false; + } return true; %} @@ -479,13 +490,13 @@ refine typeattr ServerHello += &let { }; refine typeattr V2ServerHello += &let { - proc : bool = $context.connection.proc_server_hello(rec, server_version, 0, - conn_id_data, 0, 0, ciphers, 0); - check_v2 : bool = $context.connection.proc_check_v2_server_hello_version(server_version); + proc : bool = $context.connection.proc_server_hello(rec, server_version, 0, + conn_id_data, 0, 0, ciphers, 0) &if(check_v2 == true); + cert : bool = $context.connection.proc_v2_certificate(rec, cert_data) - &requires(proc); + &requires(proc) &if(check_v2 == true); }; refine typeattr Certificate += &let { diff --git a/src/analyzer/protocol/ssl/ssl-protocol.pac b/src/analyzer/protocol/ssl/ssl-protocol.pac index 8e7f7a221d..9038039ef9 100644 --- a/src/analyzer/protocol/ssl/ssl-protocol.pac +++ b/src/analyzer/protocol/ssl/ssl-protocol.pac @@ -759,6 +759,7 @@ refine connection SSL_Conn += { version != TLSv11 && version != TLSv12 ) { bro_analyzer()->ProtocolViolation(fmt("Invalid version late in TLS connection. Packet reported version: %d", version)); + bro_analyzer()->SetSkip(true); return UNKNOWN_VERSION; } } @@ -775,6 +776,7 @@ refine connection SSL_Conn += { version != TLSv11 && version != TLSv12 ) { bro_analyzer()->ProtocolViolation(fmt("Invalid version in SSL client hello. Version: %d", version)); + bro_analyzer()->SetSkip(true); return UNKNOWN_VERSION; } @@ -791,6 +793,7 @@ refine connection SSL_Conn += { else // this is not SSL or TLS. { bro_analyzer()->ProtocolViolation(fmt("Invalid headers in SSL connection. Head1: %d, head2: %d, head3: %d", head1, head2, head3)); + bro_analyzer()->SetSkip(true); return UNKNOWN_VERSION; } } @@ -800,6 +803,7 @@ refine connection SSL_Conn += { version != TLSv11 && version != TLSv12 ) { bro_analyzer()->ProtocolViolation(fmt("Invalid version in TLS connection. Version: %d", version)); + bro_analyzer()->SetSkip(true); return UNKNOWN_VERSION; } @@ -810,6 +814,7 @@ refine connection SSL_Conn += { } bro_analyzer()->ProtocolViolation(fmt("Invalid type in TLS connection. Version: %d, Type: %d", version, head0)); + bro_analyzer()->SetSkip(true); return UNKNOWN_VERSION; %} From d87476b4030fc3f4816a84213c318340bb854f65 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 25 Nov 2014 15:01:12 -0800 Subject: [PATCH 2/2] and just to be safe - also require the &if check in binpac --- src/analyzer/protocol/ssl/ssl-analyzer.pac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/analyzer/protocol/ssl/ssl-analyzer.pac b/src/analyzer/protocol/ssl/ssl-analyzer.pac index e74236b923..2433886d14 100644 --- a/src/analyzer/protocol/ssl/ssl-analyzer.pac +++ b/src/analyzer/protocol/ssl/ssl-analyzer.pac @@ -493,10 +493,10 @@ refine typeattr V2ServerHello += &let { check_v2 : bool = $context.connection.proc_check_v2_server_hello_version(server_version); proc : bool = $context.connection.proc_server_hello(rec, server_version, 0, - conn_id_data, 0, 0, ciphers, 0) &if(check_v2 == true); + conn_id_data, 0, 0, ciphers, 0) &requires(check_v2) &if(check_v2 == true); cert : bool = $context.connection.proc_v2_certificate(rec, cert_data) - &requires(proc) &if(check_v2 == true); + &requires(proc) &requires(check_v2) &if(check_v2 == true); }; refine typeattr Certificate += &let {