diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 2b53e6d56aa..7ab002857cf 100644 --- a/change-notes/1.24/analysis-cpp.md +++ b/change-notes/1.24/analysis-cpp.md @@ -24,6 +24,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. | No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. | | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | | This query is no longer run on LGTM. | | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings. This approach produces fewer, more accurate results. | +| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. | | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. | | Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) | | This query is no longer run on LGTM. | | Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. | @@ -53,3 +54,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. * The library now models data flow through formatting functions such as `sprintf`. * The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) uses a new intermediate representation. This provides a more precise analysis of pointers to stack variables and flow through parameters, improving the results of many security queries. * The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering`) uses a new intermediate representation to provide a more precise analysis of heap allocated memory and pointers to stack variables. +* `freeCall` in `semmle.code.cpp.commons.Alloc` has been deprecated. The`Allocation` and `Deallocation` models in `semmle.code.cpp.models.interfaces` should be used instead. diff --git a/cpp/ql/src/Critical/MemoryFreed.qll b/cpp/ql/src/Critical/MemoryFreed.qll index d548aca26e7..880199e54c9 100644 --- a/cpp/ql/src/Critical/MemoryFreed.qll +++ b/cpp/ql/src/Critical/MemoryFreed.qll @@ -4,7 +4,7 @@ private predicate freed(Expr e) { e = any(DeallocationExpr de).getFreedExpr() or exists(ExprCall c | - // cautiously assume that any ExprCall could be a freeCall. + // cautiously assume that any `ExprCall` could be a deallocation expression. c.getAnArgument() = e ) } diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll index 9dd55525b59..30b9f9ad94a 100644 --- a/cpp/ql/src/Critical/NewDelete.qll +++ b/cpp/ql/src/Critical/NewDelete.qll @@ -5,17 +5,34 @@ import cpp import semmle.code.cpp.controlflow.SSA import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.models.implementations.Allocation +import semmle.code.cpp.models.implementations.Deallocation /** * Holds if `alloc` is a use of `malloc` or `new`. `kind` is * a string describing the type of the allocation. */ predicate allocExpr(Expr alloc, string kind) { - isAllocationExpr(alloc) and - not alloc.isFromUninstantiatedTemplate(_) and ( - alloc instanceof FunctionCall and - kind = "malloc" + exists(Function target | + alloc.(AllocationExpr).(FunctionCall).getTarget() = target and + ( + target.getName() = "operator new" and + kind = "new" and + // exclude placement new and custom overloads as they + // may not conform to assumptions + not target.getNumberOfParameters() > 1 + or + target.getName() = "operator new[]" and + kind = "new[]" and + // exclude placement new and custom overloads as they + // may not conform to assumptions + not target.getNumberOfParameters() > 1 + or + not target instanceof OperatorNewAllocationFunction and + kind = "malloc" + ) + ) or alloc instanceof NewExpr and kind = "new" and @@ -28,7 +45,8 @@ predicate allocExpr(Expr alloc, string kind) { // exclude placement new and custom overloads as they // may not conform to assumptions not alloc.(NewArrayExpr).getAllocatorCall().getTarget().getNumberOfParameters() > 1 - ) + ) and + not alloc.isFromUninstantiatedTemplate(_) } /** @@ -110,8 +128,20 @@ predicate allocReaches(Expr e, Expr alloc, string kind) { * describing the type of that free or delete. */ predicate freeExpr(Expr free, Expr freed, string kind) { - freeCall(free, freed) and - kind = "free" + exists(Function target | + freed = free.(DeallocationExpr).getFreedExpr() and + free.(FunctionCall).getTarget() = target and + ( + target.getName() = "operator delete" and + kind = "delete" + or + target.getName() = "operator delete[]" and + kind = "delete[]" + or + not target instanceof OperatorDeleteDeallocationFunction and + kind = "free" + ) + ) or free.(DeleteExpr).getExpr() = freed and kind = "delete" diff --git a/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll b/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll index a508c929d9a..a9597fc72b5 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll @@ -23,6 +23,8 @@ predicate freeFunction(Function f, int argNum) { argNum = f.(DeallocationFunctio /** * A call to a library routine that frees memory. + * + * DEPRECATED: Use `DeallocationExpr` instead (this also includes `delete` expressions). */ predicate freeCall(FunctionCall fc, Expr arg) { arg = fc.(DeallocationExpr).getFreedExpr() } diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index 28f8c6f46e1..7a289ebb494 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -144,7 +144,17 @@ private predicate writesVariable(StoreInstruction store, Variable var) { } /** - * A variable that has any kind of upper-bound check anywhere in the program + * A variable that has any kind of upper-bound check anywhere in the program. This is + * biased towards being inclusive because there are a lot of valid ways of doing an + * upper bounds checks if we don't consider where it occurs, for example: + * ``` + * if (x < 10) { sink(x); } + * + * if (10 > y) { sink(y); } + * + * if (z > 10) { z = 10; } + * sink(z); + * ``` */ // TODO: This coarse overapproximation, ported from the old taint tracking // library, could be replaced with an actual semantic check that a particular @@ -153,10 +163,10 @@ private predicate writesVariable(StoreInstruction store, Variable var) { // previously suppressed by this predicate by coincidence. private predicate hasUpperBoundsCheck(Variable var) { exists(RelationalOperation oper, VariableAccess access | - oper.getLeftOperand() = access and + oper.getAnOperand() = access and access.getTarget() = var and // Comparing to 0 is not an upper bound check - not oper.getRightOperand().getValue() = "0" + not oper.getAnOperand().getValue() = "0" ) } diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll index c6766983889..f6f7ab279a6 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll @@ -1,3 +1,9 @@ +/** + * Provides implementation classes modelling various methods of allocation + * (`malloc`, `new` etc). See `semmle.code.cpp.models.interfaces.Allocation` + * for usage information. + */ + import semmle.code.cpp.models.interfaces.Allocation /** diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll index d2e4951e436..980645df031 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll @@ -1,4 +1,10 @@ -import semmle.code.cpp.models.interfaces.Allocation +/** + * Provides implementation classes modelling various methods of deallocation + * (`free`, `delete` etc). See `semmle.code.cpp.models.interfaces.Deallocation` + * for usage information. + */ + +import semmle.code.cpp.models.interfaces.Deallocation /** * A deallocation function such as `free`. diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll index 3a37b43b319..a24820b277f 100644 --- a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll +++ b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll @@ -328,14 +328,24 @@ GlobalOrNamespaceVariable globalVarFromId(string id) { } /** - * A variable that has any kind of upper-bound check anywhere in the program + * A variable that has any kind of upper-bound check anywhere in the program. This is + * biased towards being inclusive because there are a lot of valid ways of doing an + * upper bounds checks if we don't consider where it occurs, for example: + * ``` + * if (x < 10) { sink(x); } + * + * if (10 > y) { sink(y); } + * + * if (z > 10) { z = 10; } + * sink(z); + * ``` */ private predicate hasUpperBoundsCheck(Variable var) { exists(RelationalOperation oper, VariableAccess access | - oper.getLeftOperand() = access and + oper.getAnOperand() = access and access.getTarget() = var and // Comparing to 0 is not an upper bound check - not oper.getRightOperand().getValue() = "0" + not oper.getAnOperand().getValue() = "0" ) } diff --git a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected index 7350749ce96..45f88d426c9 100644 --- a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected +++ b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected @@ -1,5 +1,9 @@ | test2.cpp:19:3:19:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:18:12:18:18 | new | new | | test2.cpp:26:3:26:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:25:7:25:13 | new | new | +| test2.cpp:51:2:51:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new | +| test2.cpp:55:2:55:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:46:20:46:33 | call to operator new | new | +| test2.cpp:57:2:57:18 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc | +| test2.cpp:58:2:58:18 | call to operator delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc | | test.cpp:36:2:36:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:27:18:27:23 | call to malloc | malloc | | test.cpp:41:2:41:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test.cpp:26:7:26:17 | new | new | | test.cpp:68:3:68:11 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:64:28:64:33 | call to malloc | malloc | diff --git a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp index 9101758d85b..43a286f6f97 100644 --- a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp +++ b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp @@ -34,3 +34,27 @@ public: }; MyTest2Class mt2c_i; + +// --- + +void* operator new(size_t); +void operator delete(void*); + +void test_operator_new() +{ + void *ptr_new = new int; + void *ptr_opnew = ::operator new(sizeof(int)); + void *ptr_malloc = malloc(sizeof(int)); + + delete ptr_new; // GOOD + ::operator delete(ptr_new); // GOOD + free(ptr_new); // BAD + + delete ptr_opnew; // GOOD + ::operator delete(ptr_opnew); // GOOD + free(ptr_opnew); // BAD + + delete ptr_malloc; // BAD + ::operator delete(ptr_malloc); // BAD + free(ptr_malloc); // GOOD +} diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected index ddb16699fce..3847e91bbc8 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected @@ -39,12 +39,36 @@ edges | test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted | | test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted | | test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted | -| test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | (size_t)... | -| test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | size | -| test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | size | -| test.cpp:123:25:123:38 | (const char *)... | test.cpp:127:24:127:27 | (size_t)... | -| test.cpp:123:25:123:38 | (const char *)... | test.cpp:127:24:127:27 | size | -| test.cpp:123:25:123:38 | (const char *)... | test.cpp:127:24:127:27 | size | +| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | (unsigned long)... | +| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | size | +| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | size | +| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... | +| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... | +| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:27 | (unsigned long)... | +| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:27 | size | +| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:27 | size | +| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:41 | ... * ... | +| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:41 | ... * ... | +| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | (unsigned long)... | +| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | size | +| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | size | +| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... | +| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... | +| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:13 | (unsigned long)... | +| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:13 | size | +| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:13 | size | +| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:27 | ... * ... | +| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:27 | ... * ... | +| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | (unsigned long)... | +| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | size | +| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | size | +| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:28 | ... * ... | +| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:28 | ... * ... | +| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:14 | (unsigned long)... | +| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:14 | size | +| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:14 | size | +| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:28 | ... * ... | +| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:28 | ... * ... | nodes | test.cpp:39:21:39:24 | argv | semmle.label | argv | | test.cpp:39:21:39:24 | argv | semmle.label | argv | @@ -80,13 +104,36 @@ nodes | test.cpp:52:54:52:60 | tainted | semmle.label | tainted | | test.cpp:52:54:52:60 | tainted | semmle.label | tainted | | test.cpp:52:54:52:60 | tainted | semmle.label | tainted | -| test.cpp:123:25:123:30 | call to getenv | semmle.label | call to getenv | -| test.cpp:123:25:123:38 | (const char *)... | semmle.label | (const char *)... | -| test.cpp:127:24:127:27 | (size_t)... | semmle.label | (size_t)... | -| test.cpp:127:24:127:27 | (size_t)... | semmle.label | (size_t)... | +| test.cpp:123:18:123:23 | call to getenv | semmle.label | call to getenv | +| test.cpp:123:18:123:31 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:127:24:127:27 | (unsigned long)... | semmle.label | (unsigned long)... | +| test.cpp:127:24:127:27 | (unsigned long)... | semmle.label | (unsigned long)... | | test.cpp:127:24:127:27 | size | semmle.label | size | | test.cpp:127:24:127:27 | size | semmle.label | size | | test.cpp:127:24:127:27 | size | semmle.label | size | +| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... | +| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... | +| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... | +| test.cpp:132:19:132:24 | call to getenv | semmle.label | call to getenv | +| test.cpp:132:19:132:32 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:134:10:134:13 | (unsigned long)... | semmle.label | (unsigned long)... | +| test.cpp:134:10:134:13 | (unsigned long)... | semmle.label | (unsigned long)... | +| test.cpp:134:10:134:13 | size | semmle.label | size | +| test.cpp:134:10:134:13 | size | semmle.label | size | +| test.cpp:134:10:134:13 | size | semmle.label | size | +| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:138:19:138:24 | call to getenv | semmle.label | call to getenv | +| test.cpp:138:19:138:32 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:142:11:142:14 | (unsigned long)... | semmle.label | (unsigned long)... | +| test.cpp:142:11:142:14 | (unsigned long)... | semmle.label | (unsigned long)... | +| test.cpp:142:11:142:14 | size | semmle.label | size | +| test.cpp:142:11:142:14 | size | semmle.label | size | +| test.cpp:142:11:142:14 | size | semmle.label | size | +| test.cpp:142:11:142:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:142:11:142:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:142:11:142:28 | ... * ... | semmle.label | ... * ... | #select | test.cpp:42:31:42:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | test.cpp:43:31:43:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | @@ -96,4 +143,9 @@ nodes | test.cpp:49:17:49:30 | new[] | test.cpp:39:21:39:24 | argv | test.cpp:49:26:49:29 | size | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | test.cpp:52:21:52:27 | call to realloc | test.cpp:39:21:39:24 | argv | test.cpp:52:35:52:60 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | test.cpp:52:35:52:60 | ... * ... | test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | -| test.cpp:127:17:127:22 | call to malloc | test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | size | This allocation size is derived from $@ and might overflow | test.cpp:123:25:123:30 | call to getenv | user input (getenv) | +| test.cpp:127:17:127:22 | call to malloc | test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) | +| test.cpp:127:24:127:41 | ... * ... | test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | size | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) | +| test.cpp:134:3:134:8 | call to malloc | test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) | +| test.cpp:134:10:134:27 | ... * ... | test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | size | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) | +| test.cpp:142:4:142:9 | call to malloc | test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) | +| test.cpp:142:11:142:28 | ... * ... | test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | size | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp index 5cd5f0c0246..4a1bbd8a9dc 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp @@ -39,10 +39,10 @@ int main(int argc, char **argv) { int tainted = atoi(argv[1]); MyStruct *arr1 = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD - MyStruct *arr2 = (MyStruct *)malloc(tainted); // BAD + MyStruct *arr2 = (MyStruct *)malloc(tainted); // DUBIOUS (not multiplied by anything) MyStruct *arr3 = (MyStruct *)malloc(tainted * sizeof(MyStruct)); // BAD MyStruct *arr4 = (MyStruct *)malloc(getTainted() * sizeof(MyStruct)); // BAD [NOT DETECTED] - MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // BAD [NOT DETECTED] + MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // DUBIOUS (not multiplied by anything) int size = tainted * 8; char *chars1 = (char *)malloc(size); // BAD @@ -52,7 +52,7 @@ int main(int argc, char **argv) { arr1 = (MyStruct *)realloc(arr1, sizeof(MyStruct) * tainted); // BAD size = 8; - chars3 = new char[size]; // GOOD [FALSE POSITIVE] + chars3 = new char[size]; // GOOD return 0; } @@ -120,9 +120,73 @@ int bounded(int x, int limit) { } void open_file_bounded () { - int size = size = atoi(getenv("USER")); + int size = atoi(getenv("USER")); int bounded_size = bounded(size, MAX_SIZE); - int* a = (int*)malloc(bounded_size); // GOOD - int* b = (int*)malloc(size); // BAD -} \ No newline at end of file + int* a = (int*)malloc(bounded_size * sizeof(int)); // GOOD + int* b = (int*)malloc(size * sizeof(int)); // BAD +} + +void more_bounded_tests() { + { + int size = atoi(getenv("USER")); + + malloc(size * sizeof(int)); // BAD + } + + { + int size = atoi(getenv("USER")); + + if (size > 0) + { + malloc(size * sizeof(int)); // BAD + } + } + + { + int size = atoi(getenv("USER")); + + if (size < 100) + { + malloc(size * sizeof(int)); // BAD [NOT DETECTED] + } + } + + { + int size = atoi(getenv("USER")); + + if ((size > 0) && (size < 100)) + { + malloc(size * sizeof(int)); // GOOD + } + } + + { + int size = atoi(getenv("USER")); + + if ((100 > size) && (0 < size)) + { + malloc(size * sizeof(int)); // GOOD + } + } + + { + int size = atoi(getenv("USER")); + + malloc(size * sizeof(int)); // BAD [NOT DETECTED] + + if ((size > 0) && (size < 100)) + { + // ... + } + } + + { + int size = atoi(getenv("USER")); + + if (size > 100) + { + malloc(size * sizeof(int)); // BAD [NOT DETECTED] + } + } +}