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
This commit is contained in:
Jon Siwek 2020-06-01 18:24:11 -07:00
parent e1f35c46f9
commit 3b51d72aa1
2 changed files with 25 additions and 13 deletions

View file

@ -476,15 +476,21 @@ void NVT_Analyzer::SetEncrypting(int mode)
#define MAX_DELIVER_UNIT 128 #define MAX_DELIVER_UNIT 128
void NVT_Analyzer::DoDeliver(int len, const u_char* data) 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 // This code is very similar to that for TCP_ContentLine. We
// don't virtualize out the differences because some of them // don't virtualize out the differences because some of them
// would require per-character function calls, too expensive. // would require per-character function calls, too expensive.
if ( pending_IAC )
{
ScanOption(seq, len, data);
return;
}
// Add data up to IAC or end. // Add data up to IAC or end.
for ( ; len > 0; --len, ++data ) for ( ; len > 0; --len, ++data )
@ -555,7 +561,9 @@ void NVT_Analyzer::DoDeliver(int len, const u_char* data)
IAC_pos = offset; IAC_pos = offset;
is_suboption = false; is_suboption = false;
buf[offset++] = c; buf[offset++] = c;
ScanOption(seq, len - 1, data + 1); --len;
++data;
ScanOption(len, data);
return; return;
default: 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 ) if ( len <= 0 )
return; return;
@ -622,8 +630,8 @@ void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
pending_IAC = false; pending_IAC = false;
} }
// Recurse to munch on the remainder. --len;
DeliverStream(len - 1, data + 1, IsOrig()); ++data;
return; return;
} }
@ -636,7 +644,8 @@ void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
offset -= 2; // code + IAC offset -= 2; // code + IAC
pending_IAC = false; pending_IAC = false;
DeliverStream(len - 1, data + 1, IsOrig()); --len;
++data;
return; return;
} }
@ -676,14 +685,16 @@ void NVT_Analyzer::ScanOption(int seq, int len, const u_char* data)
pending_IAC = is_suboption = false; pending_IAC = is_suboption = false;
if ( code == TELNET_OPT_SE ) if ( code == TELNET_OPT_SE )
DeliverStream(len - 1, data + 1, IsOrig()); {
--len;
++data;
}
else else
{ {
// Munch on the new (broken) option. // Munch on the new (broken) option.
pending_IAC = true; pending_IAC = true;
IAC_pos = offset; IAC_pos = offset;
buf[offset++] = TELNET_IAC; buf[offset++] = TELNET_IAC;
DeliverStream(len, data, IsOrig());
} }
return; return;
} }

View file

@ -146,8 +146,9 @@ public:
protected: protected:
void DoDeliver(int len, const u_char* data) override; 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);
virtual void SawOption(unsigned int code, unsigned int subcode); virtual void SawOption(unsigned int code, unsigned int subcode);
virtual void SawSubOption(const char* opt, int len); virtual void SawSubOption(const char* opt, int len);