From 069eedb736e4e50666373cc490339b04ccc84b1b Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 27 Jan 2020 17:17:36 -0800 Subject: [PATCH] Improve kerberos analyzer address and event handling Adds a weird, "invalid_kerberos_addr_len", for invalid kerberos host address lengths and also fixes a memory leak when processing KRB_KDC_REQ and KRB_KDC_REP messages for message types that do not match a known/expected type. --- src/analyzer/protocol/krb/krb-analyzer.pac | 75 +++++++++++++--------- src/analyzer/protocol/krb/krb-types.pac | 55 ++++++++++------ 2 files changed, 82 insertions(+), 48 deletions(-) diff --git a/src/analyzer/protocol/krb/krb-analyzer.pac b/src/analyzer/protocol/krb/krb-analyzer.pac index 85b80eeb08..e13c93fdde 100644 --- a/src/analyzer/protocol/krb/krb-analyzer.pac +++ b/src/analyzer/protocol/krb/krb-analyzer.pac @@ -73,7 +73,7 @@ RecordVal* proc_krb_kdc_req_arguments(KRB_KDC_REQ* msg, const BroAnalyzer bro_an break; case 9: if ( element->data()->addrs()->addresses()->size() ) - rv->Assign(12, proc_host_address_list(element->data()->addrs())); + rv->Assign(12, proc_host_address_list(bro_analyzer, element->data()->addrs())); break; case 10: @@ -172,19 +172,27 @@ refine connection KRB_Conn += { function proc_krb_kdc_req_msg(msg: KRB_KDC_REQ): bool %{ bro_analyzer()->ProtocolConfirmation(); - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 10 ) && ! krb_as_request ) - return false; + auto msg_type = binary_to_int64(${msg.msg_type.data.content}); - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 12 ) && ! krb_tgs_request ) - return false; + if ( msg_type == 10 ) + { + if ( ! krb_as_request ) + return false; - RecordVal* rv = proc_krb_kdc_req_arguments(${msg}, bro_analyzer()); - - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 10 ) ) + RecordVal* rv = proc_krb_kdc_req_arguments(${msg}, bro_analyzer()); BifEvent::generate_krb_as_request(bro_analyzer(), bro_analyzer()->Conn(), rv); + return true; + } - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 12 ) ) + if ( msg_type == 12 ) + { + if ( ! krb_tgs_request ) + return false; + + RecordVal* rv = proc_krb_kdc_req_arguments(${msg}, bro_analyzer()); BifEvent::generate_krb_tgs_request(bro_analyzer(), bro_analyzer()->Conn(), rv); + return true; + } return true; %} @@ -192,32 +200,41 @@ refine connection KRB_Conn += { function proc_krb_kdc_rep_msg(msg: KRB_KDC_REP): bool %{ bro_analyzer()->ProtocolConfirmation(); + auto msg_type = binary_to_int64(${msg.msg_type.data.content}); + auto make_arg = [this, msg]() -> RecordVal* + { + RecordVal* rv = new RecordVal(BifType::Record::KRB::KDC_Response); - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 11 ) && ! krb_as_response ) - return false; + rv->Assign(0, asn1_integer_to_val(${msg.pvno.data}, TYPE_COUNT)); + rv->Assign(1, asn1_integer_to_val(${msg.msg_type.data}, TYPE_COUNT)); - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 13 ) && ! krb_tgs_response ) - return false; + if ( ${msg.padata.has_padata} ) + rv->Assign(2, proc_padata(${msg.padata.padata.padata}, bro_analyzer(), false)); + rv->Assign(3, bytestring_to_val(${msg.client_realm.encoding.content})); + rv->Assign(4, GetStringFromPrincipalName(${msg.client_name})); - RecordVal* rv = new RecordVal(BifType::Record::KRB::KDC_Response); + rv->Assign(5, proc_ticket(${msg.ticket})); + return rv; + }; - rv->Assign(0, asn1_integer_to_val(${msg.pvno.data}, TYPE_COUNT)); - rv->Assign(1, asn1_integer_to_val(${msg.msg_type.data}, TYPE_COUNT)); + if ( msg_type == 11 ) + { + if ( ! krb_as_response ) + return false; - if ( ${msg.padata.has_padata} ) - rv->Assign(2, proc_padata(${msg.padata.padata.padata}, bro_analyzer(), false)); + BifEvent::generate_krb_as_response(bro_analyzer(), bro_analyzer()->Conn(), make_arg()); + return true; + } - rv->Assign(3, bytestring_to_val(${msg.client_realm.encoding.content})); - rv->Assign(4, GetStringFromPrincipalName(${msg.client_name})); + if ( msg_type == 13 ) + { + if ( ! krb_tgs_response ) + return false; - rv->Assign(5, proc_ticket(${msg.ticket})); - - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 11 ) ) - BifEvent::generate_krb_as_response(bro_analyzer(), bro_analyzer()->Conn(), rv); - - if ( ( binary_to_int64(${msg.msg_type.data.content}) == 13 ) ) - BifEvent::generate_krb_tgs_response(bro_analyzer(), bro_analyzer()->Conn(), rv); + BifEvent::generate_krb_tgs_response(bro_analyzer(), bro_analyzer()->Conn(), make_arg()); + return true; + } return true; %} @@ -309,10 +326,10 @@ refine connection KRB_Conn += { rv->Assign(5, asn1_integer_to_val(${msg.safe_body.args[i].args.seq_number}, TYPE_COUNT)); break; case 4: - rv->Assign(6, proc_host_address(${msg.safe_body.args[i].args.sender_addr})); + rv->Assign(6, proc_host_address(bro_analyzer(), ${msg.safe_body.args[i].args.sender_addr})); break; case 5: - rv->Assign(7, proc_host_address(${msg.safe_body.args[i].args.recp_addr})); + rv->Assign(7, proc_host_address(bro_analyzer(), ${msg.safe_body.args[i].args.recp_addr})); break; default: break; diff --git a/src/analyzer/protocol/krb/krb-types.pac b/src/analyzer/protocol/krb/krb-types.pac index 3b3b9d1f09..d7cbb17ae6 100644 --- a/src/analyzer/protocol/krb/krb-types.pac +++ b/src/analyzer/protocol/krb/krb-types.pac @@ -5,8 +5,8 @@ Val* GetStringFromPrincipalName(const KRB_Principal_Name* pname); VectorVal* proc_cipher_list(const Array* list); -VectorVal* proc_host_address_list(const KRB_Host_Addresses* list); -RecordVal* proc_host_address(const KRB_Host_Address* addr); +VectorVal* proc_host_address_list(const BroAnalyzer a, const KRB_Host_Addresses* list); +RecordVal* proc_host_address(const BroAnalyzer a, const KRB_Host_Address* addr); VectorVal* proc_tickets(const KRB_Ticket_Sequence* list); RecordVal* proc_ticket(const KRB_Ticket* ticket); @@ -33,45 +33,62 @@ VectorVal* proc_cipher_list(const Array* list) return ciphers; } -VectorVal* proc_host_address_list(const KRB_Host_Addresses* list) +VectorVal* proc_host_address_list(const BroAnalyzer a, const KRB_Host_Addresses* list) { VectorVal* addrs = new VectorVal(internal_type("KRB::Host_Address_Vector")->AsVectorType()); for ( uint i = 0; i < list->addresses()->size(); ++i ) { - addrs->Assign(addrs->Size(), proc_host_address((*list->addresses())[i])); + addrs->Assign(addrs->Size(), proc_host_address(a, (*list->addresses())[i])); } return addrs; } -RecordVal* proc_host_address(const KRB_Host_Address* addr) +RecordVal* proc_host_address(const BroAnalyzer a, const KRB_Host_Address* addr) { RecordVal* rv = new RecordVal(BifType::Record::KRB::Host_Address); + const auto& addr_bytes = addr->address()->data()->content(); switch ( binary_to_int64(addr->addr_type()->encoding()->content()) ) { case 2: - rv->Assign(0, new AddrVal(IPAddr(IPv4, - (const uint32_t*) c_str(addr->address()->data()->content()), - IPAddr::Network))); - break; + { + if ( addr_bytes.length() != 4 ) + { + a->Weird("invalid_kerberos_addr_len"); + break; + } + + auto bytes = reinterpret_cast(addr_bytes.data()); + rv->Assign(0, new AddrVal(IPAddr(IPv4, bytes, IPAddr::Network))); + return rv; + } case 24: - rv->Assign(0, new AddrVal(IPAddr(IPv6, - (const uint32_t*) c_str(addr->address()->data()->content()), - IPAddr::Network))); - break; + { + if ( addr_bytes.length() != 16 ) + { + a->Weird("invalid_kerberos_addr_len"); + break; + } + + auto bytes = reinterpret_cast(addr_bytes.data()); + rv->Assign(0, new AddrVal(IPAddr(IPv6, bytes, IPAddr::Network))); + return rv; + } case 20: - rv->Assign(1, bytestring_to_val(addr->address()->data()->content())); - break; + { + rv->Assign(1, bytestring_to_val(addr_bytes)); + return rv; + } default: - RecordVal* unk = new RecordVal(BifType::Record::KRB::Type_Value); - unk->Assign(0, asn1_integer_to_val(addr->addr_type(), TYPE_COUNT)); - unk->Assign(1, bytestring_to_val(addr->address()->data()->content())); - rv->Assign(2, unk); break; } + RecordVal* unk = new RecordVal(BifType::Record::KRB::Type_Value); + unk->Assign(0, asn1_integer_to_val(addr->addr_type(), TYPE_COUNT)); + unk->Assign(1, bytestring_to_val(addr_bytes)); + rv->Assign(2, unk); return rv; }