diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 7ab002857cf6..8f8c3d5364a6 100644 --- a/change-notes/1.24/analysis-cpp.md +++ b/change-notes/1.24/analysis-cpp.md @@ -25,6 +25,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. | 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. | +| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | The query now produces fewer, more accurate results. | | 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`. | diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql index 2ed95e6cb551..54c6d1e7a966 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql @@ -15,31 +15,31 @@ import cpp import semmle.code.cpp.security.TaintTracking import TaintedWithPath -predicate taintedChild(Expr e, Expr tainted) { - ( - isAllocationExpr(e) - or - any(MulExpr me | me.getAChild() instanceof SizeofOperator) = e - ) and - tainted = e.getAChild() and +/** + * Holds if `alloc` is an allocation, and `tainted` is a child of it that is a + * taint sink. + */ +predicate allocSink(Expr alloc, Expr tainted) { + isAllocationExpr(alloc) and + tainted = alloc.getAChild() and tainted.getUnspecifiedType() instanceof IntegralType } class TaintedAllocationSizeConfiguration extends TaintTrackingConfiguration { - override predicate isSink(Element tainted) { taintedChild(_, tainted) } + override predicate isSink(Element tainted) { allocSink(_, tainted) } } predicate taintedAllocSize( - Expr e, Expr source, PathNode sourceNode, PathNode sinkNode, string taintCause + Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause ) { isUserInput(source, taintCause) and exists(Expr tainted | - taintedChild(e, tainted) and + allocSink(alloc, tainted) and taintedWithPath(source, tainted, sourceNode, sinkNode) ) } -from Expr e, Expr source, PathNode sourceNode, PathNode sinkNode, string taintCause -where taintedAllocSize(e, source, sourceNode, sinkNode, taintCause) -select e, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow", +from Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause +where taintedAllocSize(source, alloc, sourceNode, sinkNode, taintCause) +select alloc, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow", source, "user input (" + taintCause + ")" 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 3847e91bbc85..773a969e9ad4 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 @@ -5,12 +5,6 @@ edges | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | -| test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | (unsigned long)... | -| test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | (unsigned long)... | -| test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | tainted | -| test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | tainted | -| test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | tainted | -| test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | tainted | | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | @@ -33,42 +27,38 @@ edges | test.cpp:39:21:39:24 | argv | test.cpp:52:35:52:60 | ... * ... | | test.cpp:39:21:39:24 | argv | test.cpp:52:35:52:60 | ... * ... | | test.cpp:39:21:39:24 | argv | test.cpp:52:35:52:60 | ... * ... | -| test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | (unsigned long)... | -| test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | (unsigned long)... | -| 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:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted | -| 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 | ... * ... | +| test.cpp:201:9:201:42 | Store | test.cpp:231:9:231:24 | call to get_tainted_size | +| test.cpp:201:9:201:42 | Store | test.cpp:231:9:231:24 | call to get_tainted_size | +| test.cpp:201:14:201:19 | call to getenv | test.cpp:201:9:201:42 | Store | +| test.cpp:201:14:201:27 | (const char *)... | test.cpp:201:9:201:42 | Store | +| test.cpp:214:23:214:23 | s | test.cpp:215:21:215:21 | s | +| test.cpp:214:23:214:23 | s | test.cpp:215:21:215:21 | s | +| test.cpp:220:21:220:21 | s | test.cpp:221:21:221:21 | s | +| test.cpp:220:21:220:21 | s | test.cpp:221:21:221:21 | s | +| test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | (size_t)... | +| test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | +| test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | +| test.cpp:227:24:227:29 | call to getenv | test.cpp:235:11:235:20 | (size_t)... | +| test.cpp:227:24:227:29 | call to getenv | test.cpp:237:10:237:19 | (size_t)... | +| test.cpp:227:24:227:37 | (const char *)... | test.cpp:229:9:229:18 | (size_t)... | +| test.cpp:227:24:227:37 | (const char *)... | test.cpp:229:9:229:18 | local_size | +| test.cpp:227:24:227:37 | (const char *)... | test.cpp:229:9:229:18 | local_size | +| test.cpp:227:24:227:37 | (const char *)... | test.cpp:235:11:235:20 | (size_t)... | +| test.cpp:227:24:227:37 | (const char *)... | test.cpp:237:10:237:19 | (size_t)... | +| test.cpp:235:11:235:20 | (size_t)... | test.cpp:214:23:214:23 | s | +| test.cpp:237:10:237:19 | (size_t)... | test.cpp:220:21:220:21 | s | nodes | test.cpp:39:21:39:24 | argv | semmle.label | argv | | test.cpp:39:21:39:24 | argv | semmle.label | argv | @@ -77,11 +67,6 @@ nodes | test.cpp:42:38:42:44 | tainted | semmle.label | tainted | | test.cpp:42:38:42:44 | tainted | semmle.label | tainted | | test.cpp:42:38:42:44 | tainted | semmle.label | tainted | -| test.cpp:43:38:43:44 | (unsigned long)... | semmle.label | (unsigned long)... | -| test.cpp:43:38:43:44 | (unsigned long)... | semmle.label | (unsigned long)... | -| test.cpp:43:38:43:44 | tainted | semmle.label | tainted | -| test.cpp:43:38:43:44 | tainted | semmle.label | tainted | -| test.cpp:43:38:43:44 | tainted | semmle.label | tainted | | test.cpp:43:38:43:63 | ... * ... | semmle.label | ... * ... | | test.cpp:43:38:43:63 | ... * ... | semmle.label | ... * ... | | test.cpp:43:38:43:63 | ... * ... | semmle.label | ... * ... | @@ -99,53 +84,55 @@ nodes | test.cpp:52:35:52:60 | ... * ... | semmle.label | ... * ... | | test.cpp:52:35:52:60 | ... * ... | semmle.label | ... * ... | | test.cpp:52:35:52:60 | ... * ... | semmle.label | ... * ... | -| test.cpp:52:54:52:60 | (unsigned long)... | semmle.label | (unsigned long)... | -| test.cpp:52:54:52:60 | (unsigned long)... | semmle.label | (unsigned long)... | -| 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: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 | ... * ... | +| test.cpp:201:9:201:42 | Store | semmle.label | Store | +| test.cpp:201:14:201:19 | call to getenv | semmle.label | call to getenv | +| test.cpp:201:14:201:27 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:214:23:214:23 | s | semmle.label | s | +| test.cpp:215:21:215:21 | s | semmle.label | s | +| test.cpp:215:21:215:21 | s | semmle.label | s | +| test.cpp:215:21:215:21 | s | semmle.label | s | +| test.cpp:220:21:220:21 | s | semmle.label | s | +| test.cpp:221:21:221:21 | s | semmle.label | s | +| test.cpp:221:21:221:21 | s | semmle.label | s | +| test.cpp:221:21:221:21 | s | semmle.label | s | +| test.cpp:227:24:227:29 | call to getenv | semmle.label | call to getenv | +| test.cpp:227:24:227:37 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:229:9:229:18 | (size_t)... | semmle.label | (size_t)... | +| test.cpp:229:9:229:18 | (size_t)... | semmle.label | (size_t)... | +| test.cpp:229:9:229:18 | local_size | semmle.label | local_size | +| test.cpp:229:9:229:18 | local_size | semmle.label | local_size | +| test.cpp:229:9:229:18 | local_size | semmle.label | local_size | +| test.cpp:231:9:231:24 | call to get_tainted_size | semmle.label | call to get_tainted_size | +| test.cpp:231:9:231:24 | call to get_tainted_size | semmle.label | call to get_tainted_size | +| test.cpp:231:9:231:24 | call to get_tainted_size | semmle.label | call to get_tainted_size | +| test.cpp:235:11:235:20 | (size_t)... | semmle.label | (size_t)... | +| test.cpp:237:10:237:19 | (size_t)... | semmle.label | (size_t)... | #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) | -| test.cpp:43:38:43:63 | ... * ... | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | test.cpp:45:31:45:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:45:38:45:63 | ... + ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | test.cpp:48:25:48:30 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:48:32:48:35 | size | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | 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: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) | +| test.cpp:215:14:215:19 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:215:21:215:21 | s | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) | +| test.cpp:221:14:221:19 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:221:21:221:21 | s | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) | +| test.cpp:229:2:229:7 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) | +| test.cpp:231:2:231:7 | call to malloc | test.cpp:201:14:201:19 | call to getenv | test.cpp:231:9:231:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow | test.cpp:201:14:201:19 | 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 4a1bbd8a9dc8..cb21e8f1915a 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 @@ -5,8 +5,8 @@ typedef struct {} FILE; void *malloc(size_t size); void *realloc(void *ptr, size_t size); +void free(void *ptr); int atoi(const char *nptr); - struct MyStruct { char data[256]; @@ -190,3 +190,49 @@ void more_bounded_tests() { } } } + +size_t get_untainted_size() +{ + return 10 * sizeof(int); +} + +size_t get_tainted_size() +{ + return atoi(getenv("USER")) * sizeof(int); +} + +size_t get_bounded_size() +{ + size_t s = atoi(getenv("USER")) * sizeof(int); + + if (s < 0) { s = 0; } + if (s > 100) { s = 100; } + + return s; +} + +void *my_alloc(size_t s) { + void *ptr = malloc(s); // [UNHELPFUL RESULT] + + return ptr; +} + +void my_func(size_t s) { + void *ptr = malloc(s); // BAD + + free(ptr); +} + +void more_cases() { + int local_size = atoi(getenv("USER")) * sizeof(int); + + malloc(local_size); // BAD + malloc(get_untainted_size()); // GOOD + malloc(get_tainted_size()); // BAD + malloc(get_bounded_size()); // GOOD + + my_alloc(100); // GOOD + my_alloc(local_size); // BAD [NOT DETECTED IN CORRECT LOCATION] + my_func(100); // GOOD + my_func(local_size); // GOOD +}