From 14aff5c94c2e838b51fd1e50bee3943aea5e17eb Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 20 Mar 2024 14:54:53 +0000 Subject: [PATCH 1/2] C++: Convert 'cpp/missing-check-scanf' to a path-problem query. --- cpp/ql/src/Critical/MissingCheckScanf.ql | 29 ++-- .../MissingCheckScanf.expected | 154 ++++++++++++++++-- 2 files changed, 155 insertions(+), 28 deletions(-) diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql index 3418fb8485c8..ba09aae2a738 100644 --- a/cpp/ql/src/Critical/MissingCheckScanf.ql +++ b/cpp/ql/src/Critical/MissingCheckScanf.ql @@ -2,7 +2,7 @@ * @name Missing return-value check for a 'scanf'-like function * @description Failing to check that a call to 'scanf' actually writes to an * output variable can lead to unexpected behavior at reading time. - * @kind problem + * @kind path-problem * @problem.severity warning * @security-severity 7.5 * @precision medium @@ -20,6 +20,7 @@ import semmle.code.cpp.dataflow.new.DataFlow::DataFlow import semmle.code.cpp.ir.IR import semmle.code.cpp.ir.ValueNumbering import ScanfChecks +import ScanfToUseFlow::PathGraph /** * Holds if `n` represents an uninitialized stack-allocated variable, or a @@ -118,10 +119,13 @@ module ScanfToUseFlow = Global; * Holds if `source` is the `index`'th argument to the `scanf`-like call `call`, and `sink` is * a dataflow node that represents the expression `e`. */ -predicate hasFlow(Node source, ScanfFunctionCall call, int index, Node sink, Expr e) { - isSource(call, index, source, _) and - ScanfToUseFlow::flow(source, sink) and - isSink(sink, e) +predicate flowPath( + ScanfToUseFlow::PathNode source, ScanfFunctionCall call, int index, ScanfToUseFlow::PathNode sink, + Expr e +) { + isSource(call, index, source.getNode(), _) and + ScanfToUseFlow::flowPath(source, sink) and + isSink(sink.getNode(), e) } /** @@ -143,9 +147,12 @@ int getMinimumGuardConstant(ScanfFunctionCall call, int index) { * Holds the access to `e` isn't guarded by a check that ensures that `call` returned * at least `minGuard`. */ -predicate hasNonGuardedAccess(ScanfFunctionCall call, Expr e, int minGuard) { +predicate hasNonGuardedAccess( + ScanfToUseFlow::PathNode source, ScanfFunctionCall call, ScanfToUseFlow::PathNode sink, Expr e, + int minGuard +) { exists(int index | - hasFlow(_, call, index, _, e) and + flowPath(source, call, index, sink, e) and minGuard = getMinimumGuardConstant(call, index) | not exists(int value | @@ -173,9 +180,11 @@ BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) { ) } -from ScanfFunctionCall call, Expr e, int minGuard -where hasNonGuardedAccess(call, e, minGuard) -select e, +from + ScanfToUseFlow::PathNode source, ScanfToUseFlow::PathNode sink, ScanfFunctionCall call, Expr e, + int minGuard +where hasNonGuardedAccess(source, call, sink, e, minGuard) +select e, source, sink, "This variable is read, but may not have been written. " + "It should be guarded by a check that the $@ returns at least " + minGuard + ".", call, call.toString() diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected index 69f9ab820eb5..8bb2c9643a9f 100644 --- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected +++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected @@ -1,18 +1,136 @@ -| test.cpp:35:7:35:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:34:3:34:7 | call to scanf | call to scanf | -| test.cpp:68:7:68:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:67:3:67:7 | call to scanf | call to scanf | -| test.cpp:80:7:80:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:79:3:79:7 | call to scanf | call to scanf | -| test.cpp:90:7:90:8 | * ... | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:89:3:89:7 | call to scanf | call to scanf | -| test.cpp:98:7:98:8 | * ... | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:97:3:97:7 | call to scanf | call to scanf | -| test.cpp:108:7:108:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:107:3:107:8 | call to fscanf | call to fscanf | -| test.cpp:115:7:115:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:114:3:114:8 | call to sscanf | call to sscanf | -| test.cpp:224:8:224:8 | j | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:221:7:221:11 | call to scanf | call to scanf | -| test.cpp:248:9:248:9 | d | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:246:25:246:29 | call to scanf | call to scanf | -| test.cpp:252:9:252:9 | d | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:250:14:250:18 | call to scanf | call to scanf | -| test.cpp:272:7:272:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:271:3:271:7 | call to scanf | call to scanf | -| test.cpp:280:7:280:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:279:3:279:7 | call to scanf | call to scanf | -| test.cpp:292:7:292:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:291:3:291:7 | call to scanf | call to scanf | -| test.cpp:404:25:404:25 | u | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:403:6:403:11 | call to sscanf | call to sscanf | -| test.cpp:416:7:416:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:413:7:413:11 | call to scanf | call to scanf | -| test.cpp:423:7:423:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:420:7:420:11 | call to scanf | call to scanf | -| test.cpp:460:6:460:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:455:12:455:17 | call to sscanf | call to sscanf | -| test.cpp:474:6:474:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:467:8:467:12 | call to scanf | call to scanf | +edges +| test.cpp:34:15:34:16 | scanf output argument | test.cpp:35:7:35:7 | i | provenance | | +| test.cpp:41:19:41:20 | scanf output argument | test.cpp:43:8:43:8 | i | provenance | | +| test.cpp:58:19:58:20 | scanf output argument | test.cpp:60:8:60:8 | i | provenance | | +| test.cpp:67:15:67:16 | scanf output argument | test.cpp:68:7:68:7 | i | provenance | | +| test.cpp:70:19:70:20 | scanf output argument | test.cpp:72:8:72:8 | i | provenance | | +| test.cpp:79:15:79:16 | scanf output argument | test.cpp:80:7:80:7 | i | provenance | | +| test.cpp:89:15:89:15 | scanf output argument | test.cpp:90:7:90:8 | * ... | provenance | | +| test.cpp:97:15:97:15 | scanf output argument | test.cpp:98:7:98:8 | * ... | provenance | | +| test.cpp:107:32:107:33 | fscanf output argument | test.cpp:108:7:108:7 | i | provenance | | +| test.cpp:114:32:114:33 | sscanf output argument | test.cpp:115:7:115:7 | i | provenance | | +| test.cpp:121:38:121:39 | _scanf_l output argument | test.cpp:123:8:123:8 | i | provenance | | +| test.cpp:132:19:132:20 | scanf output argument | test.cpp:134:8:134:8 | i | provenance | | +| test.cpp:141:19:141:20 | scanf output argument | test.cpp:143:8:143:8 | i | provenance | | +| test.cpp:150:23:150:24 | scanf output argument | test.cpp:154:9:154:9 | i | provenance | | +| test.cpp:181:19:181:20 | scanf output argument | test.cpp:185:8:185:8 | i | provenance | | +| test.cpp:193:19:193:20 | scanf output argument | test.cpp:197:8:197:8 | i | provenance | | +| test.cpp:211:22:211:23 | scanf output argument | test.cpp:213:8:213:8 | i | provenance | | +| test.cpp:221:22:221:23 | scanf output argument | test.cpp:223:8:223:8 | i | provenance | | +| test.cpp:221:26:221:27 | scanf output argument | test.cpp:224:8:224:8 | j | provenance | | +| test.cpp:231:22:231:23 | scanf output argument | test.cpp:233:8:233:8 | i | provenance | | +| test.cpp:231:26:231:27 | scanf output argument | test.cpp:234:8:234:8 | j | provenance | | +| test.cpp:246:44:246:45 | scanf output argument | test.cpp:248:9:248:9 | d | provenance | | +| test.cpp:250:33:250:34 | scanf output argument | test.cpp:252:9:252:9 | d | provenance | | +| test.cpp:271:15:271:16 | scanf output argument | test.cpp:272:7:272:7 | i | provenance | | +| test.cpp:279:15:279:16 | scanf output argument | test.cpp:280:7:280:7 | i | provenance | | +| test.cpp:291:15:291:16 | scanf output argument | test.cpp:292:7:292:7 | i | provenance | | +| test.cpp:325:34:325:35 | sscanf output argument | test.cpp:327:8:327:8 | i | provenance | | +| test.cpp:325:38:325:39 | sscanf output argument | test.cpp:328:8:328:8 | j | provenance | | +| test.cpp:335:22:335:23 | scanf output argument | test.cpp:337:8:337:8 | i | provenance | | +| test.cpp:344:23:344:24 | scanf output argument | test.cpp:346:8:346:8 | i | provenance | | +| test.cpp:353:26:353:27 | scanf output argument | test.cpp:354:8:354:8 | d | provenance | | +| test.cpp:353:30:353:31 | scanf output argument | test.cpp:355:8:355:8 | n | provenance | | +| test.cpp:362:62:362:63 | sscanf output argument | test.cpp:364:17:364:17 | n | provenance | | +| test.cpp:403:29:403:30 | sscanf output argument | test.cpp:404:18:404:25 | u | provenance | | +| test.cpp:413:19:413:20 | scanf output argument | test.cpp:416:7:416:7 | i | provenance | | +| test.cpp:420:19:420:20 | scanf output argument | test.cpp:423:7:423:7 | i | provenance | | +| test.cpp:455:41:455:46 | sscanf output argument | test.cpp:460:6:460:10 | value | provenance | | +| test.cpp:467:20:467:25 | scanf output argument | test.cpp:474:6:474:10 | value | provenance | | +nodes +| test.cpp:34:15:34:16 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:35:7:35:7 | i | semmle.label | i | +| test.cpp:41:19:41:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:43:8:43:8 | i | semmle.label | i | +| test.cpp:58:19:58:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:60:8:60:8 | i | semmle.label | i | +| test.cpp:67:15:67:16 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:68:7:68:7 | i | semmle.label | i | +| test.cpp:70:19:70:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:72:8:72:8 | i | semmle.label | i | +| test.cpp:79:15:79:16 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:80:7:80:7 | i | semmle.label | i | +| test.cpp:89:15:89:15 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:90:7:90:8 | * ... | semmle.label | * ... | +| test.cpp:97:15:97:15 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:98:7:98:8 | * ... | semmle.label | * ... | +| test.cpp:107:32:107:33 | fscanf output argument | semmle.label | fscanf output argument | +| test.cpp:108:7:108:7 | i | semmle.label | i | +| test.cpp:114:32:114:33 | sscanf output argument | semmle.label | sscanf output argument | +| test.cpp:115:7:115:7 | i | semmle.label | i | +| test.cpp:121:38:121:39 | _scanf_l output argument | semmle.label | _scanf_l output argument | +| test.cpp:123:8:123:8 | i | semmle.label | i | +| test.cpp:132:19:132:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:134:8:134:8 | i | semmle.label | i | +| test.cpp:141:19:141:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:143:8:143:8 | i | semmle.label | i | +| test.cpp:150:23:150:24 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:154:9:154:9 | i | semmle.label | i | +| test.cpp:181:19:181:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:185:8:185:8 | i | semmle.label | i | +| test.cpp:193:19:193:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:197:8:197:8 | i | semmle.label | i | +| test.cpp:211:22:211:23 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:213:8:213:8 | i | semmle.label | i | +| test.cpp:221:22:221:23 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:221:26:221:27 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:223:8:223:8 | i | semmle.label | i | +| test.cpp:224:8:224:8 | j | semmle.label | j | +| test.cpp:231:22:231:23 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:231:26:231:27 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:233:8:233:8 | i | semmle.label | i | +| test.cpp:234:8:234:8 | j | semmle.label | j | +| test.cpp:246:44:246:45 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:248:9:248:9 | d | semmle.label | d | +| test.cpp:250:33:250:34 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:252:9:252:9 | d | semmle.label | d | +| test.cpp:271:15:271:16 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:272:7:272:7 | i | semmle.label | i | +| test.cpp:279:15:279:16 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:280:7:280:7 | i | semmle.label | i | +| test.cpp:291:15:291:16 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:292:7:292:7 | i | semmle.label | i | +| test.cpp:325:34:325:35 | sscanf output argument | semmle.label | sscanf output argument | +| test.cpp:325:38:325:39 | sscanf output argument | semmle.label | sscanf output argument | +| test.cpp:327:8:327:8 | i | semmle.label | i | +| test.cpp:328:8:328:8 | j | semmle.label | j | +| test.cpp:335:22:335:23 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:337:8:337:8 | i | semmle.label | i | +| test.cpp:344:23:344:24 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:346:8:346:8 | i | semmle.label | i | +| test.cpp:353:26:353:27 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:353:30:353:31 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:354:8:354:8 | d | semmle.label | d | +| test.cpp:355:8:355:8 | n | semmle.label | n | +| test.cpp:362:62:362:63 | sscanf output argument | semmle.label | sscanf output argument | +| test.cpp:364:17:364:17 | n | semmle.label | n | +| test.cpp:403:29:403:30 | sscanf output argument | semmle.label | sscanf output argument | +| test.cpp:404:18:404:25 | u | semmle.label | u | +| test.cpp:413:19:413:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:416:7:416:7 | i | semmle.label | i | +| test.cpp:420:19:420:20 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:423:7:423:7 | i | semmle.label | i | +| test.cpp:455:41:455:46 | sscanf output argument | semmle.label | sscanf output argument | +| test.cpp:460:6:460:10 | value | semmle.label | value | +| test.cpp:467:20:467:25 | scanf output argument | semmle.label | scanf output argument | +| test.cpp:474:6:474:10 | value | semmle.label | value | +subpaths +#select +| test.cpp:35:7:35:7 | i | test.cpp:34:15:34:16 | scanf output argument | test.cpp:35:7:35:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:34:3:34:7 | call to scanf | call to scanf | +| test.cpp:68:7:68:7 | i | test.cpp:67:15:67:16 | scanf output argument | test.cpp:68:7:68:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:67:3:67:7 | call to scanf | call to scanf | +| test.cpp:80:7:80:7 | i | test.cpp:79:15:79:16 | scanf output argument | test.cpp:80:7:80:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:79:3:79:7 | call to scanf | call to scanf | +| test.cpp:90:7:90:8 | * ... | test.cpp:89:15:89:15 | scanf output argument | test.cpp:90:7:90:8 | * ... | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:89:3:89:7 | call to scanf | call to scanf | +| test.cpp:98:7:98:8 | * ... | test.cpp:97:15:97:15 | scanf output argument | test.cpp:98:7:98:8 | * ... | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:97:3:97:7 | call to scanf | call to scanf | +| test.cpp:108:7:108:7 | i | test.cpp:107:32:107:33 | fscanf output argument | test.cpp:108:7:108:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:107:3:107:8 | call to fscanf | call to fscanf | +| test.cpp:115:7:115:7 | i | test.cpp:114:32:114:33 | sscanf output argument | test.cpp:115:7:115:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:114:3:114:8 | call to sscanf | call to sscanf | +| test.cpp:224:8:224:8 | j | test.cpp:221:26:221:27 | scanf output argument | test.cpp:224:8:224:8 | j | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:221:7:221:11 | call to scanf | call to scanf | +| test.cpp:248:9:248:9 | d | test.cpp:246:44:246:45 | scanf output argument | test.cpp:248:9:248:9 | d | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:246:25:246:29 | call to scanf | call to scanf | +| test.cpp:252:9:252:9 | d | test.cpp:250:33:250:34 | scanf output argument | test.cpp:252:9:252:9 | d | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 2. | test.cpp:250:14:250:18 | call to scanf | call to scanf | +| test.cpp:272:7:272:7 | i | test.cpp:271:15:271:16 | scanf output argument | test.cpp:272:7:272:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:271:3:271:7 | call to scanf | call to scanf | +| test.cpp:280:7:280:7 | i | test.cpp:279:15:279:16 | scanf output argument | test.cpp:280:7:280:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:279:3:279:7 | call to scanf | call to scanf | +| test.cpp:292:7:292:7 | i | test.cpp:291:15:291:16 | scanf output argument | test.cpp:292:7:292:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:291:3:291:7 | call to scanf | call to scanf | +| test.cpp:404:25:404:25 | u | test.cpp:403:29:403:30 | sscanf output argument | test.cpp:404:18:404:25 | u | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:403:6:403:11 | call to sscanf | call to sscanf | +| test.cpp:416:7:416:7 | i | test.cpp:413:19:413:20 | scanf output argument | test.cpp:416:7:416:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:413:7:413:11 | call to scanf | call to scanf | +| test.cpp:423:7:423:7 | i | test.cpp:420:19:420:20 | scanf output argument | test.cpp:423:7:423:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:420:7:420:11 | call to scanf | call to scanf | +| test.cpp:460:6:460:10 | value | test.cpp:455:41:455:46 | sscanf output argument | test.cpp:460:6:460:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:455:12:455:17 | call to sscanf | call to sscanf | +| test.cpp:474:6:474:10 | value | test.cpp:467:20:467:25 | scanf output argument | test.cpp:474:6:474:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:467:8:467:12 | call to scanf | call to scanf | From 96cd259eda97c163d19cd70b4253900bcdd1c7fc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 20 Mar 2024 14:56:39 +0000 Subject: [PATCH 2/2] C++: Add change note. --- .../2024-03-20-missing-check-scanf-path-problem.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2024-03-20-missing-check-scanf-path-problem.md diff --git a/cpp/ql/src/change-notes/2024-03-20-missing-check-scanf-path-problem.md b/cpp/ql/src/change-notes/2024-03-20-missing-check-scanf-path-problem.md new file mode 100644 index 000000000000..12a185add1ea --- /dev/null +++ b/cpp/ql/src/change-notes/2024-03-20-missing-check-scanf-path-problem.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Missing return-value check for a 'scanf'-like function" query (`cpp/missing-check-scanf`) has been converted to a `path-problem` query. \ No newline at end of file