From a9d3245e807947ba535834cda50ad177b9ce55f0 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 23 Mar 2023 15:01:09 -0700 Subject: [PATCH 1/2] RDP: add some enforcement to required values based on MS-RDPBCGR docs --- src/analyzer/protocol/rdp/rdp-protocol.pac | 8 +++++--- .../analyzer.log | 12 ++++++++++++ testing/btest/Traces/rdp/rdp-invalid-length.pcap | Bin 0 -> 1065 bytes .../base/protocols/rdp/rdp-invalid-length.zeek | 8 ++++++++ 4 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.rdp.rdp-invalid-length/analyzer.log create mode 100644 testing/btest/Traces/rdp/rdp-invalid-length.pcap create mode 100644 testing/btest/scripts/base/protocols/rdp/rdp-invalid-length.zeek 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 0000000000000000000000000000000000000000..369841243810aae2d7a721430cb28a8cdcbeb77f GIT binary patch literal 1065 zcmcIiJugF17=F&VRIj8$4UCltT_oybDNQPMX(aSYh=oK^sfcu_F!>91Hj#+UjY(pV zFi-;qX}Ve1d~F@X$NQc$xVa3C#FM<|p7%NTJnz@JJv-cysKPe{1Ucn!2o1qiDYmfQRcE z3ODG;rs6M;0Rw2Py8(hdkhG`!62UH7{7R35Jg%EqBacnN9VE*P1=;ijX(3t#xw-(V zb&#IUNF>tzK_NbWgKb5l9-i|F>NnH3W1^Q_HU-z5MLRQbU*_5~=Od!iT Date: Fri, 24 Mar 2023 11:05:33 -0700 Subject: [PATCH 2/2] RDP: Instantiate SSL analyzer instead of PIA --- src/analyzer/protocol/rdp/RDP.cc | 12 ++++-------- src/analyzer/protocol/rdp/RDP.h | 3 ++- 2 files changed, 6 insertions(+), 9 deletions(-) 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