From c7d76da9ed6483648d3d5776d55d4b305a67a16c Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Wed, 14 Aug 2024 11:25:42 +0200 Subject: [PATCH] C++: Reuse bounded predicate in TaintedAllocationSize query --- cpp/ql/src/Security/CWE/CWE-190/Bounded.qll | 4 + .../CWE/CWE-190/TaintedAllocationSize.ql | 12 +- .../TaintedAllocationSize.expected | 108 +++++++++--------- .../semmle/TaintedAllocationSize/test.cpp | 9 ++ 4 files changed, 69 insertions(+), 64 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll b/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll index ff5c347e5e213..bb855da3f4488 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll +++ b/cpp/ql/src/Security/CWE/CWE-190/Bounded.qll @@ -24,6 +24,10 @@ private predicate boundedBitwiseAnd(Expr e, Expr andExpr, Expr operand1, Expr op * operation that may greatly reduce the range of possible values. */ predicate bounded(Expr e) { + // There can be two separate reasons for `convertedExprMightOverflow` not holding: + // 1. `e` really cannot overflow. + // 2. `e` isn't analyzable. + // If we didn't rule out case 2 we would declare anything that isn't analyzable as bounded. ( e instanceof UnaryArithmeticOperation or e instanceof BinaryArithmeticOperation or diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql index a10ee006c4702..2fd0e940cdae9 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql @@ -20,6 +20,7 @@ import semmle.code.cpp.ir.IR import semmle.code.cpp.controlflow.IRGuards import semmle.code.cpp.security.FlowSources import TaintedAllocationSize::PathGraph +import Bounded /** * Holds if `alloc` is an allocation, and `tainted` is a child of it that is a @@ -61,16 +62,7 @@ module TaintedAllocationSizeConfig implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node node) { exists(Expr e | e = node.asExpr() | - // There can be two separate reasons for `convertedExprMightOverflow` not holding: - // 1. `e` really cannot overflow. - // 2. `e` isn't analyzable. - // If we didn't rule out case 2 we would place barriers on anything that isn't analyzable. - ( - e instanceof UnaryArithmeticOperation or - e instanceof BinaryArithmeticOperation or - e instanceof AssignArithmeticOperation - ) and - not convertedExprMightOverflow(e) + bounded(e) or // Subtracting two pointers is either well-defined (and the result will likely be small), or // terribly undefined and dangerous. Here, we assume that the programmer has ensured that the 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 a10ef491282c2..49f0e14276cde 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 @@ -13,26 +13,26 @@ edges | test.cpp:133:19:133:32 | *call to getenv | test.cpp:133:14:133:17 | call to atoi | provenance | TaintFunction | | test.cpp:148:15:148:18 | call to atol | test.cpp:152:11:152:28 | ... * ... | provenance | | | test.cpp:148:20:148:33 | *call to getenv | test.cpp:148:15:148:18 | call to atol | provenance | TaintFunction | -| test.cpp:209:8:209:23 | *get_tainted_size | test.cpp:241:9:241:24 | call to get_tainted_size | provenance | | -| test.cpp:211:9:211:42 | ... * ... | test.cpp:209:8:209:23 | *get_tainted_size | provenance | | -| test.cpp:211:14:211:27 | *call to getenv | test.cpp:211:9:211:42 | ... * ... | provenance | TaintFunction | -| test.cpp:230:21:230:21 | s | test.cpp:231:21:231:21 | s | provenance | | -| test.cpp:237:19:237:52 | ... * ... | test.cpp:239:9:239:18 | local_size | provenance | | -| test.cpp:237:19:237:52 | ... * ... | test.cpp:245:11:245:20 | local_size | provenance | | -| test.cpp:237:19:237:52 | ... * ... | test.cpp:247:10:247:19 | local_size | provenance | | -| test.cpp:237:24:237:37 | *call to getenv | test.cpp:237:19:237:52 | ... * ... | provenance | TaintFunction | -| test.cpp:247:10:247:19 | local_size | test.cpp:230:21:230:21 | s | provenance | | -| test.cpp:250:20:250:27 | *out_size | test.cpp:289:17:289:20 | get_size output argument | provenance | | -| test.cpp:250:20:250:27 | *out_size | test.cpp:305:18:305:21 | get_size output argument | provenance | | -| test.cpp:251:2:251:32 | ... = ... | test.cpp:250:20:250:27 | *out_size | provenance | | -| test.cpp:251:18:251:31 | *call to getenv | test.cpp:251:2:251:32 | ... = ... | provenance | TaintFunction | -| test.cpp:259:15:259:18 | call to atoi | test.cpp:263:11:263:29 | ... * ... | provenance | | -| test.cpp:259:20:259:33 | *call to getenv | test.cpp:259:15:259:18 | call to atoi | provenance | TaintFunction | -| test.cpp:289:17:289:20 | get_size output argument | test.cpp:291:11:291:28 | ... * ... | provenance | | -| test.cpp:305:18:305:21 | get_size output argument | test.cpp:308:10:308:27 | ... * ... | provenance | | -| test.cpp:353:13:353:16 | call to atoi | test.cpp:355:35:355:38 | size | provenance | | -| test.cpp:353:13:353:16 | call to atoi | test.cpp:356:35:356:38 | size | provenance | | -| test.cpp:353:18:353:31 | *call to getenv | test.cpp:353:13:353:16 | call to atoi | provenance | TaintFunction | +| test.cpp:218:8:218:23 | *get_tainted_size | test.cpp:250:9:250:24 | call to get_tainted_size | provenance | | +| test.cpp:220:9:220:42 | ... * ... | test.cpp:218:8:218:23 | *get_tainted_size | provenance | | +| test.cpp:220:14:220:27 | *call to getenv | test.cpp:220:9:220:42 | ... * ... | provenance | TaintFunction | +| test.cpp:239:21:239:21 | s | test.cpp:240:21:240:21 | s | provenance | | +| test.cpp:246:19:246:52 | ... * ... | test.cpp:248:9:248:18 | local_size | provenance | | +| test.cpp:246:19:246:52 | ... * ... | test.cpp:254:11:254:20 | local_size | provenance | | +| test.cpp:246:19:246:52 | ... * ... | test.cpp:256:10:256:19 | local_size | provenance | | +| test.cpp:246:24:246:37 | *call to getenv | test.cpp:246:19:246:52 | ... * ... | provenance | TaintFunction | +| test.cpp:256:10:256:19 | local_size | test.cpp:239:21:239:21 | s | provenance | | +| test.cpp:259:20:259:27 | *out_size | test.cpp:298:17:298:20 | get_size output argument | provenance | | +| test.cpp:259:20:259:27 | *out_size | test.cpp:314:18:314:21 | get_size output argument | provenance | | +| test.cpp:260:2:260:32 | ... = ... | test.cpp:259:20:259:27 | *out_size | provenance | | +| test.cpp:260:18:260:31 | *call to getenv | test.cpp:260:2:260:32 | ... = ... | provenance | TaintFunction | +| test.cpp:268:15:268:18 | call to atoi | test.cpp:272:11:272:29 | ... * ... | provenance | | +| test.cpp:268:20:268:33 | *call to getenv | test.cpp:268:15:268:18 | call to atoi | provenance | TaintFunction | +| test.cpp:298:17:298:20 | get_size output argument | test.cpp:300:11:300:28 | ... * ... | provenance | | +| test.cpp:314:18:314:21 | get_size output argument | test.cpp:317:10:317:27 | ... * ... | provenance | | +| test.cpp:362:13:362:16 | call to atoi | test.cpp:364:35:364:38 | size | provenance | | +| test.cpp:362:13:362:16 | call to atoi | test.cpp:365:35:365:38 | size | provenance | | +| test.cpp:362:18:362:31 | *call to getenv | test.cpp:362:13:362:16 | call to atoi | provenance | TaintFunction | nodes | test.cpp:39:27:39:30 | **argv | semmle.label | **argv | | test.cpp:40:16:40:19 | call to atoi | semmle.label | call to atoi | @@ -52,31 +52,31 @@ nodes | test.cpp:148:15:148:18 | call to atol | semmle.label | call to atol | | test.cpp:148:20:148:33 | *call to getenv | semmle.label | *call to getenv | | test.cpp:152:11:152:28 | ... * ... | semmle.label | ... * ... | -| test.cpp:209:8:209:23 | *get_tainted_size | semmle.label | *get_tainted_size | -| test.cpp:211:9:211:42 | ... * ... | semmle.label | ... * ... | -| test.cpp:211:14:211:27 | *call to getenv | semmle.label | *call to getenv | -| test.cpp:230:21:230:21 | s | semmle.label | s | -| test.cpp:231:21:231:21 | s | semmle.label | s | -| test.cpp:237:19:237:52 | ... * ... | semmle.label | ... * ... | -| test.cpp:237:24:237:37 | *call to getenv | semmle.label | *call to getenv | -| test.cpp:239:9:239:18 | local_size | semmle.label | local_size | -| test.cpp:241:9:241:24 | call to get_tainted_size | semmle.label | call to get_tainted_size | -| test.cpp:245:11:245:20 | local_size | semmle.label | local_size | -| test.cpp:247:10:247:19 | local_size | semmle.label | local_size | -| test.cpp:250:20:250:27 | *out_size | semmle.label | *out_size | -| test.cpp:251:2:251:32 | ... = ... | semmle.label | ... = ... | -| test.cpp:251:18:251:31 | *call to getenv | semmle.label | *call to getenv | -| test.cpp:259:15:259:18 | call to atoi | semmle.label | call to atoi | -| test.cpp:259:20:259:33 | *call to getenv | semmle.label | *call to getenv | -| test.cpp:263:11:263:29 | ... * ... | semmle.label | ... * ... | -| test.cpp:289:17:289:20 | get_size output argument | semmle.label | get_size output argument | -| test.cpp:291:11:291:28 | ... * ... | semmle.label | ... * ... | -| test.cpp:305:18:305:21 | get_size output argument | semmle.label | get_size output argument | -| test.cpp:308:10:308:27 | ... * ... | semmle.label | ... * ... | -| test.cpp:353:13:353:16 | call to atoi | semmle.label | call to atoi | -| test.cpp:353:18:353:31 | *call to getenv | semmle.label | *call to getenv | -| test.cpp:355:35:355:38 | size | semmle.label | size | -| test.cpp:356:35:356:38 | size | semmle.label | size | +| test.cpp:218:8:218:23 | *get_tainted_size | semmle.label | *get_tainted_size | +| test.cpp:220:9:220:42 | ... * ... | semmle.label | ... * ... | +| test.cpp:220:14:220:27 | *call to getenv | semmle.label | *call to getenv | +| test.cpp:239:21:239:21 | s | semmle.label | s | +| test.cpp:240:21:240:21 | s | semmle.label | s | +| test.cpp:246:19:246:52 | ... * ... | semmle.label | ... * ... | +| test.cpp:246:24:246:37 | *call to getenv | semmle.label | *call to getenv | +| test.cpp:248:9:248:18 | local_size | semmle.label | local_size | +| test.cpp:250:9:250:24 | call to get_tainted_size | semmle.label | call to get_tainted_size | +| test.cpp:254:11:254:20 | local_size | semmle.label | local_size | +| test.cpp:256:10:256:19 | local_size | semmle.label | local_size | +| test.cpp:259:20:259:27 | *out_size | semmle.label | *out_size | +| test.cpp:260:2:260:32 | ... = ... | semmle.label | ... = ... | +| test.cpp:260:18:260:31 | *call to getenv | semmle.label | *call to getenv | +| test.cpp:268:15:268:18 | call to atoi | semmle.label | call to atoi | +| test.cpp:268:20:268:33 | *call to getenv | semmle.label | *call to getenv | +| test.cpp:272:11:272:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:298:17:298:20 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:300:11:300:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:314:18:314:21 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:317:10:317:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:362:13:362:16 | call to atoi | semmle.label | call to atoi | +| test.cpp:362:18:362:31 | *call to getenv | semmle.label | *call to getenv | +| test.cpp:364:35:364:38 | size | semmle.label | size | +| test.cpp:365:35:365:38 | size | semmle.label | size | subpaths #select | test.cpp:43:31:43:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:43:38:43:44 | tainted | This allocation size is derived from $@ and might overflow. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) | @@ -88,12 +88,12 @@ subpaths | test.cpp:128:17:128:22 | call to malloc | test.cpp:124:18:124:31 | *call to getenv | test.cpp:128:24:128:41 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:124:18:124:31 | *call to getenv | user input (an environment variable) | | test.cpp:135:3:135:8 | call to malloc | test.cpp:133:19:133:32 | *call to getenv | test.cpp:135:10:135:27 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:133:19:133:32 | *call to getenv | user input (an environment variable) | | test.cpp:152:4:152:9 | call to malloc | test.cpp:148:20:148:33 | *call to getenv | test.cpp:152:11:152:28 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:148:20:148:33 | *call to getenv | user input (an environment variable) | -| test.cpp:231:14:231:19 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:231:21:231:21 | s | This allocation size is derived from $@ and might overflow. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) | -| test.cpp:239:2:239:7 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:239:9:239:18 | local_size | This allocation size is derived from $@ and might overflow. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) | -| test.cpp:241:2:241:7 | call to malloc | test.cpp:211:14:211:27 | *call to getenv | test.cpp:241:9:241:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow. | test.cpp:211:14:211:27 | *call to getenv | user input (an environment variable) | -| test.cpp:245:2:245:9 | call to my_alloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:245:11:245:20 | local_size | This allocation size is derived from $@ and might overflow. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) | -| test.cpp:263:4:263:9 | call to malloc | test.cpp:259:20:259:33 | *call to getenv | test.cpp:263:11:263:29 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:259:20:259:33 | *call to getenv | user input (an environment variable) | -| test.cpp:291:4:291:9 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:291:11:291:28 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) | -| test.cpp:308:3:308:8 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:308:10:308:27 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) | -| test.cpp:355:25:355:33 | call to MyMalloc1 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:355:35:355:38 | size | This allocation size is derived from $@ and might overflow. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) | -| test.cpp:356:25:356:33 | call to MyMalloc2 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:356:35:356:38 | size | This allocation size is derived from $@ and might overflow. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) | +| test.cpp:240:14:240:19 | call to malloc | test.cpp:246:24:246:37 | *call to getenv | test.cpp:240:21:240:21 | s | This allocation size is derived from $@ and might overflow. | test.cpp:246:24:246:37 | *call to getenv | user input (an environment variable) | +| test.cpp:248:2:248:7 | call to malloc | test.cpp:246:24:246:37 | *call to getenv | test.cpp:248:9:248:18 | local_size | This allocation size is derived from $@ and might overflow. | test.cpp:246:24:246:37 | *call to getenv | user input (an environment variable) | +| test.cpp:250:2:250:7 | call to malloc | test.cpp:220:14:220:27 | *call to getenv | test.cpp:250:9:250:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow. | test.cpp:220:14:220:27 | *call to getenv | user input (an environment variable) | +| test.cpp:254:2:254:9 | call to my_alloc | test.cpp:246:24:246:37 | *call to getenv | test.cpp:254:11:254:20 | local_size | This allocation size is derived from $@ and might overflow. | test.cpp:246:24:246:37 | *call to getenv | user input (an environment variable) | +| test.cpp:272:4:272:9 | call to malloc | test.cpp:268:20:268:33 | *call to getenv | test.cpp:272:11:272:29 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:268:20:268:33 | *call to getenv | user input (an environment variable) | +| test.cpp:300:4:300:9 | call to malloc | test.cpp:260:18:260:31 | *call to getenv | test.cpp:300:11:300:28 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:260:18:260:31 | *call to getenv | user input (an environment variable) | +| test.cpp:317:3:317:8 | call to malloc | test.cpp:260:18:260:31 | *call to getenv | test.cpp:317:10:317:27 | ... * ... | This allocation size is derived from $@ and might overflow. | test.cpp:260:18:260:31 | *call to getenv | user input (an environment variable) | +| test.cpp:364:25:364:33 | call to MyMalloc1 | test.cpp:362:18:362:31 | *call to getenv | test.cpp:364:35:364:38 | size | This allocation size is derived from $@ and might overflow. | test.cpp:362:18:362:31 | *call to getenv | user input (an environment variable) | +| test.cpp:365:25:365:33 | call to MyMalloc2 | test.cpp:362:18:362:31 | *call to getenv | test.cpp:365:35:365:38 | size | This allocation size is derived from $@ and might overflow. | test.cpp:362:18:362:31 | *call to getenv | user input (an environment variable) | 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 69cb2411b9fcd..f90f8f9c072cc 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 @@ -180,6 +180,15 @@ void more_bounded_tests() { } } + { + int size = atoi(getenv("USER")); + + if (size % 100) + { + malloc(size * sizeof(int)); // GOOD + } + } + { int size = atoi(getenv("USER"));