From da4cd6a4f4dd5f6069e78a064cdfdae5c38e5665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 5 Dec 2020 11:47:57 +0100 Subject: [PATCH] Bug hunting; Improved buffer overflow check --- lib/bughuntingchecks.cpp | 91 +++++++++++++++++++++++++++-------- lib/exprengine.cpp | 23 +++++++++ lib/exprengine.h | 12 +++++ test/testbughuntingchecks.cpp | 33 +++++++++++++ 4 files changed, 139 insertions(+), 20 deletions(-) diff --git a/lib/bughuntingchecks.cpp b/lib/bughuntingchecks.cpp index 1619df626..0f9ca984e 100644 --- a/lib/bughuntingchecks.cpp +++ b/lib/bughuntingchecks.cpp @@ -37,6 +37,11 @@ static float getKnownFloatValue(const Token *tok, float def) return def; } +static bool isLessThan(ExprEngine::DataBase *dataBase, ExprEngine::ValuePtr lhs, ExprEngine::ValuePtr rhs) +{ + return ExprEngine::BinOpResult("<", lhs, rhs).isTrue(dataBase); +} + static void arrayIndex(const Token *tok, const ExprEngine::Value &value, ExprEngine::DataBase *dataBase) { if (!Token::simpleMatch(tok->astParent(), "[")) @@ -76,43 +81,89 @@ static void arrayIndex(const Token *tok, const ExprEngine::Value &value, ExprEng static void bufferOverflow(const Token *tok, const ExprEngine::Value &value, ExprEngine::DataBase *dataBase) { - if (!Token::simpleMatch(tok->astParent(), ",")) + if (value.type != ExprEngine::ValueType::FunctionCallArgumentValues) + return; + if (!Token::simpleMatch(tok, "(") || !Token::Match(tok->previous(), "%name% (")) return; - if (!tok->valueType() || tok->valueType()->pointer != 1 || tok->valueType()->type != ::ValueType::Type::CHAR) + const Library::Function *func = dataBase->settings->library.getFunction(tok->previous()); + if (!func) return; - int argnr = (tok == tok->astParent()->astOperand1()) ? 0 : 1; - const Token *ftok = tok->astParent(); - while (Token::simpleMatch(ftok, ",")) { - ++argnr; - ftok = ftok->astParent(); - } - ftok = ftok ? ftok->previous() : nullptr; - if (!Token::Match(ftok, "%name% (")) + const ExprEngine::FunctionCallArgumentValues *functionCallArguments = dynamic_cast(&value); + if (!functionCallArguments) + return; + + const std::vector arguments = getArguments(tok); + if (functionCallArguments->argValues.size() != arguments.size()) + // TODO investigate what to do return; int overflowArgument = 0; + bool bailout = false; - if (const Library::Function *func = dataBase->settings->library.getFunction(ftok)) { - for (auto argNrChecks: func->argumentChecks) { - int nr = argNrChecks.first; - const Library::ArgumentChecks &checks = argNrChecks.second; - for (const Library::ArgumentChecks::MinSize &minsize: checks.minsizes) { - if (minsize.type == Library::ArgumentChecks::MinSize::STRLEN && minsize.arg == argnr) - overflowArgument = nr; + for (auto argNrChecks: func->argumentChecks) { + const int argnr = argNrChecks.first; + if (argnr <= 0 || argnr > arguments.size()) + continue; + ExprEngine::ValuePtr argValue = functionCallArguments->argValues[argnr - 1]; + if (!argValue || argValue->type == ExprEngine::ValueType::BailoutValue) { + overflowArgument = argnr; + bailout = true; + break; + } + + std::shared_ptr arrayValue = std::dynamic_pointer_cast(argValue); + if (!arrayValue || arrayValue->size.size() != 1) // TODO : multidimensional array + continue; + + const Library::ArgumentChecks &checks = argNrChecks.second; + for (const Library::ArgumentChecks::MinSize &minsize: checks.minsizes) { + if (minsize.type == Library::ArgumentChecks::MinSize::ARGVALUE && minsize.arg > 0 && minsize.arg <= arguments.size()) { + ExprEngine::ValuePtr otherValue = functionCallArguments->argValues[minsize.arg - 1]; + if (!otherValue || otherValue->type == ExprEngine::ValueType::BailoutValue) { + overflowArgument = argnr; + bailout = true; + break; + } + if (isLessThan(dataBase, arrayValue->size[0], otherValue)) { + overflowArgument = argnr; + break; + } + } else if (minsize.type == Library::ArgumentChecks::MinSize::STRLEN && minsize.arg > 0 && minsize.arg <= arguments.size()) { + if (Token::Match(arguments[minsize.arg - 1], "%str%")) { + const Token * const str = arguments[minsize.arg - 1]; + if (arrayValue->size[0]->isLessThan(dataBase, Token::getStrLength(str))) { + overflowArgument = argnr; + break; + } + } else { + ExprEngine::ValuePtr otherValue = functionCallArguments->argValues[minsize.arg - 1]; + if (!otherValue || otherValue->type == ExprEngine::ValueType::BailoutValue) { + overflowArgument = argnr; + bailout = true; + break; + } + std::shared_ptr arrayValue2 = std::dynamic_pointer_cast(otherValue); + if (arrayValue2 && arrayValue2->size.size() == 1 && isLessThan(dataBase, arrayValue->size[0], arrayValue2->size[0])) { + overflowArgument = argnr; + break; + } + } } } + + if (overflowArgument > 0) + break; } - if (!overflowArgument) + if (overflowArgument == 0) return; - const bool bailout = (value.type == ExprEngine::ValueType::BailoutValue); dataBase->reportError(tok, Severity::SeverityType::error, "bughuntingBufferOverflow", - "Buffer read/write, when calling '" + ftok->str() + "' it cannot be determined that " + std::to_string(overflowArgument) + getOrdinalText(overflowArgument) + " argument is not overflowed", + "Buffer read/write, when calling '" + tok->previous()->str() + "' it cannot be determined that " + std::to_string(overflowArgument) + getOrdinalText(overflowArgument) + " argument is not overflowed", CWE(120), false, bailout); diff --git a/lib/exprengine.cpp b/lib/exprengine.cpp index 412a81eef..67e208913 100644 --- a/lib/exprengine.cpp +++ b/lib/exprengine.cpp @@ -1421,6 +1421,27 @@ bool ExprEngine::BinOpResult::isLessThan(ExprEngine::DataBase *dataBase, int val #endif } +bool ExprEngine::BinOpResult::isTrue(ExprEngine::DataBase *dataBase) const +{ +#ifdef USE_Z3 + try { + ExprData exprData; + z3::solver solver(exprData.context); + z3::expr e = exprData.getExpr(this); + for (auto constraint : dynamic_cast(dataBase)->constraints) + solver.add(exprData.getConstraintExpr(constraint)); + exprData.addAssertions(solver); + return solver.check() == z3::sat; + } catch (const z3::exception &exception) { + std::cerr << "z3:" << exception << std::endl; + return true; // Safe option is to return true + } +#else + (void)dataBase; + return false; +#endif +} + std::string ExprEngine::BinOpResult::getExpr(ExprEngine::DataBase *dataBase) const { #ifdef USE_Z3 @@ -1852,6 +1873,8 @@ static ExprEngine::ValuePtr executeFunctionCall(const Token *tok, Data &data) } } + call(data.callbacks, tok, std::make_shared(argValues), &data); + if (tok->astOperand1()->function()) { const Function *function = tok->astOperand1()->function(); const std::string &functionName = function->fullName(); diff --git a/lib/exprengine.h b/lib/exprengine.h index 889140fd8..d14c0f5ba 100644 --- a/lib/exprengine.h +++ b/lib/exprengine.h @@ -64,6 +64,7 @@ namespace ExprEngine { AddressOfValue, BinOpResult, IntegerTruncation, + FunctionCallArgumentValues, BailoutValue }; @@ -281,6 +282,7 @@ namespace ExprEngine { bool isEqual(DataBase *dataBase, int value) const OVERRIDE; bool isGreaterThan(DataBase *dataBase, int value) const OVERRIDE; virtual bool isLessThan(DataBase *dataBase, int value) const OVERRIDE; + bool isTrue(DataBase *dataBase) const; std::string getExpr(DataBase *dataBase) const; @@ -311,6 +313,16 @@ namespace ExprEngine { char sign; }; + class FunctionCallArgumentValues: public Value { + public: + FunctionCallArgumentValues(const std::vector &argValues) + : Value("argValues", ValueType::FunctionCallArgumentValues) + , argValues(argValues) + {} + + const std::vector argValues; + }; + class BailoutValue : public Value { public: BailoutValue() : Value("bailout", ValueType::BailoutValue) {} diff --git a/test/testbughuntingchecks.cpp b/test/testbughuntingchecks.cpp index 877e7b679..bcc898521 100644 --- a/test/testbughuntingchecks.cpp +++ b/test/testbughuntingchecks.cpp @@ -37,6 +37,10 @@ private: LOAD_LIB_2(settings.library, "std.cfg"); TEST_CASE(checkAssignment); TEST_CASE(arrayIndexOutOfBounds1); + TEST_CASE(bufferOverflowMemCmp1); + TEST_CASE(bufferOverflowMemCmp2); + TEST_CASE(bufferOverflowStrcpy1); + TEST_CASE(bufferOverflowStrcpy2); TEST_CASE(uninit); TEST_CASE(uninit_array); TEST_CASE(uninit_function_par); @@ -78,6 +82,35 @@ private: errout.str()); } + void bufferOverflowMemCmp1() { + // CVE-2020-24265 + check("void foo(const char *pktdata, int datalen) {\n" + " if (memcmp(pktdata, \"MGC\", 3)) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Buffer read/write, when calling 'memcmp' it cannot be determined that 1st argument is not overflowed\n", errout.str()); + } + + void bufferOverflowMemCmp2() { + check("void foo(const char *pktdata, unsigned int datalen) {\n" + " if (memcmp(pktdata, \"MGC\", datalen)) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Buffer read/write, when calling 'memcmp' it cannot be determined that 1st argument is not overflowed\n", errout.str()); + } + + void bufferOverflowStrcpy1() { + check("void foo(char *p) {\n" + " strcpy(p, \"hello\");\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Buffer read/write, when calling 'strcpy' it cannot be determined that 1st argument is not overflowed\n", errout.str()); + } + + void bufferOverflowStrcpy2() { + check("void foo(char *p, const char *q) {\n" + " strcpy(p, q);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Buffer read/write, when calling 'strcpy' it cannot be determined that 1st argument is not overflowed\n", errout.str()); + } + void uninit() { check("void foo() { int x; x = x + 1; }"); ASSERT_EQUALS("[test.cpp:1]: (error) Cannot determine that 'x' is initialized\n", errout.str());