Skip to content

Commit

Permalink
Fix potential stack overflow in NVT analyzer
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jsiwek committed Jun 2, 2020
1 parent e1f35c4 commit 3b51d72
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 13 deletions.
35 changes: 23 additions & 12 deletions src/analyzer/protocol/login/NVT.cc
Expand Up @@ -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 )
Expand Down Expand Up @@ -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:
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion src/analyzer/protocol/login/NVT.h
Expand Up @@ -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);
Expand Down

0 comments on commit 3b51d72

Please sign in to comment.