walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Bug hunting; Improved buffer overflow check

This commit is contained in:
Daniel Marjamäki 2020-12-05 11:47:57 +01:00
parent 396c353d3c
commit da4cd6a4f4
4 changed files with 139 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

91
lib/bughuntingchecks.cpp
View File

@ -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<const ExprEngine::FunctionCallArgumentValues *>(&value);
if (!functionCallArguments)
return;
const std::vector<const Token *> 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<ExprEngine::ArrayValue> arrayValue = std::dynamic_pointer_cast<ExprEngine::ArrayValue>(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<ExprEngine::ArrayValue> arrayValue2 = std::dynamic_pointer_cast<ExprEngine::ArrayValue>(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);

23
lib/exprengine.cpp
View File

@ -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<const Data *>(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<ExprEngine::FunctionCallArgumentValues>(argValues), &data);
if (tok->astOperand1()->function()) {
const Function *function = tok->astOperand1()->function();
const std::string &functionName = function->fullName();

12
lib/exprengine.h
View File

@ -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<ExprEngine::ValuePtr> &argValues)
: Value("argValues", ValueType::FunctionCallArgumentValues)
, argValues(argValues)
{}
const std::vector<ExprEngine::ValuePtr> argValues;
};
class BailoutValue : public Value {
public:
BailoutValue() : Value("bailout", ValueType::BailoutValue) {}

33
test/testbughuntingchecks.cpp
View File

@ -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());