SMB: fix number of small issues.

Changes:
* change virtual to override where appropriate
* analyzer triggered assert in debug mode because it did not call Done()
  on manually instantiated analyzers.
* added a few length checks to methods
* commented unused code and removed a few unused class members

src/analyzer/protocol/dce-rpc/DCE_RPC.cc

@@ -37,7 +37,7 @@ void DCE_RPC_Analyzer::EndpointEOF(bool is_orig)
 	TCP_ApplicationAnalyzer::EndpointEOF(is_orig);
 	interp->FlowEOF(is_orig);
 	}
-	
+
 void DCE_RPC_Analyzer::Undelivered(uint64 seq, int len, bool orig)
 	{
 	TCP_ApplicationAnalyzer::Undelivered(seq, len, orig);
@@ -49,7 +49,7 @@ void DCE_RPC_Analyzer::DeliverStream(int len, const u_char* data, bool orig)
 	TCP_ApplicationAnalyzer::DeliverStream(len, data, orig);
 
 	assert(TCP());
-	try 
+	try
 		{
 		interp->NewData(orig, data, data + len);
 		}

src/analyzer/protocol/dce-rpc/DCE_RPC.h

@@ -16,7 +16,7 @@
 
 namespace analyzer { namespace dce_rpc {
 
-class UUID {
+/* class UUID {
 public:
 	UUID();
 	UUID(const u_char data[16]);
@@ -75,7 +75,7 @@ struct dce_rpc_endpoint_addr {
 
 		return string(buf);
 		}
-};
+}; */
 
 /*
 enum DCE_RPC_PTYPE {
@@ -180,10 +180,10 @@ public:
 	DCE_RPC_Analyzer(Connection* conn);
 	~DCE_RPC_Analyzer();
 
-	virtual void Done();
+	void Done() override;
-	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	void DeliverStream(int len, const u_char* data, bool orig) override;
-	virtual void Undelivered(uint64 seq, int len, bool orig);
+	void Undelivered(uint64 seq, int len, bool orig) override;
-	virtual void EndpointEOF(bool is_orig);
+	void EndpointEOF(bool is_orig) override;
 
 	bool SetFileID(uint64 fid_in)
 		{ interp->set_file_id(fid_in); return true; }
@@ -195,6 +195,6 @@ protected:
 	binpac::DCE_RPC::DCE_RPC_Conn* interp;
 };
 
-} } // namespace analyzer::* 
+} } // namespace analyzer::*
 
 #endif /* dce_rpc_h */

src/analyzer/protocol/dce-rpc/Plugin.cc

@@ -13,7 +13,6 @@ public:
 	plugin::Configuration Configure()
 		{
 		AddComponent(new ::analyzer::Component("DCE_RPC", ::analyzer::dce_rpc::DCE_RPC_Analyzer::Instantiate));
-		//AddComponent(new ::analyzer::Component("Contents_DCE_RPC", 0));
 
 		plugin::Configuration config;
 		config.name = "Bro::DCE_RPC";

src/analyzer/protocol/gssapi/GSSAPI.h

@@ -19,13 +19,13 @@ public:
 	virtual ~GSSAPI_Analyzer();
 
 	// Overriden from Analyzer.
-	virtual void Done();
+	void Done() override;
 
-	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	void DeliverStream(int len, const u_char* data, bool orig) override;
-	virtual void Undelivered(uint64 seq, int len, bool orig);
+	void Undelivered(uint64 seq, int len, bool orig) override;
 
 	// Overriden from tcp::TCP_ApplicationAnalyzer.
-	virtual void EndpointEOF(bool is_orig);
+	void EndpointEOF(bool is_orig) override;
 
 	static analyzer::Analyzer* Instantiate(Connection* conn)
 		{ return new GSSAPI_Analyzer(conn); }

src/analyzer/protocol/gssapi/gssapi-analyzer.pac

@@ -10,7 +10,10 @@ refine connection GSSAPI_Conn += {
 
 	%cleanup{
 		if ( ntlm )
+			{
+			ntlm->Done();
 			delete ntlm;
+			}
 	%}
 
 	function forward_ntlm(data: bytestring, is_orig: bool): bool

src/analyzer/protocol/netbios/NetbiosSSN.h

@@ -124,7 +124,7 @@ public:
 	NetbiosSSN_State State() const		{ return state; }
 
 protected:
-	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	void DeliverStream(int len, const u_char* data, bool orig) override;
 
 	NetbiosSSN_Interpreter* interp;
 
@@ -144,17 +144,17 @@ public:
 	NetbiosSSN_Analyzer(Connection* conn);
 	~NetbiosSSN_Analyzer();
 
-	virtual void Done();
+	void Done() override;
-	virtual void DeliverPacket(int len, const u_char* data, bool orig,
+	void DeliverPacket(int len, const u_char* data, bool orig,
-					uint64 seq, const IP_Hdr* ip, int caplen);
+					uint64 seq, const IP_Hdr* ip, int caplen) override;
 
 	static analyzer::Analyzer* Instantiate(Connection* conn)
 		{ return new NetbiosSSN_Analyzer(conn); }
 
 protected:
-	virtual void ConnectionClosed(tcp::TCP_Endpoint* endpoint,
+	void ConnectionClosed(tcp::TCP_Endpoint* endpoint,
-					tcp::TCP_Endpoint* peer, int gen_event);
+					tcp::TCP_Endpoint* peer, int gen_event) override;
-	virtual void EndpointEOF(bool is_orig);
+	void EndpointEOF(bool is_orig) override;
 
 	void ExpireTimer(double t);
 
@@ -168,6 +168,6 @@ protected:
 // FIXME: Doesn't really fit into new analyzer structure. What to do?
 int IsReuse(double t, const u_char* pkt);
 
-} } // namespace analyzer::* 
+} } // namespace analyzer::*
 
 #endif

src/analyzer/protocol/ntlm/NTLM.h

@@ -19,13 +19,13 @@ public:
 	virtual ~NTLM_Analyzer();
 
 	// Overriden from Analyzer.
-	virtual void Done();
+	void Done() override;
 
-	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	void DeliverStream(int len, const u_char* data, bool orig) override;
-	virtual void Undelivered(uint64 seq, int len, bool orig);
+	void Undelivered(uint64 seq, int len, bool orig) override;
 
 	// Overriden from tcp::TCP_ApplicationAnalyzer.
-	virtual void EndpointEOF(bool is_orig);
+	void EndpointEOF(bool is_orig) override;
 
 	static analyzer::Analyzer* Instantiate(Connection* conn)
 		{ return new NTLM_Analyzer(conn); }

src/analyzer/protocol/ntlm/ntlm-analyzer.pac

@@ -6,10 +6,10 @@ refine connection NTLM_Conn += {
 		%{
 		double secs = (ts / 10000000.0);
 
-		// Bro can't support times back to the 1600's 
+		// Bro can't support times back to the 1600's
 		// so we subtract a lot of seconds.
 		Val* bro_ts = new Val(secs - 11644473600.0, TYPE_TIME);
-		
+
 		return bro_ts;
 		%}
 

src/analyzer/protocol/smb/SMB.cc

@@ -33,7 +33,7 @@ void SMB_Analyzer::EndpointEOF(bool is_orig)
 	TCP_ApplicationAnalyzer::EndpointEOF(is_orig);
 	interp->FlowEOF(is_orig);
 	}
-	
+
 void SMB_Analyzer::Undelivered(uint64 seq, int len, bool orig)
 	{
 	TCP_ApplicationAnalyzer::Undelivered(seq, len, orig);
@@ -46,10 +46,10 @@ void SMB_Analyzer::DeliverStream(int len, const u_char* data, bool orig)
 
 	assert(TCP());
 
-	try 
+	try
 		{
 		interp->NewData(orig, data, data + len);
-		// Let's assume that if there are no binpac exceptions after 
+		// Let's assume that if there are no binpac exceptions after
 		// 3 data chunks that this is probably actually SMB.
 		if ( chunks >= 3 )
 			ProtocolConfirmation();
@@ -93,8 +93,11 @@ void Contents_SMB::Undelivered(uint64 seq, int len, bool orig)
 	NeedResync();
 	}
 
-bool Contents_SMB::HasSMBHeader(const u_char* data)
+bool Contents_SMB::HasSMBHeader(int len, const u_char* data)
 	{
+	if ( len < 8 )
+		return false;
+
 	return (strncmp((const char*) data+4, "\xffSMB", 4) == 0 ||
 	        strncmp((const char*) data+4, "\xfeSMB", 4) == 0);
 	}
@@ -102,12 +105,16 @@ bool Contents_SMB::HasSMBHeader(const u_char* data)
 void Contents_SMB::DeliverSMB(int len, const u_char* data)
 	{
 	// Check the 4-byte header.
-	if ( ! HasSMBHeader(data) )
+	if ( ! HasSMBHeader(len, data) )
 		{
-		Conn()->Weird(fmt("SMB-over-TCP header error: %02x %05x, >>\\x%02x%c%c%c<<",
+		if ( len >= 4 )
-			//dshdr[0], dshdr[1], dshdr[2], dshdr[3],
+			Conn()->Weird(fmt("SMB-over-TCP header error: %02x %05x, >>\\x%02x%c%c%c<<",
-			msg_type, msg_len,
+				//dshdr[0], dshdr[1], dshdr[2], dshdr[3],
-			data[0], data[1], data[2], data[3]));
+				msg_type, msg_len,
+				data[0], data[1], data[2], data[3]));
+		else
+			Conn()->Weird(fmt("SMB-over-TCP header error: %02x %05x", msg_type, msg_len));
+
 		NeedResync();
 		}
 	else
@@ -121,21 +128,21 @@ bool Contents_SMB::CheckResync(int& len, const u_char*& data, bool orig)
 	if (resync_state == INSYNC)
 		return true;
 
-	// This is an attempt to re-synchronize the stream after a content gap.  
+	// This is an attempt to re-synchronize the stream after a content gap.
-	// Returns true if we are in sync. 
+	// Returns true if we are in sync.
 	// Returns false otherwise (we are in resync mode)
 	//
-	// We try to look for the beginning of a SMB message, assuming 
+	// We try to look for the beginning of a SMB message, assuming
-	// SMB messages start at packet boundaries (though they may span 
+	// SMB messages start at packet boundaries (though they may span
 	// over multiple packets) (note that the data* of DeliverStream()
-	// usually starts at a packet boundrary). 
+	// usually starts at a packet boundrary).
 	//
 	// Now lets see whether data points to the beginning of a
 	// SMB message. If the resync processs is successful, we should
 	// be at the beginning of a frame.
 
 	// check if the SMB header starts with an SMB1 or SMB2 marker
-	if ( ! HasSMBHeader(data) )
+	if ( ! HasSMBHeader(len, data) )
 		{
 		NeedResync();
 		return false;
@@ -151,7 +158,7 @@ bool Contents_SMB::CheckResync(int& len, const u_char*& data, bool orig)
 void Contents_SMB::DeliverStream(int len, const u_char* data, bool orig)
 	{
 	TCP_SupportAnalyzer::DeliverStream(len, data, orig);
-	
+
 	if (!CheckResync(len, data, orig))
 		return;   // Not in sync yet. Still resyncing
 
@@ -178,7 +185,7 @@ void Contents_SMB::DeliverStream(int len, const u_char* data, bool orig)
 				const u_char *dummy_p = msg_buf.GetBuf();
 				int dummy_len = (int) msg_buf.GetFill();
 				DeliverSMB(dummy_len, dummy_p);
-				
+
 				state = WAIT_FOR_HDR;
 				}
 			}

src/analyzer/protocol/smb/SMB.h

@@ -15,7 +15,7 @@ public:
 	Contents_SMB(Connection* conn, bool orig);
 	~Contents_SMB();
 
-	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	void DeliverStream(int len, const u_char* data, bool orig) override;
 
 protected:
 	typedef enum {
@@ -26,20 +26,18 @@ protected:
 		NEED_RESYNC,
 		INSYNC,
 	} resync_state_t;
-	virtual void Init();
+	void Init() override;
 	virtual bool CheckResync(int& len, const u_char*& data, bool orig);
-	virtual void Undelivered(uint64 seq, int len, bool orig);
+	void Undelivered(uint64 seq, int len, bool orig) override;
 	virtual void NeedResync() {
 		resync_state = NEED_RESYNC;
 		state = WAIT_FOR_HDR;
 	}
 
-	bool HasSMBHeader(const u_char* data);
+	bool HasSMBHeader(int len, const u_char* data);
 
 	void DeliverSMB(int len, const u_char* data);
 
-	binpac::SMB::SMB_Conn* smb_session;
-
 	rpc::RPC_Reasm_Buffer hdr_buf; // Reassembles the NetBIOS length and glue.
 	rpc::RPC_Reasm_Buffer msg_buf; // Reassembles the SMB message.
 	int msg_len;
@@ -54,25 +52,23 @@ class SMB_Analyzer : public tcp::TCP_ApplicationAnalyzer {
 public:
 	SMB_Analyzer(Connection* conn);
 	virtual ~SMB_Analyzer();
-	
+
-	virtual void Done();
+	void Done() override;
-	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	void DeliverStream(int len, const u_char* data, bool orig) override;
-	virtual void Undelivered(uint64 seq, int len, bool orig);
+	void Undelivered(uint64 seq, int len, bool orig) override;
-	virtual void EndpointEOF(bool is_orig);
+	void EndpointEOF(bool is_orig) override;
 
 	static analyzer::Analyzer* Instantiate(Connection* conn)
 		{ return new SMB_Analyzer(conn); }
 
 protected:
 	binpac::SMB::SMB_Conn* interp;
-	Contents_SMB* o_smb;
-	Contents_SMB* r_smb;
 
 	// Count the number of chunks received by the analyzer
 	// but only used to count the first few.
 	uint8 chunks;
 };
 
-} } // namespace analyzer::* 
+} } // namespace analyzer::*
 
 #endif

src/analyzer/protocol/smb/pipe-mssql-tds.pac

@@ -64,6 +64,6 @@ type TDS_Token_EnvChange = record {
 };
 
 type TDS_Token_Info = record {
-	
+
 };
 

src/analyzer/protocol/smb/smb-gssapi.pac

@@ -10,7 +10,10 @@ refine connection SMB_Conn += {
 
 	%cleanup{
 		if ( gssapi )
+			{
+			gssapi->Done();
 			delete gssapi;
+			}
 	%}
 
 	function forward_gssapi(data: bytestring, is_orig: bool): bool

src/analyzer/protocol/smb/smb-pipe.pac

@@ -5,7 +5,7 @@
 refine connection SMB_Conn += {
 	%member{
 		map<uint16,bool> tree_is_pipe_map;
-		map<uint64,analyzer::dce_rpc::DCE_RPC_Analyzer*> fid_to_analyzer_map;
+		map<uint64,analyzer::dce_rpc::DCE_RPC_Analyzer*> fid_to_analyzer_map;;
 	%}
 
 	%cleanup{
@@ -13,7 +13,10 @@ refine connection SMB_Conn += {
 		for ( auto kv : fid_to_analyzer_map )
 			{
 			if ( kv.second )
+				{
+				kv.second->Done();
 				delete kv.second;
+				}
 			}
 	%}
 
@@ -50,4 +53,4 @@ refine connection SMB_Conn += {
 
 		return true;
 		%}
-};
+};

src/analyzer/protocol/smb/smb-strings.pac

@@ -1,11 +1,10 @@
-function uint8s_to_stringval(s: uint8[]): StringVal
+function uint8s_to_stringval(data: uint8[]): StringVal
 	%{
 	int length = 0;
 
 	const char* sp;
 	bool ascii = true;
 
-	vector<uint8>* data = s;
 	length = data->size();
 	// Scan the string once to see if it's all ascii
 	// embedded in UCS-2 (16 bit unicode).
@@ -21,7 +20,7 @@ function uint8s_to_stringval(s: uint8[]): StringVal
 
 	char *buf = new char[length];
 
-	for ( int i = 0; i < length; i=i+2)
+	for ( int i = 0; i + 1 < length; i=i+2) // check if we may read the character after the current one (else-case)
 		{
 		if ( ascii )
 			{