From 67e8b99c2c52956c66895c4b61b9a2f2e037125e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 12 Mar 2019 21:15:26 +0100 Subject: [PATCH] CheckBufferOverrun: Readd a check for strncpy/memcpy/etc --- lib/checkbufferoverrun.cpp | 71 ++++++++++++++++++++++++++++++++++++++ lib/checkbufferoverrun.h | 9 ++++- test/testbufferoverrun.cpp | 14 ++++---- 3 files changed, 86 insertions(+), 8 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 7afb8b9fe..6f82bcd47 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -463,3 +463,74 @@ void CheckBufferOverrun::arrayIndexThenCheckError(const Token *tok, const std::s "Reorder conditions such as '(a[i] && i < 10)' to '(i < 10 && a[i])'. That way the array will " "not be accessed if the index is out of limits.", CWE398, false); } + +//--------------------------------------------------------------------------- + +void CheckBufferOverrun::stringNotZeroTerminated() +{ + // this is currently 'inconclusive'. See TestBufferOverrun::terminateStrncpy3 + if (!mSettings->isEnabled(Settings::WARNING) || !mSettings->inconclusive) + return; + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope * const scope : symbolDatabase->functionScopes) { + for (const Token *tok = scope->bodyStart; tok && tok != scope->bodyEnd; tok = tok->next()) { + if (!Token::Match(tok, "strncpy|memcpy (")) + continue; + const std::vector args = getArguments(tok); + if (args.size() != 3) + continue; + const Token *sizeToken = args[2]; + if (!sizeToken->hasKnownIntValue()) + continue; + const size_t bufferSize = getBufferSize(args[0]); + if (bufferSize == 0 || sizeToken->getKnownIntValue() < bufferSize) + continue; + const Token *srcValue = args[1]->getValueTokenMaxStrLength(); + if (srcValue && Token::getStrLength(srcValue) < sizeToken->getKnownIntValue()) + continue; + // Is the buffer zero terminated after the call? + bool isZeroTerminated = false; + for (const Token *tok2 = tok->next()->link(); tok2 != scope->bodyEnd; tok2 = tok2->next()) { + if (!Token::Match(tok2, "] =")) + continue; + const Token *rhs = tok2->next()->astOperand2(); + if (!rhs || !rhs->hasKnownIntValue() || rhs->getKnownIntValue() != 0) + continue; + if (isSameExpression(mTokenizer->isCPP(), false, args[0], tok2->link()->astOperand1(), mSettings->library, false, false)) + isZeroTerminated = true; + } + if (isZeroTerminated) + continue; + // TODO: Locate unsafe string usage.. + if (tok->str() == "strncpy") + terminateStrncpyError(tok, args[0]->expressionString()); + else + bufferNotZeroTerminatedError(tok, args[0]->expressionString(), tok->str()); + } + } +} + +void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::string &varname) +{ + const std::string shortMessage = "The buffer '$symbol' may not be null-terminated after the call to strncpy()."; + reportError(tok, Severity::warning, "terminateStrncpy", + "$symbol:" + varname + '\n' + + shortMessage + '\n' + + shortMessage + ' ' + + "If the source string's size fits or exceeds the given size, strncpy() does not add a " + "zero at the end of the buffer. This causes bugs later in the code if the code " + "assumes buffer is null-terminated.", CWE170, true); +} + +void CheckBufferOverrun::bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function) +{ + const std::string errmsg = "$symbol:" + varname + '\n' + + "$symbol:" + function + '\n' + + "The buffer '" + varname + "' is not null-terminated after the call to " + function + "().\n" + "The buffer '" + varname + "' is not null-terminated after the call to " + function + "(). " + "This will cause bugs later in the code if the code assumes the buffer is null-terminated."; + + reportError(tok, Severity::warning, "bufferNotZeroTerminated", errmsg, CWE170, true); +} + + diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 0e4997506..5fad53add 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -64,6 +64,7 @@ public: checkBufferOverrun.arrayIndex(); checkBufferOverrun.bufferOverflow(); checkBufferOverrun.arrayIndexThenCheck(); + checkBufferOverrun.stringNotZeroTerminated(); } void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) OVERRIDE { @@ -91,6 +92,11 @@ private: void arrayIndexThenCheck(); void arrayIndexThenCheckError(const Token *tok, const std::string &indexName); + void stringNotZeroTerminated(); + void terminateStrncpyError(const Token *tok, const std::string &varname); + void bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function); + + size_t getBufferSize(const Token *bufTok) const; static std::string myName() { @@ -102,7 +108,8 @@ private: "- Array index out of bounds\n" "- Buffer overflow\n" "- Dangerous usage of strncat()\n" - "- Using array index before checking it\n"; + "- Using array index before checking it\n" + "- Partial string write that leads to buffer that is not zero terminated.\n"; } }; /// @} diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 93b8d4b5f..57464c013 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -218,11 +218,11 @@ private: TEST_CASE(minsize_mul); TEST_CASE(unknownType); - // TODO strncpy TEST_CASE(terminateStrncpy1); - // TODO strncpy TEST_CASE(terminateStrncpy2); - // TODO strncpy TEST_CASE(terminateStrncpy3); - // TODO strncpy TEST_CASE(terminateStrncpy4); - // TODO strncpy TEST_CASE(recursive_long_time); + TEST_CASE(terminateStrncpy1); + TEST_CASE(terminateStrncpy2); + TEST_CASE(terminateStrncpy3); + TEST_CASE(terminateStrncpy4); + TEST_CASE(recursive_long_time); TEST_CASE(crash1); // Ticket #1587 - crash TEST_CASE(crash2); // Ticket #3034 - crash @@ -3543,7 +3543,7 @@ private: " strncpy(baz, bar, 100);\n" " baz[99] = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'baz' may not be null-terminated after the call to strncpy().\n", errout.str()); + ASSERT_EQUALS("", errout.str()); check("void foo ( char *bar ) {\n" " char baz[100];\n" @@ -3610,7 +3610,7 @@ private: " char buf[4];\n" " strncpy(buf, \"abcde\", 4);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'buf' is not null-terminated after the call to strncpy().\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) The buffer 'buf' may not be null-terminated after the call to strncpy().\n", errout.str()); } void recursive_long_time() {