diff --git a/README.md b/README.md index 3669d3c..8325fcd 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif - |[Async unsafe signal handler](./cpp/src/docs/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.md)|Async unsafe signal handler (like the one used in CVE-2024-6387)|warning|high| |[Decrementation overflow when comparing](./cpp/src/docs/security/DecOverflowWhenComparing/DecOverflowWhenComparing.md)|This query finds unsigned integer overflows resulting from unchecked decrementation during comparison.|error|high| |[Find all problematic implicit casts](./cpp/src/docs/security/UnsafeImplicitConversions/UnsafeImplicitConversions.md)|Find all implicit casts that may be problematic. That is, casts that may result in unexpected truncation, reinterpretation or widening of values.|error|high| +|[Inconsistent handling of return values from a specific function](./cpp/src/docs/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.md)|Detects functions whose return values are compared inconsistently across call sites, which may indicate bugs|warning|medium| |[Invalid string size passed to string manipulation function](./cpp/src/docs/security/CStrnFinder/CStrnFinder.md)|Finds calls to functions that take as input a string and its size as separate arguments (e.g., `strncmp`, `strncat`, ...) and the size argument is wrong|error|low| |[Missing null terminator](./cpp/src/docs/security/NoNullTerminator/NoNullTerminator.md)|This query finds incorrectly initialized strings that are passed to functions expecting null-byte-terminated strings|error|high| diff --git a/cpp/src/docs/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.md b/cpp/src/docs/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.md new file mode 100644 index 0000000..a8e65b0 --- /dev/null +++ b/cpp/src/docs/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.md @@ -0,0 +1,69 @@ +# Inconsistent handling of return values from a specific function +When a function's return value is checked in `if` statements across multiple call sites, the comparisons typically fall into a consistent pattern (e.g., compared against a numeric literal, `NULL`, or `sizeof`). If a small number of call sites compare the return value in a different way than the majority, these inconsistent comparisons may indicate a bug. + +The query categorizes each comparison into one of the following categories: + +* Numeric literal (e.g., `ret != -1`) +* Boolean (e.g., `ret == true`) +* Null pointer (e.g., `ret != NULL`) +* Pointer +* `sizeof` expression (e.g., `ret > sizeof(buf)`) +* Another function's return value (e.g., `ret != other_func()`) +* Passed as argument to another function (e.g., `if (check(ret))`) +* Arithmetic expression + +When at least 75% of a function's return value comparisons fall into one category, the remaining comparisons in a different category are flagged as potentially incorrect. + + +## Recommendation +Review each flagged call site and verify that the comparison matches the function's return value semantics. If the function returns an error code or count, all call sites should compare it consistently. Fix any comparisons that use the wrong type of operand (e.g., comparing an integer return value against `sizeof` when all other sites compare against a numeric literal). + + +## Example + +```c +struct header { + int type; + int length; +}; + +// Returns number of items processed, or -1 on error +int process_items(int *items, int count) { + int processed = 0; + for (int i = 0; i < count; i++) { + if (items[i] < 0) + return -1; + processed++; + } + return processed; +} + +void example() { + int items[10]; + int result; + + // Typical: return value compared with a numeric literal + result = process_items(items, 10); + if (result > 0) { /* success */ } + + result = process_items(items, 5); + if (result != -1) { /* no error */ } + + result = process_items(items, 3); + if (result == 0) { /* empty */ } + + result = process_items(items, 8); + if (result >= 1) { /* at least one */ } + + // BAD: comparing with sizeof instead of a numeric literal. + // This is inconsistent with all other call sites and likely a bug. + result = process_items(items, 7); + if (result > sizeof(struct header)) { /* wrong comparison */ } +} +``` +In this example, `process_items` returns the number of items processed or `-1` on error. Most call sites correctly compare the return value with a numeric literal. However, one call site mistakenly compares it with `sizeof(struct header)`, which is inconsistent with how the return value is used everywhere else. + + +## References +* [CWE-252: Unchecked Return Value](https://cwe.mitre.org/data/definitions/252.html) +* [CWE-253: Incorrect Check of Function Return Value](https://cwe.mitre.org/data/definitions/253.html) diff --git a/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql index 8a94088..2059014 100644 --- a/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql +++ b/cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql @@ -139,8 +139,7 @@ where ) then cmp.getAFalseSuccessor().getASuccessor*() = varAccAfterOverflow else any() - ) and - // skip vendor code - not dec.getFile().getAbsolutePath().toLowerCase().matches(["%vendor%", "%third_party%"]) + ) + select dec, "Unsigned decrementation in comparison ($@) - $@", cmp, cmp.toString(), varAccAfterOverflow, varAccAfterOverflow.toString() diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.c b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.c new file mode 100644 index 0000000..d65cf09 --- /dev/null +++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.c @@ -0,0 +1,38 @@ +struct header { + int type; + int length; +}; + +// Returns number of items processed, or -1 on error +int process_items(int *items, int count) { + int processed = 0; + for (int i = 0; i < count; i++) { + if (items[i] < 0) + return -1; + processed++; + } + return processed; +} + +void example() { + int items[10]; + int result; + + // Typical: return value compared with a numeric literal + result = process_items(items, 10); + if (result > 0) { /* success */ } + + result = process_items(items, 5); + if (result != -1) { /* no error */ } + + result = process_items(items, 3); + if (result == 0) { /* empty */ } + + result = process_items(items, 8); + if (result >= 1) { /* at least one */ } + + // BAD: comparing with sizeof instead of a numeric literal. + // This is inconsistent with all other call sites and likely a bug. + result = process_items(items, 7); + if (result > sizeof(struct header)) { /* wrong comparison */ } +} diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp new file mode 100644 index 0000000..cd867cb --- /dev/null +++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.qhelp @@ -0,0 +1,62 @@ + + + +

+ When a function's return value is checked in if statements across multiple call sites, + the comparisons typically fall into a consistent pattern (e.g., compared against a numeric literal, + NULL, or sizeof). If a small number of call sites compare the return value + in a different way than the majority, these inconsistent comparisons may indicate a bug. +

+ +

+ The query categorizes each comparison into one of the following categories: +

+ + +

+ When at least 75% of a function's return value comparisons fall into one category, + the remaining comparisons in a different category are flagged as potentially incorrect. +

+ + + +

+Review each flagged call site and verify that the comparison matches the function's return value semantics. +If the function returns an error code or count, all call sites should compare it consistently. +Fix any comparisons that use the wrong type of operand (e.g., comparing an integer return value against +sizeof when all other sites compare against a numeric literal). +

+ + + +

+In this example, process_items returns the number of items processed or -1 +on error. Most call sites correctly compare the return value with a numeric literal. However, one +call site mistakenly compares it with sizeof(struct header), which is inconsistent +with how the return value is used everywhere else. +

+ + + + + +
  • + CWE-252: Unchecked Return Value +
    • +
  • + CWE-253: Incorrect Check of Function Return Value +
    • +     + +     diff --git a/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql new file mode 100644 index 0000000..c1202e3 --- /dev/null +++ b/cpp/src/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.ql @@ -0,0 +1,259 @@ +/** + * @name Inconsistent handling of return values from a specific function + * @description Detects functions whose return values are compared + * inconsistently across call sites, which may indicate bugs. + * @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() + or + Tother() + +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" + or + this = Tother() and result = " other expression" + } +} + +/** + * 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()) + or + ( + not numericArithmLiteral(comparedVal) + and not (comparedVal instanceof Literal and comparedVal.getType() instanceof BoolType) + and not (comparedVal.getType() instanceof NullPointerType or comparedVal instanceof NULL) + and not comparedVal.getUnderlyingType() instanceof DerivedType + and not comparedVal instanceof Call + and not comparedVal instanceof SizeofOperator + and not binaryComputation(comparedVal) + and result = Tother() + ) +} + +/** + * 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) + ) +} + +from Function f, int retValsTotalAmount, + TCmpClass mostCommonCategory, CmpClass mostCommonCategoryClass, int categoryMax, + TCmpClass buggyCategory, CmpClass buggyCategoryClass, Call buggyFc, + IfStmt ifs +where + // we are interested only in defined (e.g., not libc) and used functions + 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 + + // 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 400551c..d589139 100644 --- a/cpp/test/include/libc/string_stubs.h +++ b/cpp/test/include/libc/string_stubs.h @@ -11,7 +11,10 @@ extern "C" { #define NULL 0 #endif +#ifndef __cplusplus typedef int wchar_t; +#endif + extern void *memcpy(void *dst, const void *src, unsigned long n); extern char* strcpy_s(char* dst, int max_amount, char* src); extern int _mbsncat(char* dst, char* src, int count); @@ -24,6 +27,7 @@ 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*, ...); 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..b6f6d05 --- /dev/null +++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.cpp @@ -0,0 +1,841 @@ +#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_2() { + // the baseline for target_func_1 + 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"); + } +} + +void test_1_1() { + // BAD: comparing with sizeof instead of hard-coded int + if (target_func_1(2) != sizeof(something)) { + puts("something"); + } +} + +// 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"); + } +} + +// BAD 10: 4 int comparisons + 1 variable comparison (variable is outlier) +int target_func_10(int a) +{ + return a + 10; +} + +void test_10_1() { + if (target_func_10(2) != 1) { + puts("something"); + } + if (target_func_10(3) > 0) { + puts("something"); + } + int r = target_func_10(4); + if (r > 0) { + puts("something"); + } + if (target_func_10(7) != 99) { + puts("something"); + } +} + +void test_10_2() { + int threshold = 42; + // BAD: comparing with a variable instead of hard-coded int + if (target_func_10(5) != threshold) { + puts("something"); + } +} + +// BAD 11: 4 enum comparisons + 1 int literal (int literal is outlier) +enum ErrorCode { EC_OK = 0, EC_FAIL = 1, EC_RETRY = 2, EC_TIMEOUT = 3 }; + +int target_func_11(int a) +{ + return a > 10 ? EC_OK : EC_FAIL; +} + +void test_11_1() { + if (target_func_11(2) != EC_OK) { + puts("something"); + } + if (target_func_11(3) != EC_FAIL) { + puts("something"); + } + int r = target_func_11(4); + if (r == EC_RETRY) { + puts("something"); + } + r = target_func_11(5); + if (r != EC_TIMEOUT) { + puts("something"); + } +} + +void test_11_2() { + // BAD: comparing with int literal instead of enum constant + if (target_func_11(6) != 42) { + puts("something"); + } +} + +// BAD 12: 3 int + || with two sizeof calls (sizeof is the outlier) +int target_func_12(int a) +{ + return a * 2; +} + +void test_12_1() { + if (target_func_12(2) != 1) { + puts("something"); + } + if (target_func_12(3) > 0) { + puts("something"); + } + int r = target_func_12(4); + if (r > 0) { + puts("something"); + } +} + +void test_12_2() { + // BAD: sizeof comparison in short-circuit expression + if (target_func_12(5) > sizeof(something) || target_func_12(6) > sizeof(something)) { + puts("something"); + } +} + +// GOOD 5: 4 int comparisons + bare if(f()) which is silently dropped +int target_func_g5(int a) +{ + return a + 5; +} + +void test_g_5_1() { + if (target_func_g5(2) != 1) { + puts("something"); + } + if (target_func_g5(3) > 0) { + puts("something"); + } + int r = target_func_g5(4); + if (r > 0) { + puts("something"); + } + if (target_func_g5(7) != 99) { + puts("something"); + } +} + +void test_g_5_2() { + // bare conditional: no comparison, should be silently dropped + if (target_func_g5(6)) { + puts("something"); + } +} + +// GOOD 6: only 3 uses, below threshold of 4 +int target_func_g6(int a) +{ + return a + 6; +} + +void test_g_6_1() { + if (target_func_g6(2) != 1) { + puts("something"); + } + if (target_func_g6(3) > 0) { + puts("something"); + } +} + +void test_g_6_2() { + if (target_func_g6(4) > sizeof(something)) { + puts("something"); + } +} + +// GOOD 7: 2 int + 2 sizeof, 50/50 split below 74% threshold +int target_func_g7(int a) +{ + return a + 7; +} + +void test_g_7_1() { + if (target_func_g7(2) != 1) { + puts("something"); + } + if (target_func_g7(3) > 0) { + puts("something"); + } +} + +void test_g_7_2() { + if (target_func_g7(4) > sizeof(something)) { + puts("something"); + } + int r = target_func_g7(5); + if (r > sizeof(something)) { + puts("something"); + } +} + +// GOOD 8: 4 bool comparisons + if(!f()) which is silently dropped +bool target_func_g8(int a) +{ + return a > 10; +} + +void test_g_8_1() { + if (target_func_g8(2) != true) { + puts("something"); + } + if (target_func_g8(3) == false) { + puts("something"); + } + bool r = target_func_g8(4); + if (r == true) { + puts("something"); + } + if (target_func_g8(5) != false) { + puts("something"); + } +} + +void test_g_8_2() { + // negation: no comparison operator, should be silently dropped + if (!target_func_g8(6)) { + puts("something"); + } +} + +// BAD 13: bare if(func()) mixed with comparisons, should not affect categorization +int target_func_13(int a) +{ + return a + 13; +} + +void test_13_1() { + if (target_func_13(2) != 1) { + puts("something"); + } + if (target_func_13(3) > 0) { + puts("something"); + } + int r = target_func_13(4); + if (r > 0) { + puts("something"); + } +} + +void test_13_2() { + // bare conditional: no comparison, should be silently dropped + if (target_func_13(5)) { + puts("something"); + } + // BAD: sizeof comparison when others use int + if (target_func_13(6) > sizeof(something)) { + puts("something"); + } +} + +// BAD 14: comparison in else-if is properly categorized +int target_func_14(int a) +{ + return a + 14; +} + +void test_14_1() { + if (target_func_14(2) != 1) { + puts("something"); + } + if (target_func_14(3) > 0) { + puts("something"); + } + int r = target_func_14(4); + if (r > 0) { + puts("something"); + } +} + +void test_14_2() { + int x = 42; + // BAD: sizeof comparison in else-if branch + if (x > 100) { + puts("high"); + } else if (target_func_14(5) > sizeof(something)) { + puts("something"); + } +} + +// BAD 15: comparison inside && short-circuit expression +int target_func_15(int a) +{ + return a + 15; +} + +void test_15_1() { + if (target_func_15(2) != 1) { + puts("something"); + } + if (target_func_15(3) > 0) { + puts("something"); + } + int r = target_func_15(4); + if (r > 0) { + puts("something"); + } +} + +void test_15_2() { + int other = 42; + // BAD: sizeof comparison inside && expression + if (target_func_15(5) > sizeof(something) && other > 0) { + puts("something"); + } +} + +// BAD 16: deep variable copy chain before comparison +int target_func_16(int a) +{ + return a + 16; +} + +void test_16_1() { + if (target_func_16(2) != 1) { + puts("something"); + } + if (target_func_16(3) > 0) { + puts("something"); + } + int r = target_func_16(4); + if (r > 0) { + puts("something"); + } +} + +void test_16_2() { + int r = target_func_16(5); + int s = r; + // BAD: sizeof comparison after variable copy + if (s > sizeof(something)) { + puts("something"); + } +} + +// GOOD 9: all bare if(func()) — nothing categorized, below threshold +int target_func_g9(int a) +{ + return a + 9; +} + +void test_g_9_1() { + if (target_func_g9(1)) { + puts("something"); + } + if (target_func_g9(2)) { + puts("something"); + } + if (target_func_g9(3)) { + puts("something"); + } + if (target_func_g9(4)) { + puts("something"); + } + if (target_func_g9(5)) { + puts("something"); + } +} + +// GOOD 10: reversed operand order — all same category (Tint) +int target_func_g10(int a) +{ + return a + 100; +} + +void test_g_10_1() { + if (target_func_g10(1) > 0) { + puts("something"); + } + if (target_func_g10(2) != 5) { + puts("something"); + } + if (0 < target_func_g10(3)) { + puts("something"); + } + if (target_func_g10(4) >= -1) { + puts("something"); + } +} + +// GOOD 11: return value used only in while/for, not in if +int target_func_g12(int a) +{ + return a + 120; +} + +void test_g_12_1() { + while (target_func_g12(1) > 0) { + break; + } + while (target_func_g12(2) > sizeof(something)) { + break; + } + for (int i = 0; target_func_g12(3) > 0; i++) { + break; + } + for (int i = 0; target_func_g12(4) > sizeof(something); i++) { + break; + } +} \ 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..b339210 --- /dev/null +++ b/cpp/test/query-tests/security/InconsistentReturnValueHandling/InconsistentReturnValueHandling.expected @@ -0,0 +1,18 @@ +| InconsistentReturnValueHandling.cpp:51:9:51: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:51:5:53:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:77:9:77: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:57:5:57:17 | target_func_2 | target_func_2 | InconsistentReturnValueHandling.cpp:79:5:81:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:120:9:120: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:85:5:85:17 | target_func_3 | target_func_3 | InconsistentReturnValueHandling.cpp:122:5:124:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:126:9:126: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:85:5:85:17 | target_func_3 | target_func_3 | InconsistentReturnValueHandling.cpp:128:5:130:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:193:13:193: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:200:5:200:28 | target_func_4 | target_func_4 | InconsistentReturnValueHandling.cpp:195:5:197:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:225:9:225: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:205:6:205:18 | target_func_5 | target_func_5 | InconsistentReturnValueHandling.cpp:228:5:230:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:254:9:254: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:234:6:234:18 | target_func_6 | target_func_6 | InconsistentReturnValueHandling.cpp:256:5:258:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:285:9:285:21 | call to target_func_7 | Function $@ return value is usually compared within a function (3 of 4 times), but this call compares with some function's return value $@ | InconsistentReturnValueHandling.cpp:262:6:262:18 | target_func_7 | target_func_7 | InconsistentReturnValueHandling.cpp:287:5:289:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:316:9:316: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:293:6:293:18 | target_func_8 | target_func_8 | InconsistentReturnValueHandling.cpp:318:5:320:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:346:20:346: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:324:6:324:18 | target_func_9 | target_func_9 | InconsistentReturnValueHandling.cpp:346:5:348:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:496:9:496:22 | call to target_func_10 | Function $@ return value is usually compared with numeric literal (4 of 5 times), but this call compares with other expression $@ | InconsistentReturnValueHandling.cpp:472:5:472:18 | target_func_10 | target_func_10 | InconsistentReturnValueHandling.cpp:496:5:498:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:528:9:528:22 | call to target_func_11 | Function $@ return value is usually compared with other expression (4 of 5 times), but this call compares with numeric literal $@ | InconsistentReturnValueHandling.cpp:504:5:504:18 | target_func_11 | target_func_11 | InconsistentReturnValueHandling.cpp:528:5:530:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:554:9:554:22 | call to target_func_12 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:534:5:534:18 | target_func_12 | target_func_12 | InconsistentReturnValueHandling.cpp:554:5:556:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:554:50:554:63 | call to target_func_12 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:534:5:534:18 | target_func_12 | target_func_12 | InconsistentReturnValueHandling.cpp:554:5:556:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:688:9:688:22 | call to target_func_13 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:664:5:664:18 | target_func_13 | target_func_13 | InconsistentReturnValueHandling.cpp:688:5:690:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:717:16:717:29 | call to target_func_14 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:694:5:694:18 | target_func_14 | target_func_14 | InconsistentReturnValueHandling.cpp:717:12:719:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:744:9:744:22 | call to target_func_15 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:723:5:723:18 | target_func_15 | target_func_15 | InconsistentReturnValueHandling.cpp:744:5:746:5 | if (...) ... | here | +| InconsistentReturnValueHandling.cpp:769:13:769:26 | call to target_func_16 | Function $@ return value is usually compared with numeric literal (3 of 4 times), but this call compares with sizeof(something) $@ | InconsistentReturnValueHandling.cpp:750:5:750:18 | target_func_16 | target_func_16 | InconsistentReturnValueHandling.cpp:772:5:774: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 +