Merge remote-tracking branch 'security/topic/timw/154-rdp-timeout'

* security/topic/timw/154-rdp-timeout:
  RDP: Instantiate SSL analyzer instead of PIA
  RDP: add some enforcement to required values based on MS-RDPBCGR docs

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)

VERSION

@@ -1 +1 @@
-6.0.0-dev.341
+6.0.0-dev.344

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());
+				ssl = new analyzer::ssl::SSL_Analyzer(Conn());
-
+				if ( ! AddChildAnalyzer(ssl) )
-				if ( ! AddChildAnalyzer(pia) )
 					{
 					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);

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

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;

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

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