diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp new file mode 100644 index 0000000..fc60a6c --- /dev/null +++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp @@ -0,0 +1,27 @@ + + + +

+ Description of the vulnerability. +

+
+ +

+ Do XYZ. +

+
+ +

+ The following example shows ...: +

+ +
+ +
  • + XYZ: + XYZ +
  • +
    +
    diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql new file mode 100644 index 0000000..9e905a0 --- /dev/null +++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql @@ -0,0 +1,268 @@ +/** + * @name Inconsistent handling of return values from a specific function + * @description If a function's return value is used in `if` statements, + * and in a few statements the value is compared somehow differently than it is usually, + * then this rare comparisons may indicate bugs. + * The query categorizes uses of return values into a few categories + * (cmp with int, bool, nullptr, sizeof, another function, ...) + * @kind problem + * @problem.severity warning + * @precision medium + * @id tob/cpp/inconsistent-retval-handling + * @tags security + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow +import semmle.code.cpp.controlflow.IRGuards + +/** + * Categories for uses of functions' return values + */ +newtype TCmpClass = + Tint() + or + Tbool() + or + Tnull() + or + Tptr() + or + Tsizeof(Type s) + or + Tcall() + or + Targ() + or + Tarithm() + +class CmpClass extends TCmpClass { + string toString() { + this = Tint() and result = " numeric literal" + or + this = Tbool() and result = " bool" + or + this = Tnull() and result = " null" + or + this = Tptr() and result = " pointer" + or + exists(Type s | this = Tsizeof(s) and result = " sizeof(" + s + ")") + or + this = Tcall() and result = " some function's return value" + or + this = Tarithm() and result = " arithmetic expression" + or + this = Targ() and result = "in a function" + } +} + +/** + * Literals like 3, 123, 43, 2 + */ +pragma[inline] +predicate numericLiteral(Expr expr) { + expr instanceof Literal + and not expr instanceof BlockExpr + and not expr instanceof FormatLiteral + and not expr instanceof NULL + and not expr instanceof TextLiteral + and not expr instanceof LabelLiteral + and not expr.getType() instanceof NullPointerType + and not expr.getType() instanceof BoolType +} + +/** + * Literals like 3, 123, -43, +2 + * TODO: why codeql for cpp does not have IntegralLiteral? + */ +pragma[inline] +predicate numericArithmLiteral(Expr expr) { + numericLiteral(expr) + or + ( + expr instanceof UnaryArithmeticOperation + and + numericLiteral(expr.getAChild()) + ) +} + +pragma[inline] +predicate binaryComputation(Expr e) { + e instanceof BinaryArithmeticOperation + or + e instanceof BinaryBitwiseOperation + or + e instanceof BinaryLogicalOperation +} + +pragma[inline] +Expr getOtherOperand(ComparisonOperation cmp, Expr fcRetVal) { + (cmp.getRightOperand().getAChild*() = fcRetVal and result = cmp.getLeftOperand()) + or + (cmp.getLeftOperand().getAChild*() = fcRetVal and result = cmp.getRightOperand()) +} + +/** + * Categorize expressions using mostly literals instead of types, because + * we want to differentiate between functions calls and hard-coded stuff. + */ +pragma[inline] +TCmpClass operandCategory(Expr comparedVal) { + (numericArithmLiteral(comparedVal) and result = Tint()) + or + (comparedVal instanceof Literal and comparedVal.getType() instanceof BoolType and result = Tbool()) + or + ((comparedVal.getType() instanceof NullPointerType or comparedVal instanceof NULL) and result = Tnull()) + or + (comparedVal.getUnderlyingType() instanceof DerivedType and result = Tptr()) + or + (comparedVal instanceof Call and result = Tcall()) + or + (comparedVal instanceof SizeofOperator and + ( + result = Tsizeof(comparedVal.(SizeofExprOperator).getExprOperand().getType()) + or + result = Tsizeof(comparedVal.(SizeofTypeOperator).getTypeOperand()) + ) + ) + or + (binaryComputation(comparedVal) and result = Tarithm()) +} + +// module RetValFlowConfig implements DataFlow::ConfigSig { +// predicate isSource(DataFlow::Node source) { +// source.asExpr() = any(Call f) +// } + +// predicate isSink(DataFlow::Node sink) { +// exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr()) +// } +// } +// module RetValFlow = DataFlow::Global; + +/** + * Given function's return value, find its first use in an IF statement + * and assign proper TCmpClass category + */ +predicate categorize(Function f, Call fc, TCmpClass comparedValCategory, IfStmt ifs) { + exists(Expr fcRetVal | + fc.getTarget() = f + and ifs.getCondition().getAChild*() = fcRetVal + + // we could use global DF with a barrier, but that would return a lot of false positives + and DataFlow::localFlow( + DataFlow::exprNode(fc), + DataFlow::exprNode(fcRetVal) + ) + + // exclude far-reaching flows, when the ret val is not checked but is actually used + // in other words, find only the first use in an IF statement + and not exists(IfStmt ifsPrev | + ifsPrev != ifs + and DataFlow::localFlow( + DataFlow::exprNode(fc), + DataFlow::exprNode(ifsPrev.getCondition().getAChild*()) + ) + ) + + and + if + // if(func(retVal)) + exists(Call anyFc | + ifs.getCondition().getAChild*() = anyFc + and anyFc.getAnArgument().getAChild*() = fcRetVal + ) + then + comparedValCategory = Targ() + else ( + // if (retVal != comparedVal) + exists(ComparisonOperation cmp | + ifs.getCondition().getAChild*() = cmp + // skip if we are passing the return value to some function + and not exists(Call tmpCall | + cmp.getAChild*() = tmpCall + and tmpCall.getAnArgument().getAChild*() = fcRetVal + ) + and ( + // if (2*retVal+1 != comparedVal) + // if (retVal > 2*anything()+sizeof(struct)) + if ( + cmp.getAnOperand().getAChild*() = fcRetVal + and binaryComputation(cmp.getAnOperand()) + ) + then + comparedValCategory = Tarithm() + else ( + // if (retVal != comparedVal) + comparedValCategory = operandCategory(getOtherOperand(cmp, fcRetVal)) + ) + ) + ) + ) + ) +} + +/** + * Count all eligible IF statements that + * checks return values of the given function + */ +int countAllRetValTypes(Function f) { + result = count(IfStmt ifs | + categorize(f, _, _, ifs) | + ifs + ) +} + +/** + * Determine what is the most commont TCmpClass category + * for the given function (by counting eligible IF statements) + */ +int mostCommonRetValType(Function f, TCmpClass mostCommonCategory) { + result = max(int numberOfRetValTypeInstances | + categorize(f, _, mostCommonCategory, _) + and numberOfRetValTypeInstances = count(IfStmt ifs | categorize(f, _, mostCommonCategory, ifs) | ifs) + ) +} + +// uncomment for testing: +// from Function f, Call fc, TCmpClass comparedValCategory, CmpClass x, IfStmt ifs +// where +// categorize(f, fc, comparedValCategory, ifs) +// and x = comparedValCategory +// // and f.getName() = "sshbuf_fromb" +// select f, fc, x, ifs + + +from Function f, int retValsTotalAmount, + TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax, + TCmpClass buggyCategory, CmpClass buggyCategoryClass, Call buggyFc, + IfStmt ifs +where + not buggyFc.getLocation().getFile().toString().toLowerCase().regexpMatch(".*test.*") + // we are interested only in defined (e.g., not libc) and used functions + and exists(Call fc | fc.getTarget() = f) + and f.hasDefinition() + + // the function's retVal must be used in some IF statements + and retValsTotalAmount = countAllRetValTypes(f) + and retValsTotalAmount >= 4 + + // now we find how the function's retVal is used most commonly + and categoryMax = mostCommonRetValType(f, mostCommonCategory) + and mostCommonCategoryClass = mostCommonCategory + + // if threshold for "most common" use case is ~75%, then remaining 25% function calls are handled somehow incorrectly + and ((float)(categoryMax * 100) / retValsTotalAmount) >= 74 + + // // and finally we are looking for calls that use retVal in an uncommon way + and categorize(f, buggyFc, buggyCategory, ifs) + and buggyCategory != mostCommonCategory + and buggyCategoryClass = buggyCategory + + // return value could be used multiple times in a single IF statement + // don't show such findings + and not categoryMax = retValsTotalAmount + +select buggyFc, "Function $@ return value is usually compared with" + mostCommonCategoryClass + " (" + categoryMax + + " of " + retValsTotalAmount + " times), but this call compares with" + buggyCategoryClass + " $@", + f, f.getName(), ifs, "here" diff --git a/cpp/test/include/libc/string_stubs.h b/cpp/test/include/libc/string_stubs.h index 0f7889d..16e9058 100644 --- a/cpp/test/include/libc/string_stubs.h +++ b/cpp/test/include/libc/string_stubs.h @@ -1,20 +1,39 @@ #ifndef USE_HEADERS +#ifndef HEADER_STRINGSTUB_STUB_H +#define HEADER_STRINGSTUB_STUB_H + +#ifdef __cplusplus +extern "C" { +#endif + #ifndef NULL #define NULL 0 #endif -typedef int wchar_t; +#define wchar_t int extern char* strcpy_s(char* dst, int max_amount, char* src); extern int _mbsncat(char* dst, char* src, int count); extern int _mbsncmp(char* dst, char* src, int count); extern int _memicmp(char* a, char* b, long count); extern int _mbsnbcmp(char* a, char* b, int count); -extern void *printf(const char * format, ...); -extern int strlen(const char *s); +extern int printf(const char * format, ...); +extern unsigned long strlen(const char *s); extern char* strcpy(char * dst, const char * src); extern int wprintf(const wchar_t * format, ...); extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2); +extern void perror(const char *s); +extern int puts(const char *s); + +extern void openlog(const char*, int, int); +extern void syslog(int, const char*, ...); +extern void closelog(void); + +#ifdef __cplusplus +} +#endif + +#endif #else @@ -23,6 +42,7 @@ extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2); #include #include #include +#include #define strcpy_s strcpy #define _mbsncat strncat diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp new file mode 100644 index 0000000..38fe2c4 --- /dev/null +++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp @@ -0,0 +1,470 @@ +#include "../../../include/libc/string_stubs.h" + +struct something { + int x; + char w; +}; + +struct athing { + int x; + char w; + int y; +}; + +static int w = 2; + +bool cmp_func(int a) +{ + return a >= 10 ? true : false; +} +bool cmp_func2(int a) +{ + return a >= 102 ? true : false; +} + +int main() { + return 0; +} + +// BAD 1 +int target_func_1(int a) +{ + return a + 1; +} + +void test_1_1() { + // BAD: comparing with sizeof instead of hard-coded int + if (target_func_1(2) != sizeof(something)) { + puts("something"); + } +} + +void test_1_2() { + if (target_func_1(2) != 1) { + puts("something2"); + } + if (target_func_1(2) > 0) { + puts("something2"); + } + int r = target_func_1(3); + if (r > 0) { + puts("something2"); + } +} + + + +// BAD 2 +int target_func_2(int a) +{ + return a - 2; +} + +void test_2_1() { + if (target_func_2(2) != sizeof(struct something)) { + puts("something"); + } +} + +void test_2_2() { + if (target_func_2(-123) != sizeof(struct something)) { + puts("something"); + } + int r = target_func_2(123); + if (r > sizeof(struct something)) { + return; + } + + r = target_func_2(3); + // BAD: comparing with hard-coded int instead of sizeof + if (r > 0) { + puts("something2"); + } +} + +// BAD 3 +int target_func_3(int a) +{ + return a*a; +} + +void test_3_1() { + if (target_func_3(2) != sizeof(struct something)) { + puts("something"); + } +} + +void test_3_2() { + auto athingInstance = athing{}; + auto somethingInstance = something{}; + + if (target_func_3(-123) != sizeof(something)) { + puts("something"); + } + int r = target_func_3(123); + if (r > sizeof(struct something)) { + return; + } + r = target_func_3(1223); + if (r > sizeof(somethingInstance)) { + return; + } + r = target_func_3(1323); + if (r >= sizeof(somethingInstance)) { + return; + } + r = target_func_3(1523); + if (r != sizeof(somethingInstance)) { + return; + } + + r = target_func_3(-3); + // BAD: comparing with a different sizeof + if (r > sizeof(struct athing)) { + puts("something2"); + } + + r = target_func_3(-3); + // BAD: comparing with a different sizeof + if (r > sizeof(athingInstance)) { + puts("something2"); + } +} + +// BAD 4 +class SomeClass +{ +private: + bool r; + int r2; +public: + SomeClass(); + void DoSomething(int); + void DoSomethingElse(); + int target_func_4(int); +}; + +void test_4_1() { + SomeClass sc = SomeClass(); + if (sc.target_func_4(2) != 1) { + puts("something"); + } +} + +void test_4_2() { + SomeClass sc = SomeClass(); + if (sc.target_func_4(-123) != -1) { + puts("something"); + } + int r = sc.target_func_4(123); + if (r > 0) { + return; + } + + r = sc.target_func_4(-3); + if (r > 123) { + puts("something2"); + } +} + +SomeClass::SomeClass() { + r = true; + r2 = 33; +} + +void SomeClass::DoSomething(int a) { + if (target_func_4(-123) != -1) { + puts("something"); + } + int r = target_func_4(123); + if (r > 0) { + return; + } + + r = target_func_4(-3); + if (r > 123) { + puts("something2"); + } +} + +void SomeClass::DoSomethingElse() { + if (target_func_4(-123) != -1) { + puts("something"); + } + int r = target_func_4(123); + // BAD: bool instead of int comparison + if (r != true) { + return; + } +} + +int SomeClass::target_func_4(int a) { + return a + 2; +} + +// BAD 5 +int* target_func_5(int a) +{ + return &w; +} + +void test_5_1() { + if (target_func_5(2) != nullptr) { + puts("something"); + } +} + +void test_5_2() { + if (target_func_5(-123) != nullptr) { + puts("something"); + } + int *r = target_func_5(123); + if (r == NULL) { + return; + } + + r = target_func_5(-3); + int *some_ptr = &w; + // BAD: comparing with a pointer instead of NULL + if (r > some_ptr) { + puts("something2"); + } +} + +// BAD 6 +bool target_func_6(int a) +{ + return a > 10 ? true : false; +} + +void test_6_1() { + if (target_func_6(2) != cmp_func(2)) { + puts("something"); + } +} + +void test_6_2() { + if (target_func_6(-123) != cmp_func2(-123)) { + puts("something"); + } + bool r = target_func_6(123); + if (r == cmp_func(3333)) { + return; + } + + r = target_func_6(-3); + // BAD: comparing with sizeof instead of some other function + if (r != (bool)sizeof(something)) { + puts("something2"); + } +} + +// BAD 7 +bool target_func_7(int a) +{ + return a > 10 ? true : false; +} +bool some_func_cmp(bool x) { + return !x; +} + +void test_7_1() { + if (some_func_cmp(target_func_7(2))) { + puts("something"); + } +} + +void test_7_2() { + if (some_func_cmp(target_func_7(12)) != cmp_func2(-123)) { + puts("something"); + } + bool r = target_func_7(123); + if (some_func_cmp(r) < 7) { + return; + } + + r = target_func_7(-3); + // BAD: comparing with something instead of using as argument to a function + if (r != cmp_func2(-123)) { + puts("something2"); + } +} + +// BAD 8 +bool target_func_8(int a) +{ + return a > 10 ? true : false; +} +int some_func_arithmetic(int x) { + return x*3; +} + +void test_8_1() { + if (target_func_8(2)*4 == 2*5+6) { + puts("something"); + } +} + +void test_8_2() { + if (target_func_8(12) != 4*some_func_arithmetic(-3)) { + puts("something"); + } + bool r = target_func_8(123); + if (r*2 < sizeof(something)) { + return; + } + + r = target_func_8(-3); + // BAD: comparing with constant instead of dynamically computed value + if (r != 3) { + puts("something8"); + } +} + +// BAD 9 +int* target_func_9(int a) +{ + return a > 10 ? &w : NULL; +} + +void test_9_1() { + if (target_func_9(2) == NULL) { + puts("something"); + } +} + +void test_9_2() { + int *tt; + if ((tt = target_func_9(12)) != NULL) { + puts("something"); + } + tt = target_func_9(123); + if (tt == NULL) { + return; + } + + // BAD: comparing with int instead of NULL + if ((int)(tt = target_func_9(-3)) != 0) { + puts("something8"); + } +} + +// GOOD 1: always comparing with NULL +int* target_func_g1(int a) +{ + return &w; +} + +void test_g_1_1() { + if (target_func_g1(2) != nullptr) { + puts("something"); + } +} + +void test_g_1_2() { + if (target_func_g1(-123) != nullptr) { + puts("something"); + } + int *r = target_func_g1(123); + if (r == NULL) { + return; + } + + r = target_func_g1(-3); + if (r == nullptr) { + puts("something2"); + } +} + +// GOOD 2: always comparing with some function +bool target_func_g2(int a) +{ + return a > 10 ? true : false; +} + +void test_g_2_1() { + if (target_func_g2(2) != cmp_func(2)) { + puts("something"); + } +} + +void test_g_2_2() { + if (target_func_g2(-123) != cmp_func(-123)) { + puts("something"); + } + bool r = target_func_g2(123); + if (r == cmp_func(3333)) { + return; + } + + r = target_func_g2(-3); + if (r > cmp_func2(-3)) { + puts("something2"); + } +} + +// GOOD 3: always comparing with int, check only first use +bool target_func_g3(int a) +{ + return a > 10 ? true : false; +} + +void test_g_3_1() { + if (target_func_g3(2) != 3) { + puts("something"); + } +} + +void test_g_3_2() { + if (target_func_g3(-123) != 4) { + puts("something"); + } + bool r = target_func_g3(123); + if (r == 777) { + return; + } + + r = target_func_g3(-3); + if (r > -123) { + puts("something2"); + } + // We don't want to find this use, because it is not a ret val check + // but an actuall use + if (cmp_func(r)) { + return; + } +} + +// GOOD 4: always comparing with int, handle multi-condition IFs +bool target_func_g4(int a) +{ + return a > 10 ? true : false; +} + +void test_g_4_1() { + if (target_func_g4(2) != 3) { + puts("something"); + } +} + +void test_g_4_2() { + int *www = &w; + + if (target_func_g4(-123) != 4) { + puts("something"); + } + bool r = target_func_g4(123); + if (r == 777) { + return; + } + + r = target_func_g4(-3); + if (r > -123) { + puts("something2"); + } + + bool rr = target_func_g4(-3); + if (www == NULL || rr > -123) { + puts("something2"); + } +} \ No newline at end of file diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected new file mode 100644 index 0000000..6d06444 --- /dev/null +++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected @@ -0,0 +1,10 @@ +| InconsistentReturnValueHandling.cpp:37:9:37:21 | call to target_func_1 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:30:5:30:17 | target_func_1 | target_func_1 | InconsistentReturnValueHandling.cpp:37:5:39:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:78:9:78:21 | call to target_func_2 | Function $@ return value is usually compared with sizeof(something) (3 of 4 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:58:5:58:17 | target_func_2 | target_func_2 | InconsistentReturnValueHandling.cpp:80:5:82:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:121:9:121:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) $@ | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 | InconsistentReturnValueHandling.cpp:123:5:125:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:127:9:127:21 | call to target_func_3 | Function $@ return value is usually compared with sizeof(something) (6 of 8 times), but this call compares with sizeof(athing) $@ | InconsistentReturnValueHandling.cpp:86:5:86:17 | target_func_3 | target_func_3 | InconsistentReturnValueHandling.cpp:129:5:131:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:194:13:194:25 | call to target_func_4 | Function $@ return value is usually compared with numeric literal (8 of 9 times), but this call compares with bool $@ | InconsistentReturnValueHandling.cpp:201:5:201:28 | target_func_4 | target_func_4 | InconsistentReturnValueHandling.cpp:196:5:198:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:226:9:226:21 | call to target_func_5 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with pointer $@ | InconsistentReturnValueHandling.cpp:206:6:206:18 | target_func_5 | target_func_5 | InconsistentReturnValueHandling.cpp:229:5:231:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:255:9:255:21 | call to target_func_6 | Function $@ return value is usually compared with some function's return value (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:235:6:235:18 | target_func_6 | target_func_6 | InconsistentReturnValueHandling.cpp:257:5:259:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:286:9:286:21 | call to target_func_7 | Function $@ return value is usually compared within call to some function (3 of 4 times), but this call compares with some function's return value $@ | InconsistentReturnValueHandling.cpp:263:6:263:18 | target_func_7 | target_func_7 | InconsistentReturnValueHandling.cpp:288:5:290:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:317:9:317:21 | call to target_func_8 | Function $@ return value is usually compared with arithmetic expression (3 of 4 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:294:6:294:18 | target_func_8 | target_func_8 | InconsistentReturnValueHandling.cpp:319:5:321:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:347:20:347:32 | call to target_func_9 | Function $@ return value is usually compared with null (3 of 4 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:325:6:325:18 | target_func_9 | target_func_9 | InconsistentReturnValueHandling.cpp:347:5:349:5 | if (...) ... | here | diff --git a/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref new file mode 100644 index 0000000..47ca537 --- /dev/null +++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qlref @@ -0,0 +1,2 @@ +security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql +