diff --git a/CHANGES b/CHANGES index b5a52dbbd8..54419ebb8c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +6.0.0-dev.344 | 2023-04-11 15:23:42 -0700 + + * RDP: Instantiate SSL analyzer instead of PIA (Tim Wojtulewicz, Corelight) + + * RDP: add some enforcement to required values based on MS-RDPBCGR docs (Tim Wojtulewicz, Corelight) + 6.0.0-dev.341 | 2023-04-11 15:16:39 -0700 * Stop skipping expiration of empty DNS mappings (Tim Wojtulewicz, Corelight) diff --git a/VERSION b/VERSION index 3d8894ab11..727bbd8994 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.0-dev.341 +6.0.0-dev.344 diff --git a/src/analyzer/protocol/rdp/RDP.cc b/src/analyzer/protocol/rdp/RDP.cc index b50006e6ca..77288947c7 100644 --- a/src/analyzer/protocol/rdp/RDP.cc +++ b/src/analyzer/protocol/rdp/RDP.cc @@ -13,7 +13,7 @@ RDP_Analyzer::RDP_Analyzer(Connection* c) : analyzer::tcp::TCP_ApplicationAnalyz interp = new binpac::RDP::RDP_Conn(this); had_gap = false; - pia = nullptr; + ssl = nullptr; } RDP_Analyzer::~RDP_Analyzer() @@ -54,19 +54,15 @@ void RDP_Analyzer::DeliverStream(int len, const u_char* data, bool orig) // 0x03-0x04 is CredSSP which is effectively SSL/TLS if ( interp->encryption_method() > 0x00 ) { - if ( ! pia ) + if ( ! ssl ) { - pia = new analyzer::pia::PIA_TCP(Conn()); - - if ( ! AddChildAnalyzer(pia) ) + ssl = new analyzer::ssl::SSL_Analyzer(Conn()); + if ( ! AddChildAnalyzer(ssl) ) { reporter->AnalyzerError(this, "failed to add TCP child analyzer " "to RDP analyzer: already exists"); return; } - - pia->FirstPacket(true, nullptr); - pia->FirstPacket(false, nullptr); } ForwardStream(len, data, orig); diff --git a/src/analyzer/protocol/rdp/RDP.h b/src/analyzer/protocol/rdp/RDP.h index bb84b7e775..816d10720b 100644 --- a/src/analyzer/protocol/rdp/RDP.h +++ b/src/analyzer/protocol/rdp/RDP.h @@ -3,6 +3,7 @@ #include "zeek/analyzer/protocol/pia/PIA.h" #include "zeek/analyzer/protocol/rdp/events.bif.h" #include "zeek/analyzer/protocol/rdp/rdp_pac.h" +#include "zeek/analyzer/protocol/ssl/SSL.h" #include "zeek/analyzer/protocol/tcp/TCP.h" namespace zeek::analyzer::rdp @@ -30,7 +31,7 @@ protected: binpac::RDP::RDP_Conn* interp; bool had_gap; - analyzer::pia::PIA_TCP* pia; + analyzer::ssl::SSL_Analyzer* ssl; }; } // namespace zeek::analyzer::rdp diff --git a/src/analyzer/protocol/rdp/rdp-protocol.pac b/src/analyzer/protocol/rdp/rdp-protocol.pac index e09e54e3ad..3e2f6fde16 100644 --- a/src/analyzer/protocol/rdp/rdp-protocol.pac +++ b/src/analyzer/protocol/rdp/rdp-protocol.pac @@ -92,7 +92,7 @@ type Connect_Request(cotp: COTP) = record { type RDP_Negotiation_Request = record { type: uint8; flags: uint8; - length: uint16; # must be set to 8 + length: uint16 &enforce(length == 8); # must be set to 8 requested_protocols: uint32; } &let { PROTOCOL_RDP: bool = requested_protocols & 0x00; @@ -127,7 +127,7 @@ type Connect_Confirm_Record = record { type RDP_Negotiation_Response = record { flags: uint8; - length: uint16; # must be set to 8 + length: uint16 &enforce(length==8); # must be set to 8 selected_protocol: uint32; } &let { # Seems to be SSL encrypted (maybe CredSSP also?) @@ -310,10 +310,12 @@ type Server_Network_Data = record { channel_count: uint16; } &byteorder=littleendian; +# See MS-RDPBCGR Section 2.2.1.4.3 for reasoning for the &enforce value on +# the server_random_length field. type Server_Security_Data = record { encryption_method: uint32; encryption_level: uint32; - server_random_length: uint32; + server_random_length: uint32 &enforce((encryption_level == 0 && encryption_method == 0 && server_random_length == 0) || (server_random_length == 0x20)); server_cert_length: uint32; server_random: bytestring &length=server_random_length; server_certificate: Server_Certificate &length=server_cert_length; diff --git a/testing/btest/Baseline/scripts.base.protocols.rdp.rdp-invalid-length/analyzer.log b/testing/btest/Baseline/scripts.base.protocols.rdp.rdp-invalid-length/analyzer.log new file mode 100644 index 0000000000..8726e1c31c --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.rdp.rdp-invalid-length/analyzer.log @@ -0,0 +1,12 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path analyzer +#open XXXX-XX-XX-XX-XX-XX +#fields ts cause analyzer_kind analyzer_name uid fuid id.orig_h id.orig_p id.resp_h id.resp_p failure_reason failure_data +#types time string string string string string addr port addr port string string +XXXXXXXXXX.XXXXXX violation protocol RDP CHhAvVGS1DHFjwGM9 - 10.0.0.1 45257 10.0.0.2 3389 Binpac exception: binpac exception: &enforce violation : RDP_Negotiation_Response:length - +XXXXXXXXXX.XXXXXX violation protocol RDP CHhAvVGS1DHFjwGM9 - 10.0.0.1 45257 10.0.0.2 3389 Binpac exception: binpac exception: invalid index for case: Connect_Confirm_Record: 49 - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/rdp/rdp-invalid-length.pcap b/testing/btest/Traces/rdp/rdp-invalid-length.pcap new file mode 100644 index 0000000000..3698412438 Binary files /dev/null and b/testing/btest/Traces/rdp/rdp-invalid-length.pcap differ diff --git a/testing/btest/scripts/base/protocols/rdp/rdp-invalid-length.zeek b/testing/btest/scripts/base/protocols/rdp/rdp-invalid-length.zeek new file mode 100644 index 0000000000..84f6dfe33d --- /dev/null +++ b/testing/btest/scripts/base/protocols/rdp/rdp-invalid-length.zeek @@ -0,0 +1,8 @@ +# Tests a pcap that has a known-invalid length in a RDP_Negotiation_Response +# header, ensuring that it throws a binpac exception and reports a notice to +# analyzer.log. The pcap used is a snippet of a pcap from OSS-Fuzz #57109. + +# @TEST-EXEC: zeek -C -b -r $TRACES/rdp/rdp-invalid-length.pcap %INPUT +# @TEST-EXEC: btest-diff analyzer.log + +@load base/protocols/rdp \ No newline at end of file