From 3b51d72aa165fd7e4baac260b7cb8103a0a02822 Mon Sep 17 00:00:00 2001
From: Jon Siwek <jsiwek@corelight.com>
Date: Mon, 1 Jun 2020 18:24:11 -0700
Subject: [PATCH] Fix potential stack overflow in NVT analyzer

The NVT_Analyzer (e.g. as instantiated to support the FTP analyzer)
uses a recursive parsing function that may only advance one byte at a
time and can easily cause a stack overflow as a result.  This change
replaces the recursive calls with equivalent iterative logic.

Credit to OSS-Fuzz for discovery
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22898
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22972
---
 src/analyzer/protocol/login/NVT.cc | 35 ++++++++++++++++++++----------
 src/analyzer/protocol/login/NVT.h  |  3 ++-
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/src/analyzer/protocol/login/NVT.cc b/src/analyzer/protocol/login/NVT.cc
index 7923eeabab9..230c09ba169 100644
--- a/src/analyzer/protocol/login/NVT.cc
+++ b/src/analyzer/protocol/login/NVT.cc
@@ -476,15 +476,21 @@ void NVT_Analyzer::SetEncrypting(int mode)
 #define MAX_DELIVER_UNIT 128
 
 void NVT_Analyzer::DoDeliver(int len, const u_char* data)
+	{
+	while ( len > 0 )
+		{
+		if ( pending_IAC )
+			ScanOption(len, data);
+		else
+			DeliverChunk(len, data);
+		}
+	}
+
+void NVT_Analyzer::DeliverChunk(int& len, const u_char*& data)
 	{
 	// This code is very similar to that for TCP_ContentLine.  We
 	// don't virtualize out the differences because some of them
 	// would require per-character function calls, too expensive.
-	if ( pending_IAC )
-		{
-		ScanOption(seq, len, data);
-		return;
-		}
 
 	// Add data up to IAC or end.
 	for ( ; len > 0; --len, ++data )
@@ -555,7 +561,9 @@ void NVT_Analyzer::DoDeliver(int len, const u_char* data)
 			IAC_pos = offset;
 			is_suboption = false;
 			buf[offset++] = c;
-			ScanOption(seq, len - 1, data + 1);
+			--len;
+			++data;
+			ScanOption(len, data);
 			return;
 
 		default:
@@ -574,7 +582,7 @@ void NVT_Analyzer::DoDeliver(int len, const u_char* data)
 		}
 	}
 
-void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
+void NVT_Analyzer::ScanOption(int& len, const u_char*& data)
 	{
 	if ( len <= 0 )
 		return;
@@ -622,8 +630,8 @@ void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
 			pending_IAC = false;
 			}
 
-		// Recurse to munch on the remainder.
-		DeliverStream(len - 1, data + 1, IsOrig());
+		--len;
+		++data;
 		return;
 		}
 
@@ -636,7 +644,8 @@ void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
 		offset -= 2;	// code + IAC
 		pending_IAC = false;
 
-		DeliverStream(len - 1, data + 1, IsOrig());
+		--len;
+		++data;
 		return;
 		}
 
@@ -676,14 +685,16 @@ void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
 			pending_IAC = is_suboption = false;
 
 			if ( code == TELNET_OPT_SE )
-				DeliverStream(len - 1, data + 1, IsOrig());
+				{
+				--len;
+				++data;
+				}
 			else
 				{
 				// Munch on the new (broken) option.
 				pending_IAC = true;
 				IAC_pos = offset;
 				buf[offset++] = TELNET_IAC;
-				DeliverStream(len, data, IsOrig());
 				}
 			return;
 			}
diff --git a/src/analyzer/protocol/login/NVT.h b/src/analyzer/protocol/login/NVT.h
index 4f8f05345a1..bd36b806f66 100644
--- a/src/analyzer/protocol/login/NVT.h
+++ b/src/analyzer/protocol/login/NVT.h
@@ -146,8 +146,9 @@ class NVT_Analyzer final : public tcp::ContentLine_Analyzer {
 
 protected:
 	void DoDeliver(int len, const u_char* data) override;
+	void DeliverChunk(int& len, const u_char*& data);
 
-	void ScanOption(int seq, int len, const u_char* data);
+	void ScanOption(int& len, const u_char*& data);
 	virtual void SawOption(unsigned int code);
 	virtual void SawOption(unsigned int code, unsigned int subcode);
 	virtual void SawSubOption(const char* opt, int len);