From 843fce4bcd5b83f60109f8b72d9a6d505e4f493c Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 6 Sep 2022 09:12:18 +0200 Subject: [PATCH 01/49] expand localFieldStep to use access-paths, and build access-paths in more cases --- .../semmle/javascript/GlobalAccessPaths.qll | 96 ++++++++++++++++ .../semmle/javascript/dataflow/DataFlow.qll | 4 +- .../UnsafeShellCommandConstruction.expected | 10 ++ .../query-tests/Security/CWE-078/lib/lib.js | 2 +- .../UnsafeCodeConstruction.expected | 80 +++++++++++++ .../CWE-094/CodeInjection/lib/index.js | 106 ++++++++++++++++++ .../PrototypePollutingAssignment.expected | 2 + 7 files changed, 297 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index e617f597870..421d5a88860 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -233,6 +233,8 @@ module AccessPath { baseName = fromReference(write.getBase(), root) or baseName = fromRhs(write.getBase(), root) + or + baseName = fromRhs(GetLaterAccess::getLaterBaseAccess(write), root) ) or exists(GlobalVariable var | @@ -266,6 +268,100 @@ module AccessPath { ) } + /** A module for computing an access to a variable that happens after a write to that same variable */ + private module GetLaterAccess { + /** + * Gets an access to a variable that is written to in `write`, where the access is after the write. + * + * This allows `fromRhs` to compute an access path for e.g. the below example: + * ```JavaScript + * function Template(text, opts) { + * opts = opts || {}; + * var options = {}; + * options.varName = taint; + * this.opts = options; // this.opts.varName is now available + * } + * ``` + */ + pragma[noopt] + DataFlow::Node getLaterBaseAccess(DataFlow::PropWrite write) { + exists( + ControlFlowNode writeNode, BindingPattern access, VarRef otherAccess, Variable variable, + StmtContainer container + | + access = getBaseVar(write) and + writeNode = write.getWriteNode() and + access = getAnAccessInContainer(variable, container, true) and + variable = getARelevantVariable() and // manual magic + otherAccess = getAnAccessInContainer(variable, container, false) and + access != otherAccess and + result.asExpr() = otherAccess + | + exists(BasicBlock bb, int i, int j | + bb.getNode(i) = writeNode and + bb.getNode(j) = otherAccess and + i < j + ) + or + otherAccess.getBasicBlock() = getASuccessorBBThatReadsVar(write) // more manual magic - outlined into a helper predicate. + ) + } + + /** Gets a variable ref that `write` writes a property to. */ + VarRef getBaseVar(DataFlow::PropWrite write) { + result = write.getBase().asExpr() + or + exists(Assignment assign | + write.getBase().asExpr() = assign.getRhs() and + result = assign.getLhs() + ) + or + exists(VariableDeclarator decl | + write.getBase().asExpr() = decl.getInit() and + result = decl.getBindingPattern() + ) + } + + /** Gets an access to `var` inside `container` where `usedInWrite` indicates whether the access is the base of a property write. */ + private VarRef getAnAccessInContainer(Variable var, StmtContainer container, boolean usedInWrite) { + result.getVariable() = var and + result.getContainer() = container and + var.isLocal() and + if result = getBaseVar(_) then usedInWrite = true else usedInWrite = false + } + + /** Gets a variable that is relevant for the computations in the `GetLaterAccess` module. */ + private Variable getARelevantVariable() { + // The variable might be used where `getLaterBaseAccess()` is called. + exists(DataFlow::Node node | + exists(fromRhs(node, _)) and + node.asExpr().(VarAccess).getVariable() = result + ) and + // There is a write that writes to the variable. + getBaseVar(_).getVariable() = result and + // It's local. + result.isLocal() and // we skip global variables, because that turns messy quick. + // There is both a "write" and "read" in the same container of the variable. + exists(StmtContainer container | + exists(getAnAccessInContainer(result, container, true)) and // a "write", an access to the variable that is the base of a property reference. + exists(getAnAccessInContainer(result, container, false)) // a "read", an access to the variable that is not the base of a property reference. + ) + } + + /** Gets a basic-block that has a read of the variable that is written to by `write`, where the basicblock occurs after `start`. */ + private ReachableBasicBlock getASuccessorBBThatReadsVar(DataFlow::PropWrite write) { + exists(VarAccess baseExpr, Variable var, ControlFlowNode writeNode | + baseExpr = getBaseVar(write) and + var = baseExpr.getVariable() and + var = getARelevantVariable() and + writeNode = write.getWriteNode() and + writeNode.getBasicBlock().(ReachableBasicBlock).strictlyDominates(result) and + // manual magic. + result = getAnAccessInContainer(getARelevantVariable(), _, false).getBasicBlock() + ) + } + } + /** * Gets a node that refers to the given access path relative to the given `root` node, * or `root` itself if the access path is empty. diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll index 20366a8f2b5..2404fecbd30 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll @@ -1692,10 +1692,10 @@ module DataFlow { */ predicate localFieldStep(DataFlow::Node pred, DataFlow::Node succ) { exists(ClassNode cls, string prop | - pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs() or + pred = AccessPath::getAnAssignmentTo(cls.getADirectSuperClass*().getAReceiverNode(), prop) or pred = cls.getInstanceMethod(prop) | - succ = cls.getAReceiverNode().getAPropertyRead(prop) + succ = AccessPath::getAReferenceTo(cls.getAReceiverNode(), prop) ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected index 1206af38368..9cefca6150c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected @@ -170,6 +170,10 @@ nodes | lib/lib.js:277:23:277:26 | opts | | lib/lib.js:277:23:277:30 | opts.bla | | lib/lib.js:277:23:277:30 | opts.bla | +| lib/lib.js:279:19:279:22 | opts | +| lib/lib.js:279:19:279:26 | opts.bla | +| lib/lib.js:281:23:281:35 | this.opts.bla | +| lib/lib.js:281:23:281:35 | this.opts.bla | | lib/lib.js:307:39:307:42 | name | | lib/lib.js:307:39:307:42 | name | | lib/lib.js:308:23:308:26 | name | @@ -504,8 +508,13 @@ edges | lib/lib.js:268:22:268:24 | obj | lib/lib.js:268:22:268:32 | obj.version | | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts | | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts | +| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts | +| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts | | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla | | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla | +| lib/lib.js:279:19:279:22 | opts | lib/lib.js:279:19:279:26 | opts.bla | +| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla | +| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | @@ -714,6 +723,7 @@ edges | lib/lib.js:261:11:261:33 | "rm -rf ... + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | $@ based on $@ is later used in $@. | lib/lib.js:261:11:261:33 | "rm -rf ... + name | String concatenation | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command | | lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | $@ based on $@ is later used in $@. | lib/lib.js:268:10:268:32 | "rm -rf ... version | String concatenation | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command | | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:30 | opts.bla | $@ based on $@ is later used in $@. | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | String concatenation | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:277:3:277:31 | cp.exec ... ts.bla) | shell command | +| lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:281:23:281:35 | this.opts.bla | $@ based on $@ is later used in $@. | lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | String concatenation | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:281:3:281:36 | cp.exec ... ts.bla) | shell command | | lib/lib.js:308:11:308:26 | "rm -rf " + name | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:308:11:308:26 | "rm -rf " + name | String concatenation | lib/lib.js:307:39:307:42 | name | library input | lib/lib.js:308:3:308:27 | cp.exec ... + name) | shell command | | lib/lib.js:315:10:315:25 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:315:22:315:25 | name | $@ based on $@ is later used in $@. | lib/lib.js:315:10:315:25 | "rm -rf " + name | String concatenation | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:315:2:315:26 | cp.exec ... + name) | shell command | | lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:320:11:320:26 | "rm -rf " + name | String concatenation | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js index d94f430c57f..64b279d7d05 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js @@ -278,7 +278,7 @@ module.exports.Foo = class Foo { this.opts = {}; this.opts.bla = opts.bla - cp.exec("rm -rf " + this.opts.bla); // NOT OK - but FN [INCONSISTENCY] + cp.exec("rm -rf " + this.opts.bla); // NOT OK } } diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected index 49511750e6f..2018b3f45d1 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected @@ -15,6 +15,41 @@ nodes | lib/index.js:19:26:19:29 | data | | lib/index.js:22:7:22:10 | data | | lib/index.js:22:7:22:10 | data | +| lib/index.js:41:32:41:35 | opts | +| lib/index.js:41:32:41:35 | opts | +| lib/index.js:42:3:42:19 | opts | +| lib/index.js:42:10:42:13 | opts | +| lib/index.js:42:10:42:19 | opts \|\| {} | +| lib/index.js:44:21:44:24 | opts | +| lib/index.js:44:21:44:32 | opts.varName | +| lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:86:15:86:19 | taint | +| lib/index.js:86:15:86:19 | taint | +| lib/index.js:87:18:87:22 | taint | +| lib/index.js:89:36:89:40 | taint | +| lib/index.js:93:32:93:36 | taint | +| lib/index.js:98:30:98:34 | taint | +| lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:112:17:112:21 | taint | +| lib/index.js:112:17:112:21 | taint | +| lib/index.js:113:20:113:24 | taint | +| lib/index.js:121:34:121:38 | taint | +| lib/index.js:129:32:129:36 | taint | +| lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:137:23:137:49 | this.op ... dOption | +| lib/index.js:137:23:137:49 | this.op ... dOption | +| lib/index.js:138:23:138:32 | this.taint | +| lib/index.js:138:23:138:32 | this.taint | edges | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | @@ -32,8 +67,53 @@ edges | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | +| lib/index.js:41:32:41:35 | opts | lib/index.js:42:10:42:13 | opts | +| lib/index.js:41:32:41:35 | opts | lib/index.js:42:10:42:13 | opts | +| lib/index.js:42:3:42:19 | opts | lib/index.js:44:21:44:24 | opts | +| lib/index.js:42:10:42:13 | opts | lib/index.js:42:10:42:19 | opts \|\| {} | +| lib/index.js:42:10:42:19 | opts \|\| {} | lib/index.js:42:3:42:19 | opts | +| lib/index.js:44:21:44:24 | opts | lib/index.js:44:21:44:32 | opts.varName | +| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName | +| lib/index.js:86:15:86:19 | taint | lib/index.js:87:18:87:22 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:87:18:87:22 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:89:36:89:40 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:89:36:89:40 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:93:32:93:36 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:93:32:93:36 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:98:30:98:34 | taint | +| lib/index.js:86:15:86:19 | taint | lib/index.js:98:30:98:34 | taint | +| lib/index.js:87:18:87:22 | taint | lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:87:18:87:22 | taint | lib/index.js:106:21:106:30 | this.taint | +| lib/index.js:89:36:89:40 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:89:36:89:40 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | +| lib/index.js:93:32:93:36 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:93:32:93:36 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | +| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | +| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint | +| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint | +| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint | +| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint | +| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | +| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | +| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | #select | lib/index.js:2:21:2:24 | data | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | $@ flows to here and is later $@. | lib/index.js:1:35:1:38 | data | Library input | lib/index.js:2:15:2:30 | "(" + data + ")" | interpreted as code | | lib/index.js:6:26:6:29 | name | lib/index.js:5:35:5:38 | name | lib/index.js:6:26:6:29 | name | $@ flows to here and is later $@. | lib/index.js:5:35:5:38 | name | Library input | lib/index.js:6:17:6:29 | "obj." + name | interpreted as code | | lib/index.js:14:21:14:24 | data | lib/index.js:13:38:13:41 | data | lib/index.js:14:21:14:24 | data | $@ flows to here and is later $@. | lib/index.js:13:38:13:41 | data | Library input | lib/index.js:14:15:14:30 | "(" + data + ")" | interpreted as code | | lib/index.js:22:7:22:10 | data | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | $@ flows to here and is later $@. | lib/index.js:19:26:19:29 | data | Library input | lib/index.js:25:24:25:26 | str | interpreted as code | +| lib/index.js:51:21:51:32 | opts.varName | lib/index.js:41:32:41:35 | opts | lib/index.js:51:21:51:32 | opts.varName | $@ flows to here and is later $@. | lib/index.js:41:32:41:35 | opts | Library input | lib/index.js:51:10:51:52 | " var ... ing();" | interpreted as code | +| lib/index.js:103:21:103:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | $@ flows to here and is later $@. | lib/index.js:86:15:86:19 | taint | Library input | lib/index.js:103:10:103:67 | " var ... ing();" | interpreted as code | +| lib/index.js:104:21:104:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | $@ flows to here and is later $@. | lib/index.js:86:15:86:19 | taint | Library input | lib/index.js:104:10:104:67 | " var ... ing();" | interpreted as code | +| lib/index.js:105:21:105:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | $@ flows to here and is later $@. | lib/index.js:86:15:86:19 | taint | Library input | lib/index.js:105:10:105:67 | " var ... ing();" | interpreted as code | +| lib/index.js:106:21:106:30 | this.taint | lib/index.js:86:15:86:19 | taint | lib/index.js:106:21:106:30 | this.taint | $@ flows to here and is later $@. | lib/index.js:86:15:86:19 | taint | Library input | lib/index.js:106:10:106:50 | " var ... ing();" | interpreted as code | +| lib/index.js:136:23:136:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | $@ flows to here and is later $@. | lib/index.js:112:17:112:21 | taint | Library input | lib/index.js:136:12:136:69 | " var ... ing();" | interpreted as code | +| lib/index.js:137:23:137:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | $@ flows to here and is later $@. | lib/index.js:112:17:112:21 | taint | Library input | lib/index.js:137:12:137:69 | " var ... ing();" | interpreted as code | +| lib/index.js:138:23:138:32 | this.taint | lib/index.js:112:17:112:21 | taint | lib/index.js:138:23:138:32 | this.taint | $@ flows to here and is later $@. | lib/index.js:112:17:112:21 | taint | Library input | lib/index.js:138:12:138:52 | " var ... ing();" | interpreted as code | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js index 4289ebfc686..9df334c56dc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/lib/index.js @@ -33,3 +33,109 @@ export function greySink(data) { } }); } + +function codeIsAlive() { + new Template().compile(); +} + +export function Template(text, opts) { + opts = opts || {}; + var options = {}; + options.varName = opts.varName; + this.opts = options; +} + +Template.prototype = { + compile: function () { + var opts = this.opts; + eval(" var " + opts.varName + " = something();"); // NOT OK + }, + // The below are justs tests that ensure the global-access-path computations terminate. + pathsTerminate1: function (node, prev) { + node.tree = { + ancestor: node, + number: rand ? prev.tree.number + 1 : 0, + }; + }, + pathsTerminate2: function (A) { + try { + var B = A.p1; + var C = B.p2; + C.p5 = C; + } catch (ex) {} + }, + pathsTerminate3: function (A) { + var x = foo(); + while (Math.random()) { + x.r = x; + } + }, + pathsTerminate4: function () { + var dest = foo(); + var range = foo(); + while (Math.random() < 0.5) { + range.tabstop = dest; + if (Math.random() < 0.5) { + dest.firstNonLinked = range; + } + } + }, +}; + +export class AccessPathClass { + constructor(taint) { + this.taint = taint; + + var options1 = {taintedOption: taint}; + this.options1 = options1; + + var options2; + options2 = {taintedOption: taint}; + this.options2 = options2; + + var options3; + options3 = {}; + options3.taintedOption = taint; + this.options3 = options3; + } + + doesTaint() { + eval(" var " + this.options1.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options2.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options3.taintedOption + " = something();"); // NOT OK + eval(" var " + this.taint + " = something();"); // NOT OK + } +} + + +export class AccessPathClassBB { + constructor(taint) { + this.taint = taint; + + var options1 = {taintedOption: taint}; + if (Math.random() < 0.5) { console.log("foo"); } + this.options1 = options1; + + var options2; + if (Math.random() < 0.5) { console.log("foo"); } + options2 = {taintedOption: taint}; + if (Math.random() < 0.5) { console.log("foo"); } + this.options2 = options2; + + var options3; + if (Math.random() < 0.5) { console.log("foo"); } + options3 = {}; + if (Math.random() < 0.5) { console.log("foo"); } + options3.taintedOption = taint; + if (Math.random() < 0.5) { console.log("foo"); } + this.options3 = options3; + } + + doesTaint() { + eval(" var " + this.options1.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options2.taintedOption + " = something();"); // NOT OK + eval(" var " + this.options3.taintedOption + " = something();"); // NOT OK + eval(" var " + this.taint + " = something();"); // NOT OK + } + } + \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected index 7705d224298..92cd6bd6c45 100644 --- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected +++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected @@ -248,7 +248,9 @@ edges | lib.js:55:15:55:21 | path[0] | lib.js:55:11:55:22 | obj[path[0]] | | lib.js:59:18:59:18 | s | lib.js:61:17:61:17 | s | | lib.js:59:18:59:18 | s | lib.js:61:17:61:17 | s | +| lib.js:61:17:61:17 | s | lib.js:68:11:68:26 | path | | lib.js:61:17:61:17 | s | lib.js:68:18:68:26 | this.path | +| lib.js:61:17:61:17 | s | lib.js:70:17:70:20 | path | | lib.js:68:11:68:26 | path | lib.js:70:17:70:20 | path | | lib.js:68:18:68:26 | this.path | lib.js:68:11:68:26 | path | | lib.js:70:17:70:20 | path | lib.js:70:17:70:23 | path[0] | From fedf8fc575efe60cbd5570fce624e27254739b4a Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 22 Sep 2022 15:49:29 +0200 Subject: [PATCH 02/49] correct the qldoc Co-authored-by: Asger F --- javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 421d5a88860..96451661c7d 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -268,7 +268,7 @@ module AccessPath { ) } - /** A module for computing an access to a variable that happens after a write to that same variable */ + /** A module for computing an access to a variable that happens after a property has been written onto it */ private module GetLaterAccess { /** * Gets an access to a variable that is written to in `write`, where the access is after the write. From 5fb44e9dd83754e6bc78bcf8909575289c410587 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 22 Sep 2022 15:57:40 +0200 Subject: [PATCH 03/49] simplify and improve the example for getLaterBaseAccess --- .../ql/lib/semmle/javascript/GlobalAccessPaths.qll | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 96451661c7d..dc28e44ecdd 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -275,11 +275,11 @@ module AccessPath { * * This allows `fromRhs` to compute an access path for e.g. the below example: * ```JavaScript - * function Template(text, opts) { - * opts = opts || {}; - * var options = {}; - * options.varName = taint; - * this.opts = options; // this.opts.varName is now available + * function foo(x) { + * var obj = { + * bar: x // `x` has the access path "foo.bar" starting from the root `this`. + * }; + * this.foo = obj; * } * ``` */ From b16b3c0394e27359053a1f9d0693006160c393de Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 4 Oct 2022 15:01:10 +0200 Subject: [PATCH 04/49] move cwe-078 tests into subfolders --- .../cwe-078/{ => CommandInjection}/CommandInjection.expected | 0 .../cwe-078/{ => CommandInjection}/CommandInjection.qlref | 0 .../security/cwe-078/{ => CommandInjection}/CommandInjection.rb | 0 .../security/cwe-078/{ => KernelOpen}/KernelOpen.expected | 0 .../security/cwe-078/{ => KernelOpen}/KernelOpen.qlref | 0 .../query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.rb | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename ruby/ql/test/query-tests/security/cwe-078/{ => CommandInjection}/CommandInjection.expected (100%) rename ruby/ql/test/query-tests/security/cwe-078/{ => CommandInjection}/CommandInjection.qlref (100%) rename ruby/ql/test/query-tests/security/cwe-078/{ => CommandInjection}/CommandInjection.rb (100%) rename ruby/ql/test/query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.expected (100%) rename ruby/ql/test/query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.qlref (100%) rename ruby/ql/test/query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.rb (100%) diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.qlref b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.qlref similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.qlref rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.qlref diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.rb b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.rb rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb From 99b90789e5a809b906aa7388b2c050a508980a9e Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 14:04:30 +0200 Subject: [PATCH 05/49] add `.shellescape` as a sanitizer for `rb/command-injection` --- .../ruby/security/CommandInjectionCustomizations.qll | 3 +++ .../cwe-078/CommandInjection/CommandInjection.expected | 6 ++++++ .../cwe-078/CommandInjection/CommandInjection.rb | 10 ++++++++++ 3 files changed, 19 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll index e67ae044a00..20cc8d5b26b 100644 --- a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll @@ -49,6 +49,9 @@ module CommandInjection { class ShellwordsEscapeAsSanitizer extends Sanitizer { ShellwordsEscapeAsSanitizer() { this = API::getTopLevelMember("Shellwords").getAMethodCall(["escape", "shellescape"]) + or + // The method is also added as `String#shellescape`. + this.(DataFlow::CallNode).getMethodName() = "shellescape" } } } diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected index b2e4990daec..1d00f293dfd 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected @@ -15,6 +15,8 @@ edges | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:22:82:25 | args : | | CommandInjection.rb:82:22:82:25 | args : | CommandInjection.rb:82:22:82:37 | ...[...] : | | CommandInjection.rb:82:22:82:37 | ...[...] : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | +| CommandInjection.rb:94:16:94:21 | call to params : | CommandInjection.rb:94:16:94:28 | ...[...] : | +| CommandInjection.rb:94:16:94:28 | ...[...] : | CommandInjection.rb:95:16:95:28 | "cat #{...}" | nodes | CommandInjection.rb:6:15:6:20 | call to params : | semmle.label | call to params : | | CommandInjection.rb:6:15:6:26 | ...[...] : | semmle.label | ...[...] : | @@ -37,6 +39,9 @@ nodes | CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" | | CommandInjection.rb:82:22:82:25 | args : | semmle.label | args : | | CommandInjection.rb:82:22:82:37 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:94:16:94:21 | call to params : | semmle.label | call to params : | +| CommandInjection.rb:94:16:94:28 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:95:16:95:28 | "cat #{...}" | semmle.label | "cat #{...}" | subpaths #select | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params : | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value | @@ -51,3 +56,4 @@ subpaths | CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number : | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value | | CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number : | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value | | CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value | +| CommandInjection.rb:95:16:95:28 | "cat #{...}" | CommandInjection.rb:94:16:94:21 | call to params : | CommandInjection.rb:95:16:95:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:94:16:94:21 | call to params | user-provided value | diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb index ed9750128cc..75219e2131c 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb @@ -88,3 +88,13 @@ module Types end end end + +class Foo < ActionController::Base + def create + file = params[:file] + system("cat #{file}") + # .shellescape + system("cat #{file.shellescape}") # OK, because file is shell escaped + + end +end \ No newline at end of file From 75422dfa72238e49bef88353f58724ba8fe8c4e0 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 15:02:18 +0200 Subject: [PATCH 06/49] add library for reasoning about gems and .gemspec files --- ruby/ql/lib/codeql/ruby/frameworks/Core.qll | 1 + .../lib/codeql/ruby/frameworks/core/Gem.qll | 92 +++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll index 73824b123ff..3e61b50dc04 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll @@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow private import codeql.ruby.dataflow.FlowSummary import core.BasicObject::BasicObject import core.Object::Object +import core.Gem::Gem import core.Kernel::Kernel import core.Module import core.Array diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll new file mode 100644 index 00000000000..41daf6ac3b8 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll @@ -0,0 +1,92 @@ +/** + * Provides modeling for the `Gem` module and `.gemspec` files. + */ + +private import ruby +private import Ast +private import codeql.ruby.ApiGraphs + +/** Provides modeling for the `Gem` module and `.gemspec` files. */ +module Gem { + /** A .gemspec file that lists properties of a Ruby gem. */ + class GemSpec instanceof File { + API::Node specCall; + + GemSpec() { + this.getExtension() = "gemspec" and + specCall = API::root().getMember("Gem").getMember("Specification").getMethod("new") and + specCall.getLocation().getFile() = this + } + + /** Gets the name of this .gemspec file. */ + string toString() { result = File.super.getBaseName() } + + /** + * Gets a value of the `name` property of this .gemspec file. + * These properties are set using the `Gem::Specification.new` method. + */ + private Expr getSpecProperty(string key) { + exists(Expr rhs | + rhs = + specCall + .getBlock() + .getParameter(0) + .getMethod(key + "=") + .getParameter(0) + .asSink() + .asExpr() + .getExpr() + .(Ast::AssignExpr) + .getRightOperand() + | + result = rhs + or + // some properties are arrays, we just unfold them + result = rhs.(ArrayLiteral).getAnElement() + ) + } + + /** Gets the name of the gem */ + string getName() { result = getSpecProperty("name").getConstantValue().getString() } + + /** Gets a path that is loaded when the gem is required */ + private string getARequirePath() { + result = getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString() + or + not exists(getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()) and + result = "lib" // the default is "lib" + } + + /** Gets a file that is loaded when the gem is required. */ + private File getAnRequiredFile() { + result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*() + } + + /** Gets a class/module that is exported by this gem. */ + private ModuleBase getAPublicModule() { + result.(Toplevel).getLocation().getFile() = getAnRequiredFile() + or + result = getAPublicModule().getAModule() + or + result = getAPublicModule().getAClass() + or + result = getAPublicModule().getStmt(_).(SingletonClass) + } + + /** Gets a parameter from an exported method, which is an input to this gem. */ + DataFlow::ParameterNode getAnInputParameter() { + exists(MethodBase method | method = getAPublicModule().getAMethod() | + result.getParameter() = method.getAParameter() and + method.isPublic() + ) + } + } + + /** Gets a parameter that is an input to a named gem. */ + DataFlow::ParameterNode getALibraryInput() { + exists(GemSpec spec | + exists(spec.getName()) and // we only consider `.gemspec` files that have a name + result = spec.getAnInputParameter() + ) + } +} From 0d5da42ddd95c62d80ebbaaea437b65cb5a3d87a Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 15:04:50 +0200 Subject: [PATCH 07/49] add a `getName()` utility to `DataFlow::ParameterNode` --- ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index f6ec8c4ab1b..a89e4964b8b 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -147,6 +147,9 @@ class ExprNode extends Node, TExprNode { class ParameterNode extends Node, TParameterNode instanceof ParameterNodeImpl { /** Gets the parameter corresponding to this node, if any. */ final Parameter getParameter() { result = super.getParameter() } + + /** Gets the name of the parameter, if any. */ + final string getName() { result = this.getParameter().(NamedParameter).getName() } } /** From 557dd108964cf9e04c8436a0935dc40820661e84 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 15:07:07 +0200 Subject: [PATCH 08/49] add a `rb/unsafe-shell-command-construction` query --- ...ShellCommandConstructionCustomizations.qll | 108 ++++++++++++++++++ .../UnsafeShellCommandConstructionQuery.qll | 35 ++++++ .../cwe-078/UnsafeShellCommandConstruction.ql | 26 +++++ .../UnsafeShellCommandConstruction.expected | 40 +++++++ .../UnsafeShellCommandConstruction.qlref | 1 + .../impl/sub/notImported.rb | 6 + .../impl/sub/other.rb | 7 ++ .../impl/sub/other2.rb | 5 + .../impl/unsafeShell.rb | 45 ++++++++ .../unsafe-shell.gemspec | 5 + 10 files changed, 278 insertions(+) create mode 100644 ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll create mode 100644 ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll create mode 100644 ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll new file mode 100644 index 00000000000..a235c53462f --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -0,0 +1,108 @@ +/** + * Provides default sources, sinks and sanitizers for reasoning about + * shell command constructed from library input vulnerabilities, as + * well as extension points for adding your own. + */ + +private import codeql.ruby.DataFlow +private import codeql.ruby.DataFlow2 +private import codeql.ruby.ApiGraphs +private import codeql.ruby.frameworks.core.Gem::Gem as Gem +private import codeql.ruby.AST as Ast +private import codeql.ruby.Concepts as Concepts + +module UnsafeShellCommandConstruction { + /** A source for shell command constructed from library input vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** An input parameter to a gem seen as a source. */ + private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode { + LibraryInputAsSource() { + this = Gem::getALibraryInput() and + // we exclude arguments named `cmd` or similar, as they seem to execute commands on purpose + not exists(string name | name = super.getName() | + name = ["cmd", "command"] + or + name.regexpMatch(".*(Cmd|Command)$") + ) + } + } + + /** A sink for shell command constructed from library input vulnerabilities. */ + abstract class Sink extends DataFlow::Node { + /** Gets a description of how the string in this sink was constructed. */ + abstract string describe(); + + /** Gets the dataflow node where the string is constructed. */ + DataFlow::Node getStringConstruction() { result = this } + + /** Gets the dataflow node that executed the string as a shell command. */ + abstract DataFlow::Node getCommandExecution(); + } + + /** A dataflow-configuration for tracking flow from various string constructions to places where those strings are executed as shell commands. */ + class TrackSystemCommand extends DataFlow2::Configuration { + TrackSystemCommand() { this = "StringConcatAsSink::TrackSystemCommand" } + + override predicate isSource(DataFlow::Node source) { + source instanceof TaintedFormat::PrintfStyleCall + or + source.asExpr().getExpr() = + any(Ast::StringLiteral lit | + lit.getComponent(_) instanceof Ast::StringInterpolationComponent + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(Concepts::SystemCommandExecution s | s.isShellInterpreted(sink)) + } + } + + /** Holds if the string constructed at `source` is executed at `shellExec` */ + predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) { + any(TrackSystemCommand conf) + .hasFlow(source, any(DataFlow::Node arg | shellExec.isShellInterpreted(arg))) + } + + /** + * A string constructed from a string-literal (e.g. `"foo #{sink}"`), + * where the resulting string ends up being executed as a shell command. + */ + class StringFormatAsSink extends Sink { + Concepts::SystemCommandExecution s; + Ast::StringLiteral lit; + + StringFormatAsSink() { + isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and + this.asExpr().getExpr() = lit.getComponent(_) + } + + override string describe() { result = "string construction" } + + override DataFlow::Node getCommandExecution() { result = s } + + override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit } + } + + import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat + + /** + * A string constructed from a printf-style call, + * where the resulting string ends up being executed as a shell command. + */ + class TaintedFormatStringAsSink extends Sink { + Concepts::SystemCommandExecution s; + TaintedFormat::PrintfStyleCall call; + + TaintedFormatStringAsSink() { + isUsedAsShellCommand(call, s) and + this = [call.getFormatArgument(_), call.getFormatString()] + } + + override string describe() { result = "formatted string" } + + override DataFlow::Node getCommandExecution() { result = s } + + override DataFlow::Node getStringConstruction() { result = call } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll new file mode 100644 index 00000000000..87b602911ae --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll @@ -0,0 +1,35 @@ +/** + * Provides a taint tracking configuration for reasoning about shell command + * constructed from library input vulnerabilities + * + * Note, for performance reasons: only import this file if `Configuration` is needed, + * otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead. + */ + +import codeql.ruby.DataFlow +import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction +private import codeql.ruby.TaintTracking +private import CommandInjectionCustomizations::CommandInjection as CommandInjection +private import codeql.ruby.dataflow.BarrierGuards + +/** + * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "UnsafeShellCommandConstruction" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { + node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection` + node instanceof StringConstCompareBarrier or + node instanceof StringConstArrayInclusionCallBarrier + } + + // override to require the path doesn't have unmatched return steps + override DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureHasSourceCallContext + } +} diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql new file mode 100644 index 00000000000..53c71cfc314 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql @@ -0,0 +1,26 @@ +/** + * @name Unsafe shell command constructed from library input + * @description Using externally controlled strings in a command line may allow a malicious + * user to change the meaning of the command. + * @kind path-problem + * @problem.severity error + * @security-severity 6.3 + * @precision high + * @id rb/shell-command-constructed-from-input + * @tags correctness + * security + * external/cwe/cwe-078 + * external/cwe/cwe-088 + * external/cwe/cwe-073 + */ + +import codeql.ruby.security.UnsafeShellCommandConstructionQuery +import DataFlow::PathGraph + +from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode +where + config.hasFlowPath(source, sink) and + sinkNode = sink.getNode() +select sinkNode.getStringConstruction(), source, sink, + "This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(), + "library input", sinkNode.getCommandExecution(), "shell command" diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected new file mode 100644 index 00000000000..3cc91bebdd9 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -0,0 +1,40 @@ +edges +| impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | +| impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} | +| impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} | +| impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} | +| impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x | +| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} | +| impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | +| impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | +| impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | +nodes +| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/sub/other2.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/other2.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/sub/other.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/other.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/unsafeShell.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:6:12:6:12 | x : | semmle.label | x : | +| impl/unsafeShell.rb:7:32:7:32 | x | semmle.label | x | +| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | semmle.label | innocent_file_path : | +| impl/unsafeShell.rb:20:21:20:41 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:23:15:23:23 | file_path : | semmle.label | file_path : | +| impl/unsafeShell.rb:26:19:26:30 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:33:12:33:17 | target : | semmle.label | target : | +| impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:37:10:37:10 | x : | semmle.label | x : | +| impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} | +subpaths +#select +| impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command | +| impl/sub/other2.rb:3:14:3:28 | "cat #{...}" | impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other2.rb:2:12:2:17 | target | library input | impl/sub/other2.rb:3:5:3:34 | call to popen | shell command | +| impl/sub/other.rb:3:14:3:28 | "cat #{...}" | impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other.rb:2:12:2:17 | target | library input | impl/sub/other.rb:3:5:3:34 | call to popen | shell command | +| impl/unsafeShell.rb:3:14:3:28 | "cat #{...}" | impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:2:12:2:17 | target | library input | impl/unsafeShell.rb:3:5:3:34 | call to popen | shell command | +| impl/unsafeShell.rb:7:14:7:33 | call to sprintf | impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x | This formatted string which depends on $@ is later used in a $@. | impl/unsafeShell.rb:6:12:6:12 | x | library input | impl/unsafeShell.rb:8:5:8:25 | call to popen | shell command | +| impl/unsafeShell.rb:20:14:20:42 | "which #{...}" | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path | library input | impl/unsafeShell.rb:20:5:20:48 | call to popen | shell command | +| impl/unsafeShell.rb:26:14:26:31 | "cat #{...}" | impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:23:15:23:23 | file_path | library input | impl/unsafeShell.rb:26:5:26:37 | call to popen | shell command | +| impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command | +| impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command | diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref new file mode 100644 index 00000000000..99292da7663 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref @@ -0,0 +1 @@ +queries/security/cwe-078/UnsafeShellCommandConstruction.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb new file mode 100644 index 00000000000..0a385f5f6bc --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb @@ -0,0 +1,6 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK - everything assumed to be imported... + end +end + \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb new file mode 100644 index 00000000000..22eaa13bcc0 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb @@ -0,0 +1,7 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end + +require 'sub/other2' \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb new file mode 100644 index 00000000000..007dae343ff --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb @@ -0,0 +1,5 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb new file mode 100644 index 00000000000..52e3016ac91 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb @@ -0,0 +1,45 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end + + def foo2(x) + format = sprintf("cat %s", x) # NOT OK + IO.popen(format, "w") + end + + def fileRead1(path) + File.read(path) # OK + end + + def my_exec(cmd, command, myCmd, myCommand, innocent_file_path) + IO.popen("which #{cmd}", "w") # OK - the parameter is named `cmd`, so it's meant to be a command + IO.popen("which #{command}", "w") # OK - the parameter is named `command`, so it's meant to be a command + IO.popen("which #{myCmd}", "w") # OK - the parameter is named `myCmd`, so it's meant to be a command + IO.popen("which #{myCommand}", "w") # OK - the parameter is named `myCommand`, so it's meant to be a command + IO.popen("which #{innocent_file_path}", "w") # NOT OK - the parameter is named `innocent_file_path`, so it's not meant to be a command + end + + def escaped(file_path) + IO.popen("cat #{file_path.shellescape}", "w") # OK - the parameter is escaped + + IO.popen("cat #{file_path}", "w") # NOT OK - the parameter is not escaped + end +end + +require File.join(File.dirname(__FILE__), 'sub', 'other') + +class Foobar2 + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end + + def id(x) + IO.popen("cat #{x}", "w") # NOT OK - the parameter is not a constant. + return x + end + + def thisIsSafe() + IO.popen("echo #{id('foo')}", "w") # OK - only using constants. + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec new file mode 100644 index 00000000000..545bc14a7a6 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec @@ -0,0 +1,5 @@ +Gem::Specification.new do |s| + s.name = 'unsafe-shell' + s.require_path = "impl" + end + \ No newline at end of file From d427e555077158829997df0a83ad7b1f0dabbe8f Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 15:07:14 +0200 Subject: [PATCH 09/49] add qhelp --- .../UnsafeShellCommandConstruction.qhelp | 73 +++++++++++++++++++ .../unsafe-shell-command-construction.rb | 5 ++ ...unsafe-shell-command-construction_fixed.rb | 6 ++ 3 files changed, 84 insertions(+) create mode 100644 ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp create mode 100644 ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb create mode 100644 ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp new file mode 100644 index 00000000000..88cea1d80d3 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp @@ -0,0 +1,73 @@ + + + +

+ Dynamically constructing a shell command with inputs from exported + functions may inadvertently change the meaning of the shell command. + + Clients using the exported function may use inputs containing + characters that the shell interprets in a special way, for instance + quotes and spaces. + + This can result in the shell command misbehaving, or even + allowing a malicious user to execute arbitrary commands on the system. +

+ + + + + +

+ If possible, provide the dynamic arguments to the shell as an array + to APIs such as system(..) to avoid interpretation by the shell. +

+ +

+ Alternatively, if the shell command must be constructed + dynamically, then add code to ensure that special characters + do not alter the shell command unexpectedly. +

+ + + + +

+ The following example shows a dynamically constructed shell + command that downloads a file from a remote URL. +

+ + + +

+ The shell command will, however, fail to work as intended if the + input contains spaces or other special characters interpreted in a + special way by the shell. +

+ +

+ Even worse, a client might pass in user-controlled + data, not knowing that the input is interpreted as a shell command. + This could allow a malicious user to provide the input http://example.org; cat /etc/passwd + in order to execute the command cat /etc/passwd. +

+ +

+ To avoid such potentially catastrophic behaviors, provide the + inputs from exported functions as an argument that does not + get interpreted by a shell: +

+ + + + + + +
  • + OWASP: + Command Injection. +
    • + +     +     diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb new file mode 100644 index 00000000000..500bd49e890 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb @@ -0,0 +1,5 @@ +module Utils + def download(path) + system("wget #{path}") # NOT OK + end +end \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb new file mode 100644 index 00000000000..cb8730eee09 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb @@ -0,0 +1,6 @@ +module Utils + def download(path) + # using an array to call `system` is safe + system("wget", path) # OK + end +end \ No newline at end of file From cadb948d576e938368313d53e1ca35dea6f9bc0a Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 15:07:20 +0200 Subject: [PATCH 10/49] add change-note --- .../2022-10-10-unsafe-shell-command-construction.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md diff --git a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md new file mode 100644 index 00000000000..d61d32dcc5f --- /dev/null +++ b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely constructs shell commands from their inputs. From b64a1b7c42f2e7ed3214f5911bd8563c6cebaebc Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Mon, 10 Oct 2022 15:14:49 +0200 Subject: [PATCH 11/49] add a missing qldoc --- .../security/UnsafeShellCommandConstructionCustomizations.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll index a235c53462f..94b16935a55 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -11,6 +11,9 @@ private import codeql.ruby.frameworks.core.Gem::Gem as Gem private import codeql.ruby.AST as Ast private import codeql.ruby.Concepts as Concepts +/** + * Module containing sources, sinks, and sanitizers for shell command constructed from library input. + */ module UnsafeShellCommandConstruction { /** A source for shell command constructed from library input vulnerabilities. */ abstract class Source extends DataFlow::Node { } From 0220f0aa5cfb566a579dd10181875429ea78ab5e Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 11 Oct 2022 13:37:01 +0200 Subject: [PATCH 12/49] use type-tracking instead --- ...ShellCommandConstructionCustomizations.qll | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll index 94b16935a55..b89743fe68e 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -43,28 +43,22 @@ module UnsafeShellCommandConstruction { abstract DataFlow::Node getCommandExecution(); } - /** A dataflow-configuration for tracking flow from various string constructions to places where those strings are executed as shell commands. */ - class TrackSystemCommand extends DataFlow2::Configuration { - TrackSystemCommand() { this = "StringConcatAsSink::TrackSystemCommand" } - - override predicate isSource(DataFlow::Node source) { - source instanceof TaintedFormat::PrintfStyleCall - or - source.asExpr().getExpr() = - any(Ast::StringLiteral lit | - lit.getComponent(_) instanceof Ast::StringInterpolationComponent - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(Concepts::SystemCommandExecution s | s.isShellInterpreted(sink)) - } - } - /** Holds if the string constructed at `source` is executed at `shellExec` */ predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) { - any(TrackSystemCommand conf) - .hasFlow(source, any(DataFlow::Node arg | shellExec.isShellInterpreted(arg))) + source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec) + } + + import codeql.ruby.typetracking.TypeTracker as TypeTracker + + private DataFlow::LocalSourceNode backtrackShellExec( + TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec + ) { + t.start() and + result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource() + or + exists(TypeTracker::TypeBackTracker t2 | + result = backtrackShellExec(t2, shellExec).backtrack(t2, t) + ) } /** From 429bd5fbd86130f189e9957d3dad4c297d3c3732 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Oct 2022 15:30:39 +0200 Subject: [PATCH 13/49] Add flow summaries for startActivities Uses SyntheticCallables and SyntheticGlobals to pair each startActivities call to getIntent calls in the components targeted by the intent(s). --- .../semmle/code/java/dataflow/FlowSummary.qll | 8 ++ .../code/java/frameworks/android/Intent.qll | 123 +++++++++++++++++- .../intent/TestStartActivityToGetIntent.java | 21 ++- 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll b/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll index ed9b0de165d..6b790e487c1 100644 --- a/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll +++ b/java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll @@ -26,6 +26,9 @@ module SummaryComponent { /** Gets a summary component for `Element`. */ SummaryComponent element() { result = content(any(CollectionContent c)) } + /** Gets a summary component for `ArrayElement`. */ + SummaryComponent arrayElement() { result = content(any(ArrayContent c)) } + /** Gets a summary component for `MapValue`. */ SummaryComponent mapValue() { result = content(any(MapValueContent c)) } @@ -52,6 +55,11 @@ module SummaryComponentStack { result = push(SummaryComponent::element(), object) } + /** Gets a stack representing `ArrayElement` of `object`. */ + SummaryComponentStack arrayElementOf(SummaryComponentStack object) { + result = push(SummaryComponent::arrayElement(), object) + } + /** Gets a stack representing `MapValue` of `object`. */ SummaryComponentStack mapValueOf(SummaryComponentStack object) { result = push(SummaryComponent::mapValue(), object) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index be38b83e5a7..d4c67d10761 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -1,7 +1,10 @@ import java +private import semmle.code.java.frameworks.android.Android private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.java.dataflow.FlowSummary +private import semmle.code.java.dataflow.internal.BaseSSA as BaseSSA /** The class `android.content.Intent`. */ class TypeIntent extends Class { @@ -242,19 +245,52 @@ private class StartComponentMethodAccess extends MethodAccess { /** Gets the intent argument of this call. */ Argument getIntentArg() { - result.getType() instanceof TypeIntent and + ( + result.getType() instanceof TypeIntent or + result.getType().(Array).getElementType() instanceof TypeIntent + ) and result = this.getAnArgument() } /** Holds if this targets a component of type `targetType`. */ - predicate targetsComponentType(RefType targetType) { + predicate targetsComponentType(AndroidComponent targetType) { exists(NewIntent newIntent | - DataFlow::localExprFlow(newIntent, this.getIntentArg()) and + reaches(newIntent, this.getIntentArg()) and newIntent.getClassArg().getType().(ParameterizedType).getATypeArgument() = targetType ) } } +/** + * Holds if `src` reaches the intent argument `arg` of `StartComponentMethodAccess` + * through intra-procedural steps. + */ +private predicate reaches(Expr src, Argument arg) { + any(StartComponentMethodAccess ma).getIntentArg() = arg and + src = arg + or + exists(Expr mid, BaseSSA::BaseSsaVariable ssa, BaseSSA::BaseSsaUpdate upd | + reaches(mid, arg) and + mid = ssa.getAUse() and + upd = ssa.getAnUltimateLocalDefinition() and + src = upd.getDefiningExpr().(VariableAssign).getSource() + ) + or + exists(CastingExpr e | e.getExpr() = src | reaches(e, arg)) + or + exists(ChooseExpr e | e.getAResultExpr() = src | reaches(e, arg)) + or + exists(AssignExpr e | e.getSource() = src | reaches(e, arg)) + or + exists(ArrayCreationExpr e | e.getInit().getAnInit() = src | reaches(e, arg)) + or + exists(StmtExpr e | e.getResultExpr() = src | reaches(e, arg)) + or + exists(NotNullExpr e | e.getExpr() = src | reaches(e, arg)) + or + exists(WhenExpr e | e.getBranch(_).getAResult() = src | reaches(e, arg)) +} + /** * A value-preserving step from the intent argument of a `startActivity` call to * a `getIntent` call in the activity the intent targeted in its constructor. @@ -271,6 +307,87 @@ private class StartActivityIntentStep extends AdditionalValueStep { } } +/** + * Holds if `targetType` is targeted by an existing `StartComponentMethodAccess` call + * and it's identified by `id`. + */ +private predicate isTargetableType(AndroidComponent targetType, string id) { + exists(StartComponentMethodAccess ma | ma.targetsComponentType(targetType)) and + targetType.getQualifiedName() = id +} + +private class StartActivitiesSyntheticCallable extends SyntheticCallable { + AndroidComponent targetType; + + StartActivitiesSyntheticCallable() { + exists(string id | + this = "android.content.Activity.startActivities()+" + id and + isTargetableType(targetType, id) + ) + } + + override StartComponentMethodAccess getACall() { + result.getMethod().hasName("startActivities") and + result.targetsComponentType(targetType) + } + + override predicate propagatesFlow( + SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue + ) { + exists(ActivityIntentSyntheticGlobal glob | glob.getTargetType() = targetType | + input = SummaryComponentStack::arrayElementOf(SummaryComponentStack::argument(0)) and + output = SummaryComponentStack::singleton(SummaryComponent::syntheticGlobal(glob)) and + preservesValue = true + ) + } +} + +private class GetIntentSyntheticCallable extends SyntheticCallable { + AndroidComponent targetType; + + GetIntentSyntheticCallable() { + exists(string id | + this = "android.content.Activity.getIntent()+" + id and + isTargetableType(targetType, id) + ) + } + + override Call getACall() { + result.getCallee() instanceof AndroidGetIntentMethod and + result.getEnclosingCallable().getDeclaringType() = targetType + } + + override predicate propagatesFlow( + SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue + ) { + exists(ActivityIntentSyntheticGlobal glob | glob.getTargetType() = targetType | + input = SummaryComponentStack::singleton(SummaryComponent::syntheticGlobal(glob)) and + output = SummaryComponentStack::return() and + preservesValue = true + ) + } +} + +private class ActivityIntentSyntheticGlobal extends SummaryComponent::SyntheticGlobal { + AndroidComponent targetType; + + ActivityIntentSyntheticGlobal() { + exists(string id | + this = "ActivityIntentSyntheticGlobal+" + id and + isTargetableType(targetType, id) + ) + } + + AndroidComponent getTargetType() { result = targetType } +} + +private class RequiredComponentStackForStartActivities extends RequiredSummaryComponentStack { + override predicate required(SummaryComponent head, SummaryComponentStack tail) { + head = SummaryComponent::element() and + tail = SummaryComponentStack::argument(0) + } +} + /** * A value-preserving step from the intent argument of a `sendBroadcast` call to * the intent parameter in the `onReceive` method of the receiver the diff --git a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java index 35884a23a58..09e785e5b6c 100644 --- a/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java +++ b/java/ql/test/library-tests/frameworks/android/intent/TestStartActivityToGetIntent.java @@ -24,6 +24,12 @@ public class TestStartActivityToGetIntent { Intent[] intents = new Intent[] {intent}; ctx.startActivities(intents); } + { + Intent intent = new Intent(null, AnotherActivity.class); + intent.putExtra("data", (String) source("ctx-start-acts-2")); + Intent[] intents = new Intent[] {intent}; + ctx.startActivities(intents); + } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("act-start")); @@ -35,6 +41,12 @@ public class TestStartActivityToGetIntent { Intent[] intents = new Intent[] {intent}; act.startActivities(intents); } + { + Intent intent = new Intent(null, Object.class); + intent.putExtra("data", (String) source("start-activities-should-not-reach")); + Intent[] intents = new Intent[] {intent}; + act.startActivities(intents); + } { Intent intent = new Intent(null, SomeActivity.class); intent.putExtra("data", (String) source("start-for-result")); @@ -79,9 +91,16 @@ public class TestStartActivityToGetIntent { static class SomeActivity extends Activity { public void test() { - sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg MISSING: hasValueFlow=ctx-start-acts hasValueFlow=act-start-acts + // @formatter:off + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start hasValueFlow=act-start hasValueFlow=start-for-result hasValueFlow=start-if-needed hasValueFlow=start-matching hasValueFlow=start-from-child hasValueFlow=start-from-frag hasValueFlow=4-arg hasValueFlow=ctx-start-acts hasValueFlow=act-start-acts + // @formatter:on } + } + static class AnotherActivity extends Activity { + public void test() { + sink(getIntent().getStringExtra("data")); // $ hasValueFlow=ctx-start-acts-2 + } } static class SafeActivity extends Activity { From 25241276b0bf75ba598f013f87788f54f80aa368 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Oct 2022 16:29:36 +0200 Subject: [PATCH 14/49] Add change note --- .../2022-10-19-android-startactivities-summaries.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md diff --git a/java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md b/java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md new file mode 100644 index 00000000000..4716fb2ac41 --- /dev/null +++ b/java/ql/lib/change-notes/2022-10-19-android-startactivities-summaries.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added data flow summaries for tainted Android intents sent to activities via `Activity.startActivities`. \ No newline at end of file From 0678b06a9bbd4f3ce8650030b941b5d0f7ef03f3 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Oct 2022 16:58:39 +0200 Subject: [PATCH 15/49] Apply review suggestions --- java/ql/lib/semmle/code/java/frameworks/android/Intent.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll index d4c67d10761..e37e7f350b8 100644 --- a/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/lib/semmle/code/java/frameworks/android/Intent.qll @@ -4,7 +4,7 @@ private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSteps private import semmle.code.java.dataflow.FlowSummary -private import semmle.code.java.dataflow.internal.BaseSSA as BaseSSA +private import semmle.code.java.dataflow.internal.BaseSSA as BaseSsa /** The class `android.content.Intent`. */ class TypeIntent extends Class { @@ -269,7 +269,7 @@ private predicate reaches(Expr src, Argument arg) { any(StartComponentMethodAccess ma).getIntentArg() = arg and src = arg or - exists(Expr mid, BaseSSA::BaseSsaVariable ssa, BaseSSA::BaseSsaUpdate upd | + exists(Expr mid, BaseSsa::BaseSsaVariable ssa, BaseSsa::BaseSsaUpdate upd | reaches(mid, arg) and mid = ssa.getAUse() and upd = ssa.getAnUltimateLocalDefinition() and @@ -383,7 +383,7 @@ private class ActivityIntentSyntheticGlobal extends SummaryComponent::SyntheticG private class RequiredComponentStackForStartActivities extends RequiredSummaryComponentStack { override predicate required(SummaryComponent head, SummaryComponentStack tail) { - head = SummaryComponent::element() and + head = SummaryComponent::arrayElement() and tail = SummaryComponentStack::argument(0) } } From 420c35d4a2bbf5ee628a351e71292c1ae1486ec1 Mon Sep 17 00:00:00 2001 From: Karim Ali Date: Wed, 26 Oct 2022 15:32:59 +0200 Subject: [PATCH 16/49] add a query that detects the use of constant salts --- .../Security/CWE-760/ConstantSalt.qhelp | 21 ++++++ .../queries/Security/CWE-760/ConstantSalt.ql | 63 +++++++++++++++++ .../Security/CWE-760/ConstantSalt.swift | 22 ++++++ .../Security/CWE-760/ConstantSalt.expected | 17 +++++ .../Security/CWE-760/ConstantSalt.qlref | 1 + .../query-tests/Security/CWE-760/test.swift | 70 +++++++++++++++++++ 6 files changed, 194 insertions(+) create mode 100644 swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp create mode 100644 swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql create mode 100644 swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift create mode 100644 swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected create mode 100644 swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref create mode 100644 swift/ql/test/query-tests/Security/CWE-760/test.swift diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp new file mode 100644 index 00000000000..ca8d65d9982 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp @@ -0,0 +1,21 @@ + + + +

    Constant salts should not be used for password hashing. Data hashed using constant salts are vulnerable to dictionary attacks, enabling attackers to recover the original input.

    +     + + +

    Use randomly generated salts to securely hash input data.

    +     + + +

    The following example shows a few cases of hashing input data. In the 'BAD' cases, the salt is constant, making the generated hashes vulnerable to dictionary attakcs. In the 'GOOD' cases, the salt is randomly generated, which protects the hashed data against recovery.

    + +     + + +
  • What are Salted Passwords and Password Hashing?
    • +     +     diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql new file mode 100644 index 00000000000..32bc1325147 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql @@ -0,0 +1,63 @@ +/** + * @name Constant salt + * @description Using constant salts for password hashing is not secure, because potential attackers can pre-compute the hash value via dictionary attacks. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id swift/constant-salt + * @tags security + * external/cwe/cwe-760 + */ + +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.dataflow.TaintTracking +import codeql.swift.dataflow.FlowSteps +import DataFlow::PathGraph + +/** + * A constant salt is created through either a byte array or string literals. + */ +class ConstantSaltSource extends Expr { + ConstantSaltSource() { + this = any(ArrayExpr arr | arr.getType().getName() = "Array") or + this instanceof StringLiteralExpr + } +} + +/** + * A class for all ways to use a constant salt. + */ +class ConstantSaltSink extends Expr { + ConstantSaltSink() { + // `salt` arg in `init` is a sink + exists(ClassOrStructDecl c, AbstractFunctionDecl f, CallExpr call, int arg | + c.getFullName() = ["HKDF", "PBKDF1", "PBKDF2", "Scrypt"] and + c.getAMember() = f and + f.getName().matches("%init(%salt:%") and + call.getStaticTarget() = f and + f.getParam(pragma[only_bind_into](arg)).getName() = "salt" and + call.getArgument(pragma[only_bind_into](arg)).getExpr() = this + ) + } +} + +/** + * A taint configuration from the source of constants salts to expressions that use + * them to initialize password-based enecryption keys. + */ +class ConstantSaltConfig extends TaintTracking::Configuration { + ConstantSaltConfig() { this = "ConstantSaltConfig" } + + override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof ConstantSaltSource } + + override predicate isSink(DataFlow::Node node) { node.asExpr() instanceof ConstantSaltSink } +} + +// The query itself +from ConstantSaltConfig config, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode +where config.hasFlowPath(sourceNode, sinkNode) +select sinkNode.getNode(), sourceNode, sinkNode, + "The value '" + sourceNode.getNode().toString() + + "' is used as a constant salt, which is insecure for hashing passwords." diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift new file mode 100644 index 00000000000..a3bc11c826b --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift @@ -0,0 +1,22 @@ + +func encrypt(padding : Padding) { + // ... + + // BAD: Using constant salts for hashing + let salt: Array = [0x2a, 0x3a, 0x80, 0x05] + let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) + _ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2) + _ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1) + + // GOOD: Using randomly generated salts for hashing + let salt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) + let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) + _ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2) + _ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0) + _ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1) + + // ... +} diff --git a/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected new file mode 100644 index 00000000000..6c02fd2c9b4 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected @@ -0,0 +1,17 @@ +edges +| test.swift:43:35:43:130 | [...] : | test.swift:51:49:51:49 | constantSalt | +| test.swift:43:35:43:130 | [...] : | test.swift:56:59:56:59 | constantSalt | +| test.swift:43:35:43:130 | [...] : | test.swift:62:59:62:59 | constantSalt | +| test.swift:43:35:43:130 | [...] : | test.swift:67:53:67:53 | constantSalt | +nodes +| test.swift:43:35:43:130 | [...] : | semmle.label | [...] : | +| test.swift:51:49:51:49 | constantSalt | semmle.label | constantSalt | +| test.swift:56:59:56:59 | constantSalt | semmle.label | constantSalt | +| test.swift:62:59:62:59 | constantSalt | semmle.label | constantSalt | +| test.swift:67:53:67:53 | constantSalt | semmle.label | constantSalt | +subpaths +#select +| test.swift:51:49:51:49 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:51:49:51:49 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | +| test.swift:56:59:56:59 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:56:59:56:59 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | +| test.swift:62:59:62:59 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:62:59:62:59 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | +| test.swift:67:53:67:53 | constantSalt | test.swift:43:35:43:130 | [...] : | test.swift:67:53:67:53 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. | \ No newline at end of file diff --git a/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref new file mode 100644 index 00000000000..04aadc2161f --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref @@ -0,0 +1 @@ +queries/Security/CWE-760/ConstantSalt.ql diff --git a/swift/ql/test/query-tests/Security/CWE-760/test.swift b/swift/ql/test/query-tests/Security/CWE-760/test.swift new file mode 100644 index 00000000000..05f5396d244 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-760/test.swift @@ -0,0 +1,70 @@ + +// --- stubs --- + +// These stubs roughly follows the same structure as classes from CryptoSwift +enum PKCS5 { } + +enum Variant { case md5, sha1, sha2, sha3 } + +extension PKCS5 { + struct PBKDF1 { + init(password: Array, salt: Array, variant: Variant = .sha1, iterations: Int = 4096, keyLength: Int? = nil) { } + } + + struct PBKDF2 { + init(password: Array, salt: Array, iterations: Int = 4096, keyLength: Int? = nil, variant: Variant = .sha2) { } + } +} + +struct HKDF { + init(password: Array, salt: Array? = nil, info: Array? = nil, keyLength: Int? = nil, variant: Variant = .sha2) { } +} + +final class Scrypt { + init(password: Array, salt: Array, dkLen: Int, N: Int, r: Int, p: Int) { } +} + +// Helper functions +func getConstantString() -> String { + "this string is constant" +} + +func getConstantArray() -> Array { + [UInt8](getConstantString().utf8) +} + +func getRandomArray() -> Array { + (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) }) +} + +// --- tests --- + +func test() { + let constantSalt: Array = [0x2a, 0x3a, 0x80, 0x05, 0xaf, 0x46, 0x58, 0x2d, 0x66, 0x52, 0x10, 0xae, 0x86, 0xd3, 0x8e, 0x8f] + let constantStringSalt = getConstantArray() + let randomSalt = getRandomArray() + let randomArray = getRandomArray() + let variant = Variant.sha2 + let iterations = 120120 + + // HKDF test cases + let hkdfb1 = HKDF(password: randomArray, salt: constantSalt, info: randomArray, keyLength: 0, variant: variant) // BAD + let hkdfb2 = HKDF(password: randomArray, salt: constantStringSalt, info: randomArray, keyLength: 0, variant: variant) // BAD [NOT DETECTED] + let hkdfg1 = HKDF(password: randomArray, salt: randomSalt, info: randomArray, keyLength: 0, variant: variant) // GOOD + + // PBKDF1 test cases + let pbkdf1b1 = PKCS5.PBKDF1(password: randomArray, salt: constantSalt, iterations: iterations, keyLength: 0) // BAD + let pbkdf1b2 = PKCS5.PBKDF1(password: randomArray, salt: constantStringSalt, iterations: iterations, keyLength: 0) // BAD [NOT DETECTED] + let pbkdf1g1 = PKCS5.PBKDF1(password: randomArray, salt: randomSalt, iterations: iterations, keyLength: 0) // GOOD + + + // PBKDF2 test cases + let pbkdf2b1 = PKCS5.PBKDF2(password: randomArray, salt: constantSalt, iterations: iterations, keyLength: 0) // BAD + let pbkdf2b2 = PKCS5.PBKDF2(password: randomArray, salt: constantStringSalt, iterations: iterations, keyLength: 0) // BAD [NOT DETECTED] + let pbkdf2g1 = PKCS5.PBKDF2(password: randomArray, salt: randomSalt, iterations: iterations, keyLength: 0) // GOOD + + // Scrypt test cases + let scryptb1 = Scrypt(password: randomArray, salt: constantSalt, dkLen: 64, N: 16384, r: 8, p: 1) // BAD + let scryptb2 = Scrypt(password: randomArray, salt: constantStringSalt, dkLen: 64, N: 16384, r: 8, p: 1) // BAD [NOT DETECTED] + let scryptg1 = Scrypt(password: randomArray, salt: randomSalt, dkLen: 64, N: 16384, r: 8, p: 1) // GOOD +} \ No newline at end of file From fd61a5253db6a2a9b05678b03eae3680fc522b16 Mon Sep 17 00:00:00 2001 From: Harry Maclean Date: Fri, 28 Oct 2022 12:14:29 +1300 Subject: [PATCH 17/49] Ruby: Recognise try/try! as code executions --- .../codeql/ruby/frameworks/ActiveSupport.qll | 23 +++++++++++++++++++ .../active_support/ActiveSupport.expected | 9 ++++++++ .../active_support/ActiveSupport.ql | 3 +++ .../active_support/active_support.rb | 8 +++++++ 4 files changed, 43 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll index 6c57b31f6fa..bf8d46e3e7e 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll @@ -81,6 +81,29 @@ module ActiveSupport { preservesValue = true } } + + /** + * A call to `Object#try`, which may execute its first argument as a Ruby + * method call. + * ```rb + * x = "abc" + * x.try(:upcase) # => "ABC" + * y = nil + * y.try(:upcase) # => nil + * ``` + * `Object#try!` behaves similarly but raises `NoMethodError` if the + * receiver is not `nil` and does not respond to the method. + */ + class TryCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode { + TryCallCodeExecution() { + this.asExpr().getExpr() instanceof UnknownMethodCall and + this.getMethodName() = ["try", "try!"] + } + + override DataFlow::Node getCode() { result = this.getArgument(0) } + + override predicate runsArbitraryCode() { none() } + } } /** diff --git a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected index c99abbeacf3..bca8bf08959 100644 --- a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected +++ b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.expected @@ -5,3 +5,12 @@ constantizeCalls loggerInstantiations | active_support.rb:6:1:6:33 | call to new | | active_support.rb:7:1:7:40 | call to new | +codeExecutions +| active_support.rb:1:1:1:22 | call to constantize | +| active_support.rb:3:1:3:13 | call to constantize | +| active_support.rb:4:1:4:18 | call to safe_constantize | +| active_support.rb:296:5:296:18 | call to try | +| active_support.rb:297:5:297:17 | call to try | +| active_support.rb:298:5:298:19 | call to try! | +| active_support.rb:298:5:298:35 | call to try! | +| active_support.rb:299:5:299:18 | call to try! | diff --git a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql index 149113851be..239a47434e2 100644 --- a/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql +++ b/ruby/ql/test/library-tests/frameworks/active_support/ActiveSupport.ql @@ -1,9 +1,12 @@ import codeql.ruby.frameworks.ActiveSupport import codeql.ruby.DataFlow import codeql.ruby.frameworks.stdlib.Logger +import codeql.ruby.Concepts query DataFlow::Node constantizeCalls(ActiveSupport::CoreExtensions::String::Constantize c) { result = c.getCode() } query predicate loggerInstantiations(Logger::LoggerInstantiation l) { any() } + +query predicate codeExecutions(CodeExecution c) { any() } diff --git a/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb b/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb index 023f9724ce8..fe0256080d1 100644 --- a/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb +++ b/ruby/ql/test/library-tests/frameworks/active_support/active_support.rb @@ -290,3 +290,11 @@ def m_deep_dup x = source "a" sink x.deep_dup # $hasValueFlow=a end + +def m_try(method) + x = "abc" + x.try(:upcase) + x.try(method) + x.try!(:upcase).try!(:downcase) + x.try!(method) +end From 0dd63c007eb64ccc87c63bc1f40be6305a03bdc8 Mon Sep 17 00:00:00 2001 From: Harry Maclean Date: Fri, 28 Oct 2022 12:16:02 +1300 Subject: [PATCH 18/49] Ruby: Add change note --- ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md diff --git a/ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md b/ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md new file mode 100644 index 00000000000..af5b1cb59e4 --- /dev/null +++ b/ruby/ql/lib/change-notes/2022-10-28-try-code-execution.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `ActiveSupport` extensions `Object#try` and `Object#try!` are now recognised as code executions. From 04a47093eed2e0d949b58b64088c1555a0428a40 Mon Sep 17 00:00:00 2001 From: alexet Date: Tue, 1 Nov 2022 18:31:43 +0000 Subject: [PATCH 19/49] QL Spec: Add instanceof in classes --- .../ql-language-specification.rst | 59 +++++++++++++------ 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/docs/codeql/ql-language-reference/ql-language-specification.rst b/docs/codeql/ql-language-reference/ql-language-specification.rst index 46912d2b083..ac60098319f 100644 --- a/docs/codeql/ql-language-reference/ql-language-specification.rst +++ b/docs/codeql/ql-language-reference/ql-language-specification.rst @@ -819,13 +819,15 @@ A class definition has the following syntax: :: - class ::= qldoc? annotations "class" classname "extends" type ("," type)* "{" member* "}" + class ::= qldoc? annotations "class" classname ("extends" type ("," type)*)? ("instanceof" type ("," type)*)? "{" member* "}" The identifier following the ``class`` keyword is the name of the class. The types specified after the ``extends`` keyword are the *base types* of the class. -A class domain type is said to *inherit* from the base types of the associated class type, a character type is said to *inherit* from its associated class domain type and a class type is said to *inherit* from its associated character type. In addition, inheritance is transitive: If a type ``A`` inherits from a type ``B``, and ``B`` inherits from a type ``C``, then ``A`` inherits from ``C``. +The types specified after the ``instanceof`` keyword are the *instanceof types* of the class. + +A class type is said to *inherit* from the base types of the associated class type. In addition, inheritance is transitive: If a type ``A`` inherits from a type ``B``, and ``B`` inherits from a type ``C``, then ``A`` inherits from ``C``. A class adds a mapping from the class name to the class declaration to the current module's declared type environment. @@ -833,6 +835,20 @@ A valid class can be annotated with ``abstract``, ``final``, ``library``, and `` A valid class may not inherit from a final class, from itself, or from more than one primitive type. +A valid class must have at least one base type or instanceof type. + +Class dependencies +~~~~~~~~~~~~~~~~~~ + +The program is invalid if there is a cycle of class dependencies. + +The following are class dependencies: +- ``C`` depends on ``C.C`` +- ``C.C`` depends on ``C.extends`` +- If ``C`` is abstract then it depends on all classes ``D`` such that ``C`` is a base type of ``D``. +- ``C.extends`` depends on ``D.D`` for each base type ``D`` of ``C``. +- ``C.extends`` depends on ``D`` for each instanceof type ``D`` of ``C``. + Class environments ~~~~~~~~~~~~~~~~~~ @@ -848,7 +864,9 @@ For each of member predicates and fields a class *inherits* and *declares*, and The program is invalid if any of these environments is not definite. -For each of member predicates and fields a domain type *exports* an environment. This is the union of the exported ``X`` environments of types the class inherits from, excluding any elements that are ``overridden`` by another element. +For each of member predicates and fields a domain type *exports* an environment. We say the *exported X extends environment* is the union of the exported ``X`` environments of types the class inherits from, excluding any elements that are ``overridden`` by another element. +We say the *exported X instanceof environement* is the union of the exported ``X`` environments of types that a instanceof type inherits from, excluding any elements that are ``overridden`` by another element. +The *exported X environment* of the domain type is the union of the exported ``X`` extends environment and the exported ``X`` instanceof environement. Members ~~~~~~~ @@ -1090,11 +1108,7 @@ A super expression has the following syntax: super_expr ::= "super" | type "." "super" -For a super expression to be valid, the ``this`` keyword must have a type and value in the typing environment. The type of the expression is the same as the type of ``this`` in the typing environment. - -A super expression may only occur in a QL program as the receiver expression for a predicate call. - -If a super expression includes a ``type``, then that type must be a class that the enclosing class inherits from. +For a super expression to be valid, the ``this`` keyword must have a type and value in the typing environment. The type of the expression is the same as the domain type of the type of ``this`` in the typing environment. The value of a super expression is the same as the value of ``this`` in the named tuple. @@ -1147,13 +1161,6 @@ A valid call with results *resolves* to a set of predicates. The ways a call can - If the call has no receiver and the predicate name is a selection identifier, then the qualifier is resolved as a module (see "`Module resolution <#module-resolution>`__"). The identifier is then resolved in the exported predicate environment of the qualifier module. -- If the call has a super expression as the receiver, then it resolves to a member predicate in a class that the enclosing class inherits from: - - If the super expression is unqualified and there is a single class that the current class inherits from, then the super-class is that class. - - If the super expression is unqualified and there are multiple classes that the current class inherits from, then the super-class is the domain type. - - Otherwise, the super-class is the class named by the qualifier of the super expression. - - The predicate is resolved by looking up its name and arity in the exported predicate environment of the super-class. - - If the type of the receiver is the same as the enclosing class, the predicate is resolved by looking up its name and arity in the visible predicate environment of the class. - If the type of the receiver is not the same as the enclosing class, the predicate is resolved by looking up its name and arity in the exported predicate environment of the class or domain type. @@ -1177,11 +1184,20 @@ If the resolved predicate is built in, then the call may not include a closure. If the call includes a closure, then all declared predicate arguments, the enclosing type of the declaration (if it exists), and the result type of the declaration (if it exists) must be compatible. If one of those types is a subtype of ``int``, then all the other arguments must be a subtype of ``int``. +A call to a member predicate may be a *direct* call: + - If the receiver is not a super expression it is not direct. + - The receiver is ``A.super`` and ``A`` is an instanceof type and not a base type then it is not direct. + - The receiver is ``A.super`` and ``A`` is a base type type and not an instanceof type then it is direct. + - If the receiver is ``A.super`` and ``A`` is a base type and an instanceof type then the call is not valid + - The receiver is ``super`` and the member predicate is in the exported member predicate environment of an instanceof type and not in the exported member predicate environment of a base type then it isn't direct. + - The receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and not in the exported member predicate environment of an instanceof type then it is direct. + - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and in the exported member predicate environment of an instanceof type then the call is not valid. + If the call resolves to a member predicate, then the *receiver values* are as follows. If the call has a receiver, then the receiver values are the values of that receiver. If the call does not have a receiver, then the single receiver value is the value of ``this`` in the contextual named tuple. The *tuple prefixes* of a call with results include one value from each of the argument expressions' values, in the same order as the order of the arguments. If the call resolves to a non-member predicate, then those values are exactly the tuple prefixes of the call. If the call instead resolves to a member predicate, then the tuple prefixes additionally include a receiver value, ordered before the argument values. -The *matching tuples* of a call with results are all ordered tuples that are one of the tuple prefixes followed by any value of the same type as the call. If the call has no closure, then all matching tuples must additionally satisfy the resolved predicate of the call, unless the receiver is a super expression, in which case they must *directly* satisfy the resolved predicate of the call. If the call has a ``*`` or ``+`` closure, then the matching tuples must satisfy or directly satisfy the associated closure of the resolved predicate. +The *matching tuples* of a call with results are all ordered tuples that are one of the tuple prefixes followed by any value of the same type as the call. If the call has no closure, then all matching tuples must additionally satisfy the resolved predicate of the call, unless the call is direct in which case they must *directly* satisfy the resolved predicate of the call. If the call has a ``*`` or ``+`` closure, then the matching tuples must satisfy or directly satisfy the associated closure of the resolved predicate. The values of a call with results are the final elements of each of the call's matching tuples. @@ -1534,13 +1550,15 @@ The identifier is called the *predicate name* of the call. A call must resolve to a predicate, using the same definition of resolve as for calls with results (see "`Calls with results <#calls-with-results>`__"). +A call may be direct using the same definition of direct as for calls with results (see "`Calls with results <#calls-with-results>`__"). + The resolved predicate must not have a result type. If the resolved predicate is a built-in member predicate of a primitive type, then the call may not include a closure. If the call does have a closure, then the call must resolve to a predicate with relational arity of 2. The *candidate tuples* of a call are the ordered tuples formed by selecting a value from each of the arguments of the call. -If the call has no closure, then it matches whenever one of the candidate tuples satisfies the resolved predicate of the call, unless the call has a super expression as a receiver, in which case the candidate tuple must *directly* satisfy the resolved predicate. If the call has ``*`` or ``+`` closure, then the call matches whenever one of the candidate tuples satisfies or directly satisfies the associated closure of the resolved predicate. +If the call has no closure, then it matches whenever one of the candidate tuples satisfies the resolved predicate of the call, unless the call is direct, in which case the candidate tuple must *directly* satisfy the resolved predicate. If the call has ``*`` or ``+`` closure, then the call matches whenever one of the candidate tuples satisfies or directly satisfies the associated closure of the resolved predicate. Disambiguation of formulas ~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -1926,10 +1944,12 @@ Predicates, and types can *depend* and *strictly depend* on each other. Such dep - A predicate containing a predicate call depends on the predicate to which the call resolves. If the call has negative or zero polarity then the dependency is strict. -- A predicate containing a predicate call, which resolves to a member predicate and does not have a ``super`` expression as a qualifier, depends on the dispatch dependencies of the root definitions of the target of the call. If the call has negative or zero polarity then the dependencies are strict. The predicate strictly depends on the strict dispatch dependencies of the root definitions. +- A predicate containing a predicate call, which resolves to a member predicate, where the call is not direct, depends on the dispatch dependencies of the root definitions of the target of the call. If the call has negative or zero polarity then the dependencies are strict. The predicate strictly depends on the strict dispatch dependencies of the root definitions. - For each class ``C`` in the program, for each base class ``B`` of ``C``, ``C.extends`` depends on ``B.B``. +- For each class ``C`` in the program, for each instanceof type ``B`` of ``C``, ``C.extends`` depends on ``B``. + - For each class ``C`` in the program, for each base type ``B`` of ``C`` that is not a class type, ``C.extends`` depends on ``B``. - For each class ``C`` in the program, ``C.class`` depends on ``C.C``. @@ -1976,6 +1996,7 @@ Each layer of the stratification is *populated* in order. To populate a layer, e - To populate the type ``C.extends`` for a class ``C``, identify each named tuple that has the following properties: - The value of ``this`` is in all non-class base types of ``C``. + - The value of ``this`` is in all instanceof types of ``C``. - The keys of the tuple are ``this`` and the union of the public fields from each base type. - For each class base type ``B`` of ``C`` there is a named tuple with variables from the public fields of ``B`` and ``this`` that the given tuple and some tuple in ``B.B`` both extend. @@ -2060,7 +2081,7 @@ The complete grammar for QL is as follows: | "{" formula "}" | "=" literalId "(" (predicateRef "/" int ("," predicateRef "/" int)*)? ")" "(" (exprs)? ")" - class ::= qldoc? annotations "class" classname "extends" type ("," type)* "{" member* "}" + class ::= qldoc? annotations "class" classname ("extends" type ("," type)*)? ("instanceof" type ("," type)*)? "{" member* "}" member ::= character | predicate | field From 556c199a89cf9da531ef814b5a03fd0e87c885d8 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Mon, 31 Oct 2022 10:00:52 +0100 Subject: [PATCH 20/49] Kotlin: Add test case for confusingly overloaded `$default` method --- .../ConfusingMethodSignature.expected | 1 + .../test/kotlin/query-tests/ConfusingMethodSignature/Test.kt | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected index e69de29bb2d..59956f2674d 100644 --- a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected +++ b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected @@ -0,0 +1 @@ +| Test.kt:12:5:12:45 | fn$default | Method A.fn$default(..) could be confused with overloaded method $@, since dispatch depends on static types. | Test.kt:13:5:13:40 | fn$default | fn$default | diff --git a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt index e8ead8d323e..b802d9f76a0 100644 --- a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt +++ b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/Test.kt @@ -7,3 +7,8 @@ class C { prop(this) } } + +class A { + fun fn(value: T, i: Int = 1) {} + fun fn(value: String, i: Int = 1) {} +} From ec5ac17f87b2ff30a497d82bc910d260fe64c072 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Mon, 31 Oct 2022 10:01:46 +0100 Subject: [PATCH 21/49] Kotlin: Excluded compiler generated methods from `java/confusing-method-signature` --- .../Naming Conventions/ConfusingOverloading.ql | 2 ++ .../ConfusingMethodSignature/ConfusingMethodSignature.expected | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql b/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql index e3ac1384243..6034d35bff3 100644 --- a/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql +++ b/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql @@ -137,6 +137,8 @@ private predicate delegate(Method caller, Method callee) { from Method m, Method n, string messageQualifier where + // Exclude compiler generated methods, such as Kotlin `$default` methods: + not m.isCompilerGenerated() and confusinglyOverloaded(m, n) and ( if m.getDeclaringType() = n.getDeclaringType() diff --git a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected index 59956f2674d..e69de29bb2d 100644 --- a/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected +++ b/java/ql/test/kotlin/query-tests/ConfusingMethodSignature/ConfusingMethodSignature.expected @@ -1 +0,0 @@ -| Test.kt:12:5:12:45 | fn$default | Method A.fn$default(..) could be confused with overloaded method $@, since dispatch depends on static types. | Test.kt:13:5:13:40 | fn$default | fn$default | From a7cc8fced5ddc0953e53bc1eeff93250d69a722f Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Wed, 2 Nov 2022 09:46:46 +0100 Subject: [PATCH 22/49] Adjust code based on review --- .../Naming Conventions/ConfusingOverloading.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql b/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql index 6034d35bff3..f355cd5f219 100644 --- a/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql +++ b/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql @@ -57,6 +57,8 @@ private predicate candidateMethod(RefType t, Method m, string name, int numParam m.getNumberOfParameters() = numParam and m = m.getSourceDeclaration() and not m.getAnAnnotation() instanceof DeprecatedAnnotation and + // Exclude compiler generated methods, such as Kotlin `$default` methods: + not m.isCompilerGenerated() and not whitelist(name) } @@ -137,8 +139,6 @@ private predicate delegate(Method caller, Method callee) { from Method m, Method n, string messageQualifier where - // Exclude compiler generated methods, such as Kotlin `$default` methods: - not m.isCompilerGenerated() and confusinglyOverloaded(m, n) and ( if m.getDeclaringType() = n.getDeclaringType() From e48dfcc5b12d5cbe82af5c83dc68abdd75e12161 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Fri, 28 Oct 2022 13:36:47 +0200 Subject: [PATCH 23/49] Kotlin: exclude loop variables on ranges from 'unused locals' check --- .../Dead Code/UnreadLocal.ql | 5 ++++ .../UnreadLocal/UnreadLocal.expected | 3 +++ .../query-tests/UnreadLocal/UnreadLocal.qlref | 1 + .../kotlin/query-tests/UnreadLocal/test.kt | 25 +++++++++++++++++++ 4 files changed, 34 insertions(+) create mode 100644 java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected create mode 100644 java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref create mode 100644 java/ql/test/kotlin/query-tests/UnreadLocal/test.kt diff --git a/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql b/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql index c2052344b7b..31697e561ed 100644 --- a/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql +++ b/java/ql/src/Violations of Best Practice/Dead Code/UnreadLocal.ql @@ -26,5 +26,10 @@ where not exists(getARead(v)) and // Discarded exceptions are covered by another query. not exists(CatchClause cc | cc.getVariable().getVariable() = v) and + // Exclude common Kotlin pattern to do something n times: `for(i in 1..n) { doSomething() } + not exists(EnhancedForStmt f | + f.getVariable().getVariable() = v and + f.getExpr().getType().(RefType).hasQualifiedName("kotlin.ranges", ["IntRange", "LongRange"]) + ) and not readImplicitly(v) select v, "Variable '" + v + "' is never read." diff --git a/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected new file mode 100644 index 00000000000..3c83d812b43 --- /dev/null +++ b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.expected @@ -0,0 +1,3 @@ +| test.kt:8:10:8:10 | int e | Variable 'int e' is never read. | +| test.kt:14:11:14:13 | int idx | Variable 'int idx' is never read. | +| test.kt:14:16:14:16 | int e | Variable 'int e' is never read. | diff --git a/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref new file mode 100644 index 00000000000..5a77117711e --- /dev/null +++ b/java/ql/test/kotlin/query-tests/UnreadLocal/UnreadLocal.qlref @@ -0,0 +1 @@ +Violations of Best Practice/Dead Code/UnreadLocal.ql diff --git a/java/ql/test/kotlin/query-tests/UnreadLocal/test.kt b/java/ql/test/kotlin/query-tests/UnreadLocal/test.kt new file mode 100644 index 00000000000..e1663d7c116 --- /dev/null +++ b/java/ql/test/kotlin/query-tests/UnreadLocal/test.kt @@ -0,0 +1,25 @@ +fun fn0(size: Int) { + for (idx in 1..size) { + println() + } +} + +fun fn1(a: Array) { + for (e in a) { + println() + } +} + +fun fn2(a: Array) { + for ((idx, e) in a.withIndex()) { + println() + } +} + +fun fn3() { + for (i in 1 until 10) { + println() + } +} + +// Diagnostic Matches: % Couldn't find a Java equivalent function to kotlin.Int.rangeTo in java.lang.Integer% From f3741ff1e44745ab18fa92b09cedfd860f9f6d09 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 3 Nov 2022 09:41:05 +0100 Subject: [PATCH 24/49] changes based on review --- .../lib/codeql/ruby/frameworks/core/Gem.qll | 22 ++++++++++++++----- ...ShellCommandConstructionCustomizations.qll | 4 ++-- .../UnsafeShellCommandConstruction.expected | 4 ++++ .../impl/unsafeShell.rb | 5 +++++ 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll index 41daf6ac3b8..9e417ad371a 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll @@ -8,7 +8,17 @@ private import codeql.ruby.ApiGraphs /** Provides modeling for the `Gem` module and `.gemspec` files. */ module Gem { - /** A .gemspec file that lists properties of a Ruby gem. */ + /** + * A .gemspec file that lists properties of a Ruby gem. + * The contents of a .gemspec file generally look like: + * ```Ruby + * Gem::Specification.new do |s| + * s.name = 'library-name' + * s.require_path = "lib" + * # more properties + * end + * ``` + */ class GemSpec instanceof File { API::Node specCall; @@ -25,13 +35,13 @@ module Gem { * Gets a value of the `name` property of this .gemspec file. * These properties are set using the `Gem::Specification.new` method. */ - private Expr getSpecProperty(string key) { + private Expr getSpecProperty(string name) { exists(Expr rhs | rhs = specCall .getBlock() .getParameter(0) - .getMethod(key + "=") + .getMethod(name + "=") .getParameter(0) .asSink() .asExpr() @@ -57,14 +67,14 @@ module Gem { result = "lib" // the default is "lib" } - /** Gets a file that is loaded when the gem is required. */ - private File getAnRequiredFile() { + /** Gets a file that could be loaded when the gem is required. */ + private File getAPossiblyRequiredFile() { result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*() } /** Gets a class/module that is exported by this gem. */ private ModuleBase getAPublicModule() { - result.(Toplevel).getLocation().getFile() = getAnRequiredFile() + result.(Toplevel).getLocation().getFile() = getAPossiblyRequiredFile() or result = getAPublicModule().getAModule() or diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll index b89743fe68e..515b563c1e1 100644 --- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -65,11 +65,11 @@ module UnsafeShellCommandConstruction { * A string constructed from a string-literal (e.g. `"foo #{sink}"`), * where the resulting string ends up being executed as a shell command. */ - class StringFormatAsSink extends Sink { + class StringInterpolationAsSink extends Sink { Concepts::SystemCommandExecution s; Ast::StringLiteral lit; - StringFormatAsSink() { + StringInterpolationAsSink() { isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and this.asExpr().getExpr() = lit.getComponent(_) } diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected index 3cc91bebdd9..964fb4433ce 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -8,6 +8,7 @@ edges | impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | | impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | | impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | +| impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | nodes | impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : | | impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} | @@ -27,6 +28,8 @@ nodes | impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} | | impl/unsafeShell.rb:37:10:37:10 | x : | semmle.label | x : | | impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:47:16:47:21 | target : | semmle.label | target : | +| impl/unsafeShell.rb:48:19:48:27 | #{...} | semmle.label | #{...} | subpaths #select | impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command | @@ -38,3 +41,4 @@ subpaths | impl/unsafeShell.rb:26:14:26:31 | "cat #{...}" | impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:23:15:23:23 | file_path | library input | impl/unsafeShell.rb:26:5:26:37 | call to popen | shell command | | impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command | | impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command | +| impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command | diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb index 52e3016ac91..f1fd9f685a8 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb @@ -42,4 +42,9 @@ class Foobar2 def thisIsSafe() IO.popen("echo #{id('foo')}", "w") # OK - only using constants. end + + # class methods + def self.foo(target) + IO.popen("cat #{target}", "w") # NOT OK + end end From c07db098a73be1c0168ca93807bda37aeb84d0fa Mon Sep 17 00:00:00 2001 From: alexet Date: Fri, 4 Nov 2022 15:27:21 +0000 Subject: [PATCH 25/49] QLSpec: Adress comments from review --- .../ql-language-specification.rst | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/codeql/ql-language-reference/ql-language-specification.rst b/docs/codeql/ql-language-reference/ql-language-specification.rst index ac60098319f..765a5fda4ab 100644 --- a/docs/codeql/ql-language-reference/ql-language-specification.rst +++ b/docs/codeql/ql-language-reference/ql-language-specification.rst @@ -827,7 +827,7 @@ The types specified after the ``extends`` keyword are the *base types* of the cl The types specified after the ``instanceof`` keyword are the *instanceof types* of the class. -A class type is said to *inherit* from the base types of the associated class type. In addition, inheritance is transitive: If a type ``A`` inherits from a type ``B``, and ``B`` inherits from a type ``C``, then ``A`` inherits from ``C``. +A class type is said to *inherit* from the base types. In addition, inheritance is transitive: If a type ``A`` inherits from a type ``B``, and ``B`` inherits from a type ``C``, then ``A`` inherits from ``C``. A class adds a mapping from the class name to the class declaration to the current module's declared type environment. @@ -1186,11 +1186,11 @@ If the call includes a closure, then all declared predicate arguments, the enclo A call to a member predicate may be a *direct* call: - If the receiver is not a super expression it is not direct. - - The receiver is ``A.super`` and ``A`` is an instanceof type and not a base type then it is not direct. - - The receiver is ``A.super`` and ``A`` is a base type type and not an instanceof type then it is direct. - - If the receiver is ``A.super`` and ``A`` is a base type and an instanceof type then the call is not valid - - The receiver is ``super`` and the member predicate is in the exported member predicate environment of an instanceof type and not in the exported member predicate environment of a base type then it isn't direct. - - The receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and not in the exported member predicate environment of an instanceof type then it is direct. + - If the receiver is ``A.super`` and ``A`` is an instanceof type and not a base type then it is not direct. + - If the receiver is ``A.super`` and ``A`` is a base type type and not an instanceof type then it is direct. + - If the receiver is ``A.super`` and ``A`` is a base type and an instanceof type then the call is not valid. + - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of an instanceof type and not in the exported member predicate environment of a base type then it isn't direct. + - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and not in the exported member predicate environment of an instanceof type then it is direct. - If the receiver is ``super`` and the member predicate is in the exported member predicate environment of a base type and in the exported member predicate environment of an instanceof type then the call is not valid. If the call resolves to a member predicate, then the *receiver values* are as follows. If the call has a receiver, then the receiver values are the values of that receiver. If the call does not have a receiver, then the single receiver value is the value of ``this`` in the contextual named tuple. From 1756feae716c274a72541813cd8d0733fa7ddbda Mon Sep 17 00:00:00 2001 From: Karim Ali Date: Mon, 7 Nov 2022 13:20:02 +0200 Subject: [PATCH 26/49] address docs review --- swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp | 2 +- swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp index ca8d65d9982..99fd0168225 100644 --- a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelp @@ -11,7 +11,7 @@ -

    The following example shows a few cases of hashing input data. In the 'BAD' cases, the salt is constant, making the generated hashes vulnerable to dictionary attakcs. In the 'GOOD' cases, the salt is randomly generated, which protects the hashed data against recovery.

    +

    The following example shows a few cases of hashing input data. In the 'BAD' cases, the salt is constant, making the generated hashes vulnerable to dictionary attacks. In the 'GOOD' cases, the salt is randomly generated, which protects the hashed data against recovery.

     diff --git a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql index 32bc1325147..9cd6dbd5ace 100644 --- a/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql +++ b/swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql @@ -1,6 +1,6 @@ /** - * @name Constant salt - * @description Using constant salts for password hashing is not secure, because potential attackers can pre-compute the hash value via dictionary attacks. + * @name Use of constant salts + * @description Using constant salts for password hashing is not secure because potential attackers can precompute the hash value via dictionary attacks. * @kind path-problem * @problem.severity error * @security-severity 7.5 From be808deb59235b5f673ce02206fea4a8bd086edf Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Nov 2022 12:59:44 +0000 Subject: [PATCH 27/49] JS: Bump minor version of ML-powered model pack --- .../ql/experimental/adaptivethreatmodeling/model/qlpack.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml index 4cf8ca1dbef..e13ad637280 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml @@ -1,5 +1,5 @@ name: codeql/javascript-experimental-atm-model -version: 0.2.1 +version: 0.3.0 groups: - javascript - experimental From a1e0bf022e965547a253b74f5a2af2ab88764a71 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Nov 2022 13:00:27 +0000 Subject: [PATCH 28/49] ATM: Update model pack dependency of ML-powered model building and query packs --- .../adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml | 2 +- .../adaptivethreatmodeling/modelbuilding/qlpack.yml | 2 +- .../adaptivethreatmodeling/src/codeql-pack.lock.yml | 2 +- .../ql/experimental/adaptivethreatmodeling/src/qlpack.yml | 2 +- .../adaptivethreatmodeling/test/codeql-pack.lock.yml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml index 46630e15fd9..fc9f87b3bbb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/codeql-pack.lock.yml @@ -1,6 +1,6 @@ --- dependencies: codeql/javascript-experimental-atm-model: - version: 0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d + version: 0.3.0 compiled: false lockVersion: 1.0.0 diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml index 0c2521e5411..e6657138f1c 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/qlpack.yml @@ -6,4 +6,4 @@ groups: - experimental dependencies: codeql/javascript-experimental-atm-lib: ${workspace} - codeql/javascript-experimental-atm-model: "0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d" + codeql/javascript-experimental-atm-model: "0.3.0" diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml b/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml index 46630e15fd9..fc9f87b3bbb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/codeql-pack.lock.yml @@ -1,6 +1,6 @@ --- dependencies: codeql/javascript-experimental-atm-model: - version: 0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d + version: 0.3.0 compiled: false lockVersion: 1.0.0 diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml index 1b2e3da82e9..682997a7fb5 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml @@ -8,4 +8,4 @@ groups: - experimental dependencies: codeql/javascript-experimental-atm-lib: ${workspace} - codeql/javascript-experimental-atm-model: "0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d" + codeql/javascript-experimental-atm-model: "0.3.0" diff --git a/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml b/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml index 46630e15fd9..fc9f87b3bbb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/test/codeql-pack.lock.yml @@ -1,6 +1,6 @@ --- dependencies: codeql/javascript-experimental-atm-model: - version: 0.2.1-2022-09-06-08h55m54s.bubbly-basin-xpztl8fh.f3c3c9360a727959e428ecc6932257e6a546dc65d8a9baac525a49247123822d + version: 0.3.0 compiled: false lockVersion: 1.0.0 From 268a990aa6174b7850e13f0ab88ddbf00969762b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Nov 2022 13:00:28 +0000 Subject: [PATCH 29/49] JS: Bump version of ML-powered model pack to 0.3.1 --- .../ql/experimental/adaptivethreatmodeling/model/qlpack.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml index e13ad637280..40b611fc72a 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/model/qlpack.yml @@ -1,5 +1,5 @@ name: codeql/javascript-experimental-atm-model -version: 0.3.0 +version: 0.3.1 groups: - javascript - experimental From 82277d8f565ae97cc928441fe05396f006545af6 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Nov 2022 13:00:28 +0000 Subject: [PATCH 30/49] JS: Bump minor version of ML-powered library and query packs --- .../ql/experimental/adaptivethreatmodeling/lib/qlpack.yml | 2 +- .../ql/experimental/adaptivethreatmodeling/src/qlpack.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml index 3bb1669cdff..dda5982f82e 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml @@ -1,5 +1,5 @@ name: codeql/javascript-experimental-atm-lib -version: 0.3.7 +version: 0.4.0 extractor: javascript library: true groups: diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml index 682997a7fb5..a25ef767105 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml @@ -1,6 +1,6 @@ name: codeql/javascript-experimental-atm-queries language: javascript -version: 0.3.7 +version: 0.4.0 suites: codeql-suites defaultSuiteFile: codeql-suites/javascript-atm-code-scanning.qls groups: From 69df9f9daa620cf2fada0b241e1ee78e558ba84c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Nov 2022 13:06:46 +0000 Subject: [PATCH 31/49] JS: Bump version of ML-powered library and query packs to 0.4.1 --- .../ql/experimental/adaptivethreatmodeling/lib/qlpack.yml | 2 +- .../ql/experimental/adaptivethreatmodeling/src/qlpack.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml index dda5982f82e..e0571f38255 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml @@ -1,5 +1,5 @@ name: codeql/javascript-experimental-atm-lib -version: 0.4.0 +version: 0.4.1 extractor: javascript library: true groups: diff --git a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml index a25ef767105..cab87ce0e33 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml +++ b/javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml @@ -1,6 +1,6 @@ name: codeql/javascript-experimental-atm-queries language: javascript -version: 0.4.0 +version: 0.4.1 suites: codeql-suites defaultSuiteFile: codeql-suites/javascript-atm-code-scanning.qls groups: From 3f871a08e2fa2ab7116e05d29b8a7a2bf4f49e9f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 7 Nov 2022 16:29:10 +0100 Subject: [PATCH 32/49] apply suggestions from doc review Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- .../2022-10-10-unsafe-shell-command-construction.md | 2 +- .../security/cwe-078/UnsafeShellCommandConstruction.qhelp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md index d61d32dcc5f..fba6a9304cf 100644 --- a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md +++ b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md @@ -1,4 +1,4 @@ --- category: newQuery --- -* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely constructs shell commands from their inputs. +* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs. diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp index 88cea1d80d3..4231f7cb0b4 100644 --- a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp @@ -55,7 +55,7 @@

    To avoid such potentially catastrophic behaviors, provide the - inputs from exported functions as an argument that does not + input from exported functions as an argument that does not get interpreted by a shell:

    From 03aa8df8e2aab8fcb86208d9d2f92d4e257a52b2 Mon Sep 17 00:00:00 2001 From: Harry Maclean Date: Tue, 8 Nov 2022 09:43:49 +1300 Subject: [PATCH 33/49] Ruby: Cosmetic change --- ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll index bf8d46e3e7e..88fe072395d 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll @@ -94,13 +94,13 @@ module ActiveSupport { * `Object#try!` behaves similarly but raises `NoMethodError` if the * receiver is not `nil` and does not respond to the method. */ - class TryCallCodeExecution extends CodeExecution::Range, DataFlow::CallNode { + class TryCallCodeExecution extends CodeExecution::Range instanceof DataFlow::CallNode { TryCallCodeExecution() { this.asExpr().getExpr() instanceof UnknownMethodCall and this.getMethodName() = ["try", "try!"] } - override DataFlow::Node getCode() { result = this.getArgument(0) } + override DataFlow::Node getCode() { result = super.getArgument(0) } override predicate runsArbitraryCode() { none() } } From 21adcca06592876b9db20ec19fc794bc0ba33663 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 08:53:36 +0100 Subject: [PATCH 34/49] Swift: add bitwise ops to `PrintAst` test --- .../expressions/expressions.swift | 9 +++- .../test/library-tests/ast/PrintAst.expected | 41 +++++++++++++++++++ .../test/library-tests/ast/expressions.swift | 9 +++- 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/swift/ql/test/extractor-tests/expressions/expressions.swift b/swift/ql/test/extractor-tests/expressions/expressions.swift index bc720abe70f..42eb9d9fcb3 100644 --- a/swift/ql/test/extractor-tests/expressions/expressions.swift +++ b/swift/ql/test/extractor-tests/expressions/expressions.swift @@ -152,4 +152,11 @@ func test(a : A, keyPathInt : WritableKeyPath, keyPathB : WritableKeyPat var apply_keyPathInt = a[keyPath: keyPathInt] var apply_keyPathB = a[keyPath: keyPathB] var nested_apply = a[keyPath: keyPathB][keyPath: \B.x] -} \ No newline at end of file +} + +func bitwise() { + _ = ~1 + _ = 1 & 2 + _ = 1 | 2 + _ = 1 ^ 2 +} diff --git a/swift/ql/test/library-tests/ast/PrintAst.expected b/swift/ql/test/library-tests/ast/PrintAst.expected index 54af4da242c..6b837081217 100644 --- a/swift/ql/test/library-tests/ast/PrintAst.expected +++ b/swift/ql/test/library-tests/ast/PrintAst.expected @@ -4338,6 +4338,47 @@ expressions.swift: # 154| getPattern(0): [NamedPattern] nested_apply # 154| getElement(5): [ConcreteVarDecl] nested_apply # 154| Type = Int +# 157| [ConcreteFuncDecl] bitwise() +# 157| InterfaceType = () -> () +# 157| getBody(): [BraceStmt] { ... } +# 158| getElement(0): [AssignExpr] ... = ... +# 158| getDest(): [DiscardAssignmentExpr] _ +# 158| getSource(): [PrefixUnaryExpr] call to ~(_:) +# 158| getFunction(): [MethodRefExpr] .~(_:) +# 158| getBase(): [TypeExpr] Int.Type +# 158| getTypeRepr(): [TypeRepr] Int +# 158| getArgument(0): [Argument] : 1 +# 158| getExpr(): [IntegerLiteralExpr] 1 +# 159| getElement(1): [AssignExpr] ... = ... +# 159| getDest(): [DiscardAssignmentExpr] _ +# 159| getSource(): [BinaryExpr] ... .&(_:_:) ... +# 159| getFunction(): [MethodRefExpr] .&(_:_:) +# 159| getBase(): [TypeExpr] Int.Type +# 159| getTypeRepr(): [TypeRepr] Int +# 159| getArgument(0): [Argument] : 1 +# 159| getExpr(): [IntegerLiteralExpr] 1 +# 159| getArgument(1): [Argument] : 2 +# 159| getExpr(): [IntegerLiteralExpr] 2 +# 160| getElement(2): [AssignExpr] ... = ... +# 160| getDest(): [DiscardAssignmentExpr] _ +# 160| getSource(): [BinaryExpr] ... .|(_:_:) ... +# 160| getFunction(): [MethodRefExpr] .|(_:_:) +# 160| getBase(): [TypeExpr] Int.Type +# 160| getTypeRepr(): [TypeRepr] Int +# 160| getArgument(0): [Argument] : 1 +# 160| getExpr(): [IntegerLiteralExpr] 1 +# 160| getArgument(1): [Argument] : 2 +# 160| getExpr(): [IntegerLiteralExpr] 2 +# 161| getElement(3): [AssignExpr] ... = ... +# 161| getDest(): [DiscardAssignmentExpr] _ +# 161| getSource(): [BinaryExpr] ... .^(_:_:) ... +# 161| getFunction(): [MethodRefExpr] .^(_:_:) +# 161| getBase(): [TypeExpr] Int.Type +# 161| getTypeRepr(): [TypeRepr] Int +# 161| getArgument(0): [Argument] : 1 +# 161| getExpr(): [IntegerLiteralExpr] 1 +# 161| getArgument(1): [Argument] : 2 +# 161| getExpr(): [IntegerLiteralExpr] 2 patterns.swift: # 1| [ConcreteFuncDecl] basic_patterns() # 1| InterfaceType = () -> () diff --git a/swift/ql/test/library-tests/ast/expressions.swift b/swift/ql/test/library-tests/ast/expressions.swift index bc720abe70f..42eb9d9fcb3 100644 --- a/swift/ql/test/library-tests/ast/expressions.swift +++ b/swift/ql/test/library-tests/ast/expressions.swift @@ -152,4 +152,11 @@ func test(a : A, keyPathInt : WritableKeyPath, keyPathB : WritableKeyPat var apply_keyPathInt = a[keyPath: keyPathInt] var apply_keyPathB = a[keyPath: keyPathB] var nested_apply = a[keyPath: keyPathB][keyPath: \B.x] -} \ No newline at end of file +} + +func bitwise() { + _ = ~1 + _ = 1 & 2 + _ = 1 | 2 + _ = 1 ^ 2 +} From c86f59715381ee9064f51abba1a8b431a8bc08f1 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 08:25:08 +0100 Subject: [PATCH 35/49] Ruby: Add test for disjunctive guard --- .../barrier-guards/barrier-guards.expected | 32 ++++++++++++++++++- .../dataflow/barrier-guards/barrier-guards.ql | 8 +++++ .../dataflow/barrier-guards/barrier-guards.rb | 6 ++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index 783d414dad6..393b2beb304 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -1,4 +1,4 @@ -WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:8,3-15) +WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:9,3-15) oldStyleBarrierGuards | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true | | barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true | @@ -20,3 +20,33 @@ newStyleBarrierGuards | barrier-guards.rb:71:5:71:7 | foo | | barrier-guards.rb:83:5:83:7 | foo | | barrier-guards.rb:91:5:91:7 | foo | +controls +| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true | +| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false | +| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | true | +| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:12:5:12:7 | foo | false | +| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:16:5:16:7 | foo | true | +| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | false | +| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:22:5:22:7 | foo | false | +| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | true | +| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | false | +| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:30:5:30:7 | foo | true | +| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | true | +| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:40:5:40:7 | foo | false | +| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:44:5:46:5 | self | true | +| barrier-guards.rb:49:4:49:15 | ... == ... | barrier-guards.rb:50:5:53:5 | self | true | +| barrier-guards.rb:56:4:56:15 | ... == ... | barrier-guards.rb:57:5:57:13 | my_lambda | true | +| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | true | +| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:73:5:73:7 | foo | false | +| barrier-guards.rb:76:4:76:21 | call to include? | barrier-guards.rb:77:5:77:7 | foo | true | +| barrier-guards.rb:76:4:76:21 | call to include? | barrier-guards.rb:79:5:79:7 | foo | false | +| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | true | +| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:85:5:85:7 | foo | false | +| barrier-guards.rb:88:4:88:25 | ... == ... | barrier-guards.rb:89:5:89:7 | foo | true | +| barrier-guards.rb:88:4:88:25 | ... == ... | barrier-guards.rb:91:5:91:7 | foo | false | +| barrier-guards.rb:96:4:96:12 | call to condition | barrier-guards.rb:97:5:97:8 | bars | true | +| barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:101:5:101:7 | foo | true | +| barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql index 84a962ade35..7998164b6da 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql @@ -2,6 +2,7 @@ import codeql.ruby.dataflow.internal.DataFlowPublic import codeql.ruby.dataflow.BarrierGuards import codeql.ruby.controlflow.CfgNodes import codeql.ruby.controlflow.ControlFlowGraph +import codeql.ruby.controlflow.BasicBlocks import codeql.ruby.DataFlow query predicate oldStyleBarrierGuards( @@ -14,3 +15,10 @@ query predicate newStyleBarrierGuards(DataFlow::Node n) { n instanceof StringConstCompareBarrier or n instanceof StringConstArrayInclusionCallBarrier } + +query predicate controls(CfgNode condition, BasicBlock bb, SuccessorTypes::ConditionalSuccessor s) { + exists(ConditionBlock cb | + cb.controls(bb, s) and + condition = cb.getLastNode() + ) +} diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb index bc9599fd926..21a878e938c 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb @@ -102,3 +102,9 @@ if bars.include?(foo) else foo end + +if x or y then + foo +else + foo +end From 7ba068229734f68165abf0b082831a1b1d999522 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 08:59:59 +0100 Subject: [PATCH 36/49] Ruby: Split basic blocks around constant conditionals --- .../codeql/ruby/controlflow/BasicBlocks.qll | 24 +++++++++++++++++++ .../barrier-guards/barrier-guards.expected | 4 ++++ 2 files changed, 28 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll index 9ce0bf32fd7..30045054503 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll @@ -252,6 +252,30 @@ private module Cached { cfn.isJoin() or cfn.getAPredecessor().isBranch() + or + /* + * In cases such as + * + * ```rb + * if x or y + * foo + * else + * bar + * ``` + * + * we have a CFG that looks like + * + * x --false--> [false] x or y --false--> bar + * \ | + * --true--> y --false-- + * \ + * --true--> [true] x or y --true--> foo + * + * and we want to ensure that both `foo` and `bar` start a new basic block, + * in order to get a `ConditionalBlock` out of the disjunction. + */ + + exists(cfn.getAPredecessor(any(SuccessorTypes::ConditionalSuccessor s))) } /** diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index 393b2beb304..d2f5df8fcd9 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -49,4 +49,8 @@ controls | barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:4:106:9 | [true] ... or ... | barrier-guards.rb:107:5:107:7 | foo | true | | barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:7 | foo | false | From 072edad0fdc13b9684077fffd0405cf6b4878120 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 09:30:25 +0100 Subject: [PATCH 37/49] Swift: accept new test changes --- .../extractor-tests/expressions/all.expected | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/swift/ql/test/extractor-tests/expressions/all.expected b/swift/ql/test/extractor-tests/expressions/all.expected index edb8c16c545..d0068a17220 100644 --- a/swift/ql/test/extractor-tests/expressions/all.expected +++ b/swift/ql/test/extractor-tests/expressions/all.expected @@ -229,3 +229,30 @@ | expressions.swift:154:22:154:56 | \\...[...] | KeyPathApplicationExpr | | expressions.swift:154:33:154:33 | keyPathB | DeclRefExpr | | expressions.swift:154:52:154:55 | #keyPath(...) | KeyPathExpr | +| expressions.swift:158:3:158:3 | _ | DiscardAssignmentExpr | +| expressions.swift:158:3:158:8 | ... = ... | AssignExpr | +| expressions.swift:158:7:158:7 | .~(_:) | MethodRefExpr | +| expressions.swift:158:7:158:7 | Int.Type | TypeExpr | +| expressions.swift:158:7:158:8 | call to ~(_:) | PrefixUnaryExpr | +| expressions.swift:158:8:158:8 | 1 | IntegerLiteralExpr | +| expressions.swift:159:3:159:3 | _ | DiscardAssignmentExpr | +| expressions.swift:159:3:159:11 | ... = ... | AssignExpr | +| expressions.swift:159:7:159:7 | 1 | IntegerLiteralExpr | +| expressions.swift:159:7:159:11 | ... .&(_:_:) ... | BinaryExpr | +| expressions.swift:159:9:159:9 | .&(_:_:) | MethodRefExpr | +| expressions.swift:159:9:159:9 | Int.Type | TypeExpr | +| expressions.swift:159:11:159:11 | 2 | IntegerLiteralExpr | +| expressions.swift:160:3:160:3 | _ | DiscardAssignmentExpr | +| expressions.swift:160:3:160:11 | ... = ... | AssignExpr | +| expressions.swift:160:7:160:7 | 1 | IntegerLiteralExpr | +| expressions.swift:160:7:160:11 | ... .\|(_:_:) ... | BinaryExpr | +| expressions.swift:160:9:160:9 | .\|(_:_:) | MethodRefExpr | +| expressions.swift:160:9:160:9 | Int.Type | TypeExpr | +| expressions.swift:160:11:160:11 | 2 | IntegerLiteralExpr | +| expressions.swift:161:3:161:3 | _ | DiscardAssignmentExpr | +| expressions.swift:161:3:161:11 | ... = ... | AssignExpr | +| expressions.swift:161:7:161:7 | 1 | IntegerLiteralExpr | +| expressions.swift:161:7:161:11 | ... .^(_:_:) ... | BinaryExpr | +| expressions.swift:161:9:161:9 | .^(_:_:) | MethodRefExpr | +| expressions.swift:161:9:161:9 | Int.Type | TypeExpr | +| expressions.swift:161:11:161:11 | 2 | IntegerLiteralExpr | From 0d4a2239fcc7768e7cd0ed3635aaa7387013fd6b Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Tue, 8 Nov 2022 09:55:10 +0100 Subject: [PATCH 38/49] C++: Fix wrong return types and missing statement in dataflow test --- .../dataflow-tests/true_upon_entry.cpp | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp index d08e8b1ded3..923a7c0513d 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/true_upon_entry.cpp @@ -5,7 +5,7 @@ int source(); void sink(...); bool random(); -int test1() { +void test1() { int x = source(); for (int i = 0; i < 10; i++) { x = 0; @@ -13,7 +13,7 @@ int test1() { sink(x); // $ SPURIOUS: ir } -int test2(int iterations) { +void test2(int iterations) { int x = source(); for (int i = 0; i < iterations; i++) { x = 0; @@ -21,7 +21,7 @@ int test2(int iterations) { sink(x); // $ ast,ir } -int test3() { +void test3() { int x = 0; for (int i = 0; i < 10; i++) { x = source(); @@ -29,7 +29,7 @@ int test3() { sink(x); // $ ast,ir } -int test4() { +void test4() { int x = source(); for (int i = 0; i < 10; i++) { if (random()) @@ -39,7 +39,7 @@ int test4() { sink(x); // $ ast,ir } -int test5() { +void test5() { int x = source(); for (int i = 0; i < 10; i++) { if (random()) @@ -49,7 +49,7 @@ int test5() { sink(x); // $ ast,ir } -int test6() { +void test6() { int y; int x = source(); for (int i = 0; i < 10 && (y = 1); i++) { @@ -57,7 +57,7 @@ int test6() { sink(x); // $ ast,ir } -int test7() { +void test7() { int y; int x = source(); for (int i = 0; i < 10 && (y = 1); i++) { @@ -66,7 +66,7 @@ int test7() { sink(x); // $ SPURIOUS: ir } -int test8() { +void test8() { int x = source(); // It appears to the analysis that the condition can exit after `i < 10` // without having assigned to `x`. That is an effect of how the @@ -78,7 +78,7 @@ int test8() { sink(x); // $ SPURIOUS: ast,ir } -int test9() { +void test9() { int y; int x = source(); for (int i = 0; (y = 1) && i < 10; i++) { @@ -86,21 +86,21 @@ int test9() { sink(x); // $ ast,ir } -int test10() { +void test10() { int x = source(); for (int i = 0; (x = 1) && i < 10; i++) { } sink(x); // no flow } -int test10(int b, int d) { +void test10(int b, int d) { int i = 0; int x = source(); if (b) goto L; for (; i < 10; i += d) { x = 0; - L: + L: ; } sink(x); // $ ir MISSING: ast } From 37a69b4569ee4cb6cbbbcdeb46f3faa505776db8 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 10:51:30 +0100 Subject: [PATCH 39/49] Ruby: Avoid stage recomputation --- .../codeql/ruby/regexp/internal/RegExpConfiguration.qll | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll index 3c451b15b78..5c08054ac14 100644 --- a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll +++ b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll @@ -7,8 +7,7 @@ private import codeql.ruby.dataflow.internal.DataFlowImplForRegExp private import codeql.ruby.typetracking.TypeTracker private import codeql.ruby.ApiGraphs private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate -private import codeql.ruby.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl -private import codeql.ruby.dataflow.FlowSummary as FlowSummary +private import codeql.ruby.TaintTracking private import codeql.ruby.frameworks.core.String class RegExpConfiguration extends Configuration { @@ -38,8 +37,8 @@ class RegExpConfiguration extends Configuration { } override predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // include taint flow through `String` summaries, - FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false) and + // include taint flow through `String` summaries + TaintTracking::localTaintStep(nodeFrom, nodeTo) and nodeFrom.(DataFlowPrivate::SummaryNode).getSummarizedCallable() instanceof String::SummarizedCallable or From f0b9ca4bf9dfd54b3d469c2ac6da77b4f3e1a6cc Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 11:09:54 +0100 Subject: [PATCH 40/49] Ruby: Add more guards tests --- .../barrier-guards/barrier-guards.expected | 19 ++++++++++++++++--- .../dataflow/barrier-guards/barrier-guards.rb | 14 +++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index d2f5df8fcd9..a50542e8df3 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -49,8 +49,21 @@ controls | barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | -| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:7 | foo | false | -| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:8 | bars | false | | barrier-guards.rb:106:4:106:9 | [true] ... or ... | barrier-guards.rb:107:5:107:7 | foo | true | | barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | -| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:112:4:112:10 | [true] ... and ... | true | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:112:10:112:10 | self | true | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:112:4:112:10 | [false] ... and ... | barrier-guards.rb:115:5:115:8 | bars | false | +| barrier-guards.rb:112:4:112:10 | [true] ... and ... | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:112:10:112:10 | call to y | barrier-guards.rb:112:4:112:10 | [true] ... and ... | true | +| barrier-guards.rb:112:10:112:10 | call to y | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:118:4:118:8 | [false] not ... | barrier-guards.rb:121:5:121:8 | bars | false | +| barrier-guards.rb:118:4:118:8 | [true] not ... | barrier-guards.rb:119:5:119:7 | foo | true | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [false] not ... | true | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [true] not ... | false | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:119:5:119:7 | foo | false | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:121:5:121:8 | bars | true | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb index 21a878e938c..5ead7bc04cf 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb @@ -106,5 +106,17 @@ end if x or y then foo else - foo + bars +end + +if x and y then + foo +else + bars +end + +if not x then + foo +else + bars end From 2aa528852e9af9a752127703768b41b512d9a304 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 4 Nov 2022 08:07:04 +0100 Subject: [PATCH 41/49] Swift: add possibility to specify null class --- swift/codegen/generators/cppgen.py | 33 ++++--- swift/codegen/generators/dbschemegen.py | 34 +++++-- swift/codegen/generators/qlgen.py | 4 +- swift/codegen/generators/trapgen.py | 4 +- swift/codegen/lib/cpp.py | 21 +++-- swift/codegen/lib/schema/defs.py | 2 + swift/codegen/lib/schema/schema.py | 28 +++++- swift/codegen/test/test_cppgen.py | 19 ++++ swift/codegen/test/test_dbschemegen.py | 112 +++++++++++++++++++++++- swift/codegen/test/test_schema.py | 56 +++++++++++- 10 files changed, 281 insertions(+), 32 deletions(-) diff --git a/swift/codegen/generators/cppgen.py b/swift/codegen/generators/cppgen.py index 0ea5fbec2b8..45f0d98949b 100644 --- a/swift/codegen/generators/cppgen.py +++ b/swift/codegen/generators/cppgen.py @@ -12,15 +12,14 @@ Each class in the schema gets a corresponding `struct` in `TrapClasses.h`, where """ import functools -import pathlib -from typing import Dict +import typing import inflection from swift.codegen.lib import cpp, schema -def _get_type(t: str) -> str: +def _get_type(t: str, add_or_none_except: typing.Optional[str] = None) -> str: if t is None: # this is a predicate return "bool" @@ -29,11 +28,15 @@ def _get_type(t: str) -> str: if t == "boolean": return "bool" if t[0].isupper(): - return f"TrapLabel<{t}Tag>" + if add_or_none_except is not None and t != add_or_none_except: + suffix = "OrNone" + else: + suffix = "" + return f"TrapLabel<{t}{suffix}Tag>" return t -def _get_field(cls: schema.Class, p: schema.Property) -> cpp.Field: +def _get_field(cls: schema.Class, p: schema.Property, add_or_none_except: typing.Optional[str] = None) -> cpp.Field: trap_name = None if not p.is_single: trap_name = inflection.camelize(f"{cls.name}_{p.name}") @@ -41,7 +44,7 @@ def _get_field(cls: schema.Class, p: schema.Property) -> cpp.Field: trap_name = inflection.pluralize(trap_name) args = dict( field_name=p.name + ("_" if p.name in cpp.cpp_keywords else ""), - type=_get_type(p.type), + base_type=_get_type(p.type, add_or_none_except), is_optional=p.is_optional, is_repeated=p.is_repeated, is_predicate=p.is_predicate, @@ -52,8 +55,13 @@ def _get_field(cls: schema.Class, p: schema.Property) -> cpp.Field: class Processor: - def __init__(self, data: Dict[str, schema.Class]): - self._classmap = data + def __init__(self, data: schema.Schema): + self._classmap = data.classes + if data.null: + root_type = next(iter(data.classes)) + self._add_or_none_except = root_type + else: + self._add_or_none_except = None @functools.lru_cache(maxsize=None) def _get_class(self, name: str) -> cpp.Class: @@ -64,7 +72,10 @@ class Processor: return cpp.Class( name=name, bases=[self._get_class(b) for b in cls.bases], - fields=[_get_field(cls, p) for p in cls.properties if "cpp_skip" not in p.pragmas], + fields=[ + _get_field(cls, p, self._add_or_none_except) + for p in cls.properties if "cpp_skip" not in p.pragmas + ], final=not cls.derived, trap_name=trap_name, ) @@ -78,8 +89,8 @@ class Processor: def generate(opts, renderer): assert opts.cpp_output - processor = Processor(schema.load_file(opts.schema).classes) + processor = Processor(schema.load_file(opts.schema)) out = opts.cpp_output for dir, classes in processor.get_classes().items(): renderer.render(cpp.ClassList(classes, opts.schema, - include_parent=bool(dir)), out / dir / "TrapClasses") + include_parent=bool(dir)), out / dir / "TrapClasses") diff --git a/swift/codegen/generators/dbschemegen.py b/swift/codegen/generators/dbschemegen.py index 5e04b412646..b750a9d6e12 100755 --- a/swift/codegen/generators/dbschemegen.py +++ b/swift/codegen/generators/dbschemegen.py @@ -13,6 +13,7 @@ Moreover: as columns The type hierarchy will be translated to corresponding `union` declarations. """ +import typing import inflection @@ -23,14 +24,21 @@ from typing import Set, List log = logging.getLogger(__name__) -def dbtype(typename): - """ translate a type to a dbscheme counterpart, using `@lower_underscore` format for classes """ +def dbtype(typename: str, add_or_none_except: typing.Optional[str] = None) -> str: + """ translate a type to a dbscheme counterpart, using `@lower_underscore` format for classes. + For class types, appends an underscore followed by `null` if provided + """ if typename[0].isupper(): - return "@" + inflection.underscore(typename) + underscored = inflection.underscore(typename) + if add_or_none_except is not None and typename != add_or_none_except: + suffix = "_or_none" + else: + suffix = "" + return f"@{underscored}{suffix}" return typename -def cls_to_dbscheme(cls: schema.Class): +def cls_to_dbscheme(cls: schema.Class, add_or_none_except: typing.Optional[str] = None): """ Yield all dbscheme entities needed to model class `cls` """ if cls.derived: yield Union(dbtype(cls.name), (dbtype(c) for c in cls.derived)) @@ -48,7 +56,7 @@ def cls_to_dbscheme(cls: schema.Class): columns=[ Column("id", type=dbtype(cls.name), binding=binding), ] + [ - Column(f.name, dbtype(f.type)) for f in cls.properties if f.is_single + Column(f.name, dbtype(f.type, add_or_none_except)) for f in cls.properties if f.is_single ], dir=dir, ) @@ -61,7 +69,7 @@ def cls_to_dbscheme(cls: schema.Class): columns=[ Column("id", type=dbtype(cls.name)), Column("index", type="int"), - Column(inflection.singularize(f.name), dbtype(f.type)), + Column(inflection.singularize(f.name), dbtype(f.type, add_or_none_except)), ], dir=dir, ) @@ -71,7 +79,7 @@ def cls_to_dbscheme(cls: schema.Class): name=inflection.tableize(f"{cls.name}_{f.name}"), columns=[ Column("id", type=dbtype(cls.name)), - Column(f.name, dbtype(f.type)), + Column(f.name, dbtype(f.type, add_or_none_except)), ], dir=dir, ) @@ -87,7 +95,17 @@ def cls_to_dbscheme(cls: schema.Class): def get_declarations(data: schema.Schema): - return [d for cls in data.classes.values() for d in cls_to_dbscheme(cls)] + add_or_none_except = data.root_class.name if data.null else None + declarations = [d for cls in data.classes.values() for d in cls_to_dbscheme(cls, add_or_none_except)] + if data.null: + property_classes = { + prop.type for cls in data.classes.values() for prop in cls.properties + if cls.name != data.null and prop.type and prop.type[0].isupper() + } + declarations += [ + Union(dbtype(t, data.null), [dbtype(t), dbtype(data.null)]) for t in sorted(property_classes) + ] + return declarations def get_includes(data: schema.Schema, include_dir: pathlib.Path, swift_dir: pathlib.Path): diff --git a/swift/codegen/generators/qlgen.py b/swift/codegen/generators/qlgen.py index de2cddd981b..2172a9517e6 100755 --- a/swift/codegen/generators/qlgen.py +++ b/swift/codegen/generators/qlgen.py @@ -147,7 +147,7 @@ def get_ql_property(cls: schema.Class, prop: schema.Property, prev_child: str = return ql.Property(**args) -def get_ql_class(cls: schema.Class, lookup: typing.Dict[str, schema.Class]): +def get_ql_class(cls: schema.Class): pragmas = {k: True for k in cls.pragmas if k.startswith("ql")} prev_child = "" properties = [] @@ -314,7 +314,7 @@ def generate(opts, renderer): data = schema.load_file(input) - classes = {name: get_ql_class(cls, data.classes) for name, cls in data.classes.items()} + classes = {name: get_ql_class(cls) for name, cls in data.classes.items()} if not classes: raise NoClasses root = next(iter(classes.values())) diff --git a/swift/codegen/generators/trapgen.py b/swift/codegen/generators/trapgen.py index 22c46e6e146..f01203e9272 100755 --- a/swift/codegen/generators/trapgen.py +++ b/swift/codegen/generators/trapgen.py @@ -41,10 +41,10 @@ def get_cpp_type(schema_type: str): def get_field(c: dbscheme.Column): args = { "field_name": c.schema_name, - "type": c.type, + "base_type": c.type, } args.update(cpp.get_field_override(c.schema_name)) - args["type"] = get_cpp_type(args["type"]) + args["base_type"] = get_cpp_type(args["base_type"]) return cpp.Field(**args) diff --git a/swift/codegen/lib/cpp.py b/swift/codegen/lib/cpp.py index acebe616d7b..27bac12231b 100644 --- a/swift/codegen/lib/cpp.py +++ b/swift/codegen/lib/cpp.py @@ -16,7 +16,7 @@ cpp_keywords = {"alignas", "alignof", "and", "and_eq", "asm", "atomic_cancel", " "xor", "xor_eq"} _field_overrides = [ - (re.compile(r"(start|end)_(line|column)|(.*_)?index|width|num_.*"), {"type": "unsigned"}), + (re.compile(r"(start|end)_(line|column)|(.*_)?index|width|num_.*"), {"base_type": "unsigned"}), (re.compile(r"(.*)_"), lambda m: {"field_name": m[1]}), ] @@ -32,7 +32,7 @@ def get_field_override(field: str): @dataclass class Field: field_name: str - type: str + base_type: str is_optional: bool = False is_repeated: bool = False is_predicate: bool = False @@ -40,13 +40,18 @@ class Field: first: bool = False def __post_init__(self): - if self.is_optional: - self.type = f"std::optional<{self.type}>" - if self.is_repeated: - self.type = f"std::vector<{self.type}>" if self.field_name in cpp_keywords: self.field_name += "_" + @property + def type(self) -> str: + type = self.base_type + if self.is_optional: + type = f"std::optional<{type}>" + if self.is_repeated: + type = f"std::vector<{type}>" + return type + # using @property breaks pystache internals here def get_streamer(self): if self.type == "std::string": @@ -60,6 +65,10 @@ class Field: def is_single(self): return not (self.is_optional or self.is_repeated or self.is_predicate) + @property + def is_label(self): + return self.base_type.startswith("TrapLabel<") + @dataclass class Trap: diff --git a/swift/codegen/lib/schema/defs.py b/swift/codegen/lib/schema/defs.py index 6470fb789a1..9657d587f85 100644 --- a/swift/codegen/lib/schema/defs.py +++ b/swift/codegen/lib/schema/defs.py @@ -115,6 +115,8 @@ child = _ChildModifier() doc = _DocModifier desc = _DescModifier +use_for_null = _annotate(null=True) + qltest = _Namespace( skip=_Pragma("qltest_skip"), collapse_hierarchy=_Pragma("qltest_collapse_hierarchy"), diff --git a/swift/codegen/lib/schema/schema.py b/swift/codegen/lib/schema/schema.py index e92af734a1c..72b56ad19af 100644 --- a/swift/codegen/lib/schema/schema.py +++ b/swift/codegen/lib/schema/schema.py @@ -55,6 +55,14 @@ class Property: def is_predicate(self) -> bool: return self.kind == self.Kind.PREDICATE + @property + def has_class_type(self) -> bool: + return bool(self.type) and self.type[0].isupper() + + @property + def has_builtin_type(self) -> bool: + return bool(self.type) and self.type[0].islower() + SingleProperty = functools.partial(Property, Property.Kind.SINGLE) OptionalProperty = functools.partial(Property, Property.Kind.OPTIONAL) @@ -104,6 +112,16 @@ class Class: class Schema: classes: Dict[str, Class] = field(default_factory=dict) includes: Set[str] = field(default_factory=set) + null: Optional[str] = None + + @property + def root_class(self): + # always the first in the dictionary + return next(iter(self.classes.values())) + + @property + def null_class(self): + return self.classes[self.null] if self.null else None predicate_marker = object() @@ -195,6 +213,8 @@ def _get_class(cls: type) -> Class: raise Error(f"Class name must be capitalized, found {cls.__name__}") if len({b._group for b in cls.__bases__ if hasattr(b, "_group")}) > 1: raise Error(f"Bases with mixed groups for {cls.__name__}") + if any(getattr(b, "_null", False) for b in cls.__bases__): + raise Error(f"Null class cannot be derived") return Class(name=cls.__name__, bases=[b.__name__ for b in cls.__bases__ if b is not object], derived={d.__name__ for d in cls.__subclasses__()}, @@ -233,6 +253,7 @@ def load(m: types.ModuleType) -> Schema: known = {"int", "string", "boolean"} known.update(n for n in m.__dict__ if not n.startswith("__")) import swift.codegen.lib.schema.defs as defs + null = None for name, data in m.__dict__.items(): if hasattr(defs, name): continue @@ -247,8 +268,13 @@ def load(m: types.ModuleType) -> Schema: f"Only one root class allowed, found second root {name}") cls.check_types(known) classes[name] = cls + if getattr(data, "_null", False): + if null is not None: + raise Error(f"Null class {null} already defined, second null class {name} not allowed") + null = name + cls.is_null_class = True - return Schema(includes=includes, classes=_toposort_classes_by_group(classes)) + return Schema(includes=includes, classes=_toposort_classes_by_group(classes), null=null) def load_file(path: pathlib.Path) -> Schema: diff --git a/swift/codegen/test/test_cppgen.py b/swift/codegen/test/test_cppgen.py index c2214d7775a..df4925cb70e 100644 --- a/swift/codegen/test/test_cppgen.py +++ b/swift/codegen/test/test_cppgen.py @@ -80,6 +80,25 @@ def test_class_with_field(generate, type, expected, property_cls, optional, repe ] +def test_class_field_with_null(generate, input): + input.null = "Null" + a = cpp.Class(name="A") + assert generate([ + schema.Class(name="A", derived={"B"}), + schema.Class(name="B", bases=["A"], properties=[ + schema.SingleProperty("x", "A"), + schema.SingleProperty("y", "B"), + ]) + ]) == [ + a, + cpp.Class(name="B", bases=[a], final=True, trap_name="Bs", + fields=[ + cpp.Field("x", "TrapLabel"), + cpp.Field("y", "TrapLabel"), + ]), + ] + + def test_class_with_predicate(generate): assert generate([ schema.Class(name="MyClass", properties=[ diff --git a/swift/codegen/test/test_dbschemegen.py b/swift/codegen/test/test_dbschemegen.py index a302ba8f155..0fb71fbc35b 100644 --- a/swift/codegen/test/test_dbschemegen.py +++ b/swift/codegen/test/test_dbschemegen.py @@ -18,8 +18,9 @@ def dir_param(request): @pytest.fixture def generate(opts, input, renderer): - def func(classes): + def func(classes, null=None): input.classes = {cls.name: cls for cls in classes} + input.null = null (out, data), = run_generation(dbschemegen.generate, opts, renderer).items() assert out is opts.dbscheme return data @@ -359,5 +360,114 @@ def test_class_with_derived_and_repeated_property(generate, dir_param): ) +def test_null_class(generate): + assert generate([ + schema.Class( + name="Base", + derived={"W", "X", "Y", "Z", "Null"}, + ), + schema.Class( + name="W", + bases=["Base"], + properties=[ + schema.SingleProperty("w", "W"), + schema.SingleProperty("x", "X"), + schema.OptionalProperty("y", "Y"), + schema.RepeatedProperty("z", "Z"), + ] + ), + schema.Class( + name="X", + bases=["Base"], + ), + schema.Class( + name="Y", + bases=["Base"], + ), + schema.Class( + name="Z", + bases=["Base"], + ), + schema.Class( + name="Null", + bases=["Base"], + ), + ], null="Null") == dbscheme.Scheme( + src=schema_file, + includes=[], + declarations=[ + dbscheme.Union( + lhs="@base", + rhs=["@null", "@w", "@x", "@y", "@z"], + ), + dbscheme.Table( + name="ws", + columns=[ + dbscheme.Column('id', '@w', binding=True), + dbscheme.Column('w', '@w_or_none'), + dbscheme.Column('x', '@x_or_none'), + ], + ), + dbscheme.Table( + name="w_ies", + keyset=dbscheme.KeySet(["id"]), + columns=[ + dbscheme.Column('id', '@w'), + dbscheme.Column('y', '@y_or_none'), + ], + ), + dbscheme.Table( + name="w_zs", + keyset=dbscheme.KeySet(["id", "index"]), + columns=[ + dbscheme.Column('id', '@w'), + dbscheme.Column('index', 'int'), + dbscheme.Column('z', '@z_or_none'), + ], + ), + dbscheme.Table( + name="xes", + columns=[ + dbscheme.Column('id', '@x', binding=True), + ], + ), + dbscheme.Table( + name="ys", + columns=[ + dbscheme.Column('id', '@y', binding=True), + ], + ), + dbscheme.Table( + name="zs", + columns=[ + dbscheme.Column('id', '@z', binding=True), + ], + ), + dbscheme.Table( + name="nulls", + columns=[ + dbscheme.Column('id', '@null', binding=True), + ], + ), + dbscheme.Union( + lhs="@w_or_none", + rhs=["@w", "@null"], + ), + dbscheme.Union( + lhs="@x_or_none", + rhs=["@x", "@null"], + ), + dbscheme.Union( + lhs="@y_or_none", + rhs=["@y", "@null"], + ), + dbscheme.Union( + lhs="@z_or_none", + rhs=["@z", "@null"], + ), + ], + ) + + if __name__ == '__main__': sys.exit(pytest.main([__file__] + sys.argv[1:])) diff --git a/swift/codegen/test/test_schema.py b/swift/codegen/test/test_schema.py index 457b60ce680..98f3c8aee41 100644 --- a/swift/codegen/test/test_schema.py +++ b/swift/codegen/test/test_schema.py @@ -13,6 +13,8 @@ def test_empty_schema(): assert data.classes == {} assert data.includes == set() + assert data.null is None + assert data.null_class is None def test_one_empty_class(): @@ -24,6 +26,7 @@ def test_one_empty_class(): assert data.classes == { 'MyClass': schema.Class('MyClass'), } + assert data.root_class is data.classes['MyClass'] def test_two_empty_classes(): @@ -39,6 +42,7 @@ def test_two_empty_classes(): 'MyClass1': schema.Class('MyClass1', derived={'MyClass2'}), 'MyClass2': schema.Class('MyClass2', bases=['MyClass1']), } + assert data.root_class is data.classes['MyClass1'] def test_no_external_bases(): @@ -452,7 +456,8 @@ def test_property_docstring_newline(): property.""") assert data.classes == { - 'A': schema.Class('A', properties=[schema.SingleProperty('x', 'int', description=["very important", "property."])]) + 'A': schema.Class('A', + properties=[schema.SingleProperty('x', 'int', description=["very important", "property."])]) } @@ -566,5 +571,54 @@ def test_class_default_doc_name(): } +def test_null_class(): + @schema.load + class data: + class Root: + pass + + @defs.use_for_null + class Null(Root): + pass + + assert data.classes == { + 'Root': schema.Class('Root', derived={'Null'}), + 'Null': schema.Class('Null', bases=['Root']), + } + assert data.null == 'Null' + assert data.null_class is data.classes[data.null] + + +def test_null_class_cannot_be_derived(): + with pytest.raises(schema.Error): + @schema.load + class data: + class Root: + pass + + @defs.use_for_null + class Null(Root): + pass + + class Impossible(Null): + pass + + +def test_null_class_cannot_be_defined_multiple_times(): + with pytest.raises(schema.Error): + @schema.load + class data: + class Root: + pass + + @defs.use_for_null + class Null1(Root): + pass + + @defs.use_for_null + class Null2(Root): + pass + + if __name__ == '__main__': sys.exit(pytest.main([__file__] + sys.argv[1:])) From 4411852e59223f425620ed6376a4c50bfef008e9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 8 Nov 2022 11:33:10 +0100 Subject: [PATCH 42/49] Add BitwiseOperation.qll --- .../swift/elements/expr/BitwiseOperation.qll | 108 ++++++++++++++++++ swift/ql/lib/swift.qll | 1 + .../extractor-tests/expressions/all.expected | 14 +++ .../expressions/expressions.swift | 2 + .../test/library-tests/ast/PrintAst.expected | 20 ++++ .../test/library-tests/ast/expressions.swift | 2 + .../bitwiseopration/bitwiseoperation.expected | 6 + .../expr/bitwiseopration/bitwiseoperation.ql | 22 ++++ .../bitwiseopration/bitwiseoperation.swift | 8 ++ 9 files changed, 183 insertions(+) create mode 100644 swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll create mode 100644 swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected create mode 100644 swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql create mode 100644 swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift diff --git a/swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll b/swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll new file mode 100644 index 00000000000..d1e0a0bc8b6 --- /dev/null +++ b/swift/ql/lib/codeql/swift/elements/expr/BitwiseOperation.qll @@ -0,0 +1,108 @@ +private import codeql.swift.elements.expr.BinaryExpr +private import codeql.swift.elements.expr.Expr +private import codeql.swift.elements.expr.PrefixUnaryExpr + +/** + * A bitwise operation, such as: + * ``` + * a & b + * ``` + */ +class BitwiseOperation extends Expr { + BitwiseOperation() { + this instanceof BinaryBitwiseOperation or + this instanceof UnaryBitwiseOperation + } + + /** + * Gets an operand of this bitwise operation. + */ + Expr getAnOperand() { + result = + [this.(BinaryBitwiseOperation).getAnOperand(), this.(UnaryBitwiseOperation).getOperand()] + } +} + +/** + * A binary bitwise operation, such as: + * ``` + * a & b + * ``` + */ +class BinaryBitwiseOperation extends BinaryExpr { + BinaryBitwiseOperation() { + this instanceof AndBitwiseExpr or + this instanceof OrBitwiseExpr or + this instanceof XorBitwiseExpr or + this instanceof ShiftLeftBitwiseExpr or + this instanceof ShiftRightBitwiseExpr + } +} + +/** + * A bitwise AND expression. + * ``` + * a & b + * ``` + */ +class AndBitwiseExpr extends BinaryExpr { + AndBitwiseExpr() { this.getStaticTarget().getName() = "&(_:_:)" } +} + +/** + * A bitwise OR expression. + * ``` + * a | b + * ``` + */ +class OrBitwiseExpr extends BinaryExpr { + OrBitwiseExpr() { this.getStaticTarget().getName() = "|(_:_:)" } +} + +/** + * A bitwise XOR expression. + * ``` + * a ^ b + * ``` + */ +class XorBitwiseExpr extends BinaryExpr { + XorBitwiseExpr() { this.getStaticTarget().getName() = "^(_:_:)" } +} + +/** + * A bitwise shift left expression. + * ``` + * a << b + * ``` + */ +class ShiftLeftBitwiseExpr extends BinaryExpr { + ShiftLeftBitwiseExpr() { this.getStaticTarget().getName() = "<<(_:_:)" } +} + +/** + * A bitwise shift right expression. + * ``` + * a >> b + * ``` + */ +class ShiftRightBitwiseExpr extends BinaryExpr { + ShiftRightBitwiseExpr() { this.getStaticTarget().getName() = ">>(_:_:)" } +} + +/** + * A unary bitwise operation, such as: + * ``` + * ~a + * ``` + */ +class UnaryBitwiseOperation extends PrefixUnaryExpr instanceof NotBitwiseExpr { } + +/** + * A bitwise NOT expression. + * ``` + * ~a + * ``` + */ +class NotBitwiseExpr extends PrefixUnaryExpr { + NotBitwiseExpr() { this.getStaticTarget().getName() = "~(_:)" } +} diff --git a/swift/ql/lib/swift.qll b/swift/ql/lib/swift.qll index 51d3ee15c83..f890aca45e5 100644 --- a/swift/ql/lib/swift.qll +++ b/swift/ql/lib/swift.qll @@ -2,6 +2,7 @@ import codeql.swift.elements import codeql.swift.elements.expr.ArithmeticOperation +import codeql.swift.elements.expr.BitwiseOperation import codeql.swift.elements.expr.LogicalOperation import codeql.swift.elements.decl.MethodDecl import codeql.swift.elements.decl.ClassOrStructDecl diff --git a/swift/ql/test/extractor-tests/expressions/all.expected b/swift/ql/test/extractor-tests/expressions/all.expected index d0068a17220..12c3ec02368 100644 --- a/swift/ql/test/extractor-tests/expressions/all.expected +++ b/swift/ql/test/extractor-tests/expressions/all.expected @@ -256,3 +256,17 @@ | expressions.swift:161:9:161:9 | .^(_:_:) | MethodRefExpr | | expressions.swift:161:9:161:9 | Int.Type | TypeExpr | | expressions.swift:161:11:161:11 | 2 | IntegerLiteralExpr | +| expressions.swift:162:3:162:3 | _ | DiscardAssignmentExpr | +| expressions.swift:162:3:162:12 | ... = ... | AssignExpr | +| expressions.swift:162:7:162:7 | 1 | IntegerLiteralExpr | +| expressions.swift:162:7:162:12 | ... .<<(_:_:) ... | BinaryExpr | +| expressions.swift:162:9:162:9 | .<<(_:_:) | MethodRefExpr | +| expressions.swift:162:9:162:9 | Int.Type | TypeExpr | +| expressions.swift:162:12:162:12 | 0 | IntegerLiteralExpr | +| expressions.swift:163:3:163:3 | _ | DiscardAssignmentExpr | +| expressions.swift:163:3:163:12 | ... = ... | AssignExpr | +| expressions.swift:163:7:163:7 | 1 | IntegerLiteralExpr | +| expressions.swift:163:7:163:12 | ... .>>(_:_:) ... | BinaryExpr | +| expressions.swift:163:9:163:9 | .>>(_:_:) | MethodRefExpr | +| expressions.swift:163:9:163:9 | Int.Type | TypeExpr | +| expressions.swift:163:12:163:12 | 0 | IntegerLiteralExpr | diff --git a/swift/ql/test/extractor-tests/expressions/expressions.swift b/swift/ql/test/extractor-tests/expressions/expressions.swift index 42eb9d9fcb3..3e16c0be0ac 100644 --- a/swift/ql/test/extractor-tests/expressions/expressions.swift +++ b/swift/ql/test/extractor-tests/expressions/expressions.swift @@ -159,4 +159,6 @@ func bitwise() { _ = 1 & 2 _ = 1 | 2 _ = 1 ^ 2 + _ = 1 << 0 + _ = 1 >> 0 } diff --git a/swift/ql/test/library-tests/ast/PrintAst.expected b/swift/ql/test/library-tests/ast/PrintAst.expected index 6b837081217..8dbfecabeee 100644 --- a/swift/ql/test/library-tests/ast/PrintAst.expected +++ b/swift/ql/test/library-tests/ast/PrintAst.expected @@ -4379,6 +4379,26 @@ expressions.swift: # 161| getExpr(): [IntegerLiteralExpr] 1 # 161| getArgument(1): [Argument] : 2 # 161| getExpr(): [IntegerLiteralExpr] 2 +# 162| getElement(4): [AssignExpr] ... = ... +# 162| getDest(): [DiscardAssignmentExpr] _ +# 162| getSource(): [BinaryExpr] ... .<<(_:_:) ... +# 162| getFunction(): [MethodRefExpr] .<<(_:_:) +# 162| getBase(): [TypeExpr] Int.Type +# 162| getTypeRepr(): [TypeRepr] Int +# 162| getArgument(0): [Argument] : 1 +# 162| getExpr(): [IntegerLiteralExpr] 1 +# 162| getArgument(1): [Argument] : 0 +# 162| getExpr(): [IntegerLiteralExpr] 0 +# 163| getElement(5): [AssignExpr] ... = ... +# 163| getDest(): [DiscardAssignmentExpr] _ +# 163| getSource(): [BinaryExpr] ... .>>(_:_:) ... +# 163| getFunction(): [MethodRefExpr] .>>(_:_:) +# 163| getBase(): [TypeExpr] Int.Type +# 163| getTypeRepr(): [TypeRepr] Int +# 163| getArgument(0): [Argument] : 1 +# 163| getExpr(): [IntegerLiteralExpr] 1 +# 163| getArgument(1): [Argument] : 0 +# 163| getExpr(): [IntegerLiteralExpr] 0 patterns.swift: # 1| [ConcreteFuncDecl] basic_patterns() # 1| InterfaceType = () -> () diff --git a/swift/ql/test/library-tests/ast/expressions.swift b/swift/ql/test/library-tests/ast/expressions.swift index 42eb9d9fcb3..3e16c0be0ac 100644 --- a/swift/ql/test/library-tests/ast/expressions.swift +++ b/swift/ql/test/library-tests/ast/expressions.swift @@ -159,4 +159,6 @@ func bitwise() { _ = 1 & 2 _ = 1 | 2 _ = 1 ^ 2 + _ = 1 << 0 + _ = 1 >> 0 } diff --git a/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected new file mode 100644 index 00000000000..8c8c5fe47e2 --- /dev/null +++ b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.expected @@ -0,0 +1,6 @@ +| bitwiseoperation.swift:2:7:2:8 | call to ~(_:) | NotBitwiseExpr, UnaryBitwiseOperation | +| bitwiseoperation.swift:3:7:3:11 | ... .&(_:_:) ... | AndBitwiseExpr, BinaryBitwiseOperation | +| bitwiseoperation.swift:4:7:4:11 | ... .\|(_:_:) ... | BinaryBitwiseOperation, OrBitwiseExpr | +| bitwiseoperation.swift:5:7:5:11 | ... .^(_:_:) ... | BinaryBitwiseOperation, XorBitwiseExpr | +| bitwiseoperation.swift:6:7:6:12 | ... .<<(_:_:) ... | BinaryBitwiseOperation, ShiftLeftBitwiseExpr | +| bitwiseoperation.swift:7:7:7:12 | ... .>>(_:_:) ... | BinaryBitwiseOperation, ShiftRightBitwiseExpr | diff --git a/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql new file mode 100644 index 00000000000..6e52b7d8e4a --- /dev/null +++ b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.ql @@ -0,0 +1,22 @@ +import swift + +string describe(BitwiseOperation e) { + e instanceof BinaryBitwiseOperation and result = "BinaryBitwiseOperation" + or + e instanceof AndBitwiseExpr and result = "AndBitwiseExpr" + or + e instanceof OrBitwiseExpr and result = "OrBitwiseExpr" + or + e instanceof XorBitwiseExpr and result = "XorBitwiseExpr" + or + e instanceof ShiftLeftBitwiseExpr and result = "ShiftLeftBitwiseExpr" + or + e instanceof ShiftRightBitwiseExpr and result = "ShiftRightBitwiseExpr" + or + e instanceof UnaryBitwiseOperation and result = "UnaryBitwiseOperation" + or + e instanceof NotBitwiseExpr and result = "NotBitwiseExpr" +} + +from BitwiseOperation e +select e, concat(describe(e), ", ") diff --git a/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift new file mode 100644 index 00000000000..2c6ae67eb84 --- /dev/null +++ b/swift/ql/test/library-tests/elements/expr/bitwiseopration/bitwiseoperation.swift @@ -0,0 +1,8 @@ +func bitwise() { + _ = ~1 + _ = 1 & 2 + _ = 1 | 2 + _ = 1 ^ 2 + _ = 1 << 0 + _ = 1 >> 0 +} From e17bc6c58108936d24562fdd93ddb6808ad9e1d7 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Wed, 2 Nov 2022 16:24:56 +0100 Subject: [PATCH 43/49] Swift: add `UnspecifiedElement` --- swift/ql/lib/codeql/swift/elements.qll | 1 + .../swift/elements/UnspecifiedElement.qll | 4 + .../UnspecifiedElementConstructor.qll | 4 + .../codeql/swift/generated/ParentChild.qll | 17 + swift/ql/lib/codeql/swift/generated/Raw.qll | 12 + swift/ql/lib/codeql/swift/generated/Synth.qll | 17 +- .../swift/generated/SynthConstructors.qll | 1 + .../swift/generated/UnspecifiedElement.qll | 60 ++ swift/ql/lib/swift.dbscheme | 523 ++++++++++++------ .../UnspecifiedElement/MISSING_SOURCE.txt | 4 + swift/schema.py | 7 + 11 files changed, 482 insertions(+), 168 deletions(-) create mode 100644 swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll create mode 100644 swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll create mode 100644 swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt diff --git a/swift/ql/lib/codeql/swift/elements.qll b/swift/ql/lib/codeql/swift/elements.qll index e7b1a6a299a..6e229c07741 100644 --- a/swift/ql/lib/codeql/swift/elements.qll +++ b/swift/ql/lib/codeql/swift/elements.qll @@ -11,6 +11,7 @@ import codeql.swift.elements.Location import codeql.swift.elements.UnknownFile import codeql.swift.elements.UnknownLocation import codeql.swift.elements.UnresolvedElement +import codeql.swift.elements.UnspecifiedElement import codeql.swift.elements.decl.AbstractFunctionDecl import codeql.swift.elements.decl.AbstractStorageDecl import codeql.swift.elements.decl.AbstractTypeParamDecl diff --git a/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll b/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll new file mode 100644 index 00000000000..194ef8fc175 --- /dev/null +++ b/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll @@ -0,0 +1,4 @@ +// generated by codegen/codegen.py, remove this comment if you wish to edit this file +private import codeql.swift.generated.UnspecifiedElement + +class UnspecifiedElement extends Generated::UnspecifiedElement { } diff --git a/swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll b/swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll new file mode 100644 index 00000000000..8c87ad1281f --- /dev/null +++ b/swift/ql/lib/codeql/swift/elements/UnspecifiedElementConstructor.qll @@ -0,0 +1,4 @@ +// generated by codegen/codegen.py, remove this comment if you wish to edit this file +private import codeql.swift.generated.Raw + +predicate constructUnspecifiedElement(Raw::UnspecifiedElement id) { any() } diff --git a/swift/ql/lib/codeql/swift/generated/ParentChild.qll b/swift/ql/lib/codeql/swift/generated/ParentChild.qll index eb76c02e23e..591c32a2fb5 100644 --- a/swift/ql/lib/codeql/swift/generated/ParentChild.qll +++ b/swift/ql/lib/codeql/swift/generated/ParentChild.qll @@ -165,6 +165,21 @@ private module Impl { ) } + private Element getImmediateChildOfUnspecifiedElement( + UnspecifiedElement e, int index, string partialPredicateCall + ) { + exists(int b, int bLocatable, int n | + b = 0 and + bLocatable = b + 1 + max(int i | i = -1 or exists(getImmediateChildOfLocatable(e, i, _)) | i) and + n = bLocatable and + ( + none() + or + result = getImmediateChildOfLocatable(e, index - b, partialPredicateCall) + ) + ) + } + private Element getImmediateChildOfDecl(Decl e, int index, string partialPredicateCall) { exists(int b, int bAstNode, int n | b = 0 and @@ -4871,6 +4886,8 @@ private module Impl { or result = getImmediateChildOfUnknownLocation(e, index, partialAccessor) or + result = getImmediateChildOfUnspecifiedElement(e, index, partialAccessor) + or result = getImmediateChildOfEnumCaseDecl(e, index, partialAccessor) or result = getImmediateChildOfExtensionDecl(e, index, partialAccessor) diff --git a/swift/ql/lib/codeql/swift/generated/Raw.qll b/swift/ql/lib/codeql/swift/generated/Raw.qll index 37233a66b01..d6340128034 100644 --- a/swift/ql/lib/codeql/swift/generated/Raw.qll +++ b/swift/ql/lib/codeql/swift/generated/Raw.qll @@ -51,6 +51,18 @@ module Raw { override string toString() { result = "DbLocation" } } + class UnspecifiedElement extends @unspecified_element, Locatable { + override string toString() { result = "UnspecifiedElement" } + + Element getParent() { unspecified_element_parents(this, result) } + + string getProperty() { unspecified_elements(this, result, _) } + + int getIndex() { unspecified_element_indices(this, result) } + + string getError() { unspecified_elements(this, _, result) } + } + class Decl extends @decl, AstNode { ModuleDecl getModule() { decls(this, result) } } diff --git a/swift/ql/lib/codeql/swift/generated/Synth.qll b/swift/ql/lib/codeql/swift/generated/Synth.qll index 1baf1fd0f3b..f9fca8aab66 100644 --- a/swift/ql/lib/codeql/swift/generated/Synth.qll +++ b/swift/ql/lib/codeql/swift/generated/Synth.qll @@ -10,6 +10,7 @@ module Synth { TDbLocation(Raw::DbLocation id) { constructDbLocation(id) } or TUnknownFile() or TUnknownLocation() or + TUnspecifiedElement(Raw::UnspecifiedElement id) { constructUnspecifiedElement(id) } or TAccessorDecl(Raw::AccessorDecl id) { constructAccessorDecl(id) } or TAssociatedTypeDecl(Raw::AssociatedTypeDecl id) { constructAssociatedTypeDecl(id) } or TClassDecl(Raw::ClassDecl id) { constructClassDecl(id) } or @@ -321,7 +322,7 @@ module Synth { class TFile = TDbFile or TUnknownFile; - class TLocatable = TArgument or TAstNode or TComment; + class TLocatable = TArgument or TAstNode or TComment or TUnspecifiedElement; class TLocation = TDbLocation or TUnknownLocation; @@ -499,6 +500,11 @@ module Synth { cached TUnknownLocation convertUnknownLocationFromRaw(Raw::Element e) { none() } + cached + TUnspecifiedElement convertUnspecifiedElementFromRaw(Raw::Element e) { + result = TUnspecifiedElement(e) + } + cached TAccessorDecl convertAccessorDeclFromRaw(Raw::Element e) { result = TAccessorDecl(e) } @@ -1464,6 +1470,8 @@ module Synth { result = convertAstNodeFromRaw(e) or result = convertCommentFromRaw(e) + or + result = convertUnspecifiedElementFromRaw(e) } cached @@ -2199,6 +2207,11 @@ module Synth { cached Raw::Element convertUnknownLocationToRaw(TUnknownLocation e) { none() } + cached + Raw::Element convertUnspecifiedElementToRaw(TUnspecifiedElement e) { + e = TUnspecifiedElement(result) + } + cached Raw::Element convertAccessorDeclToRaw(TAccessorDecl e) { e = TAccessorDecl(result) } @@ -3162,6 +3175,8 @@ module Synth { result = convertAstNodeToRaw(e) or result = convertCommentToRaw(e) + or + result = convertUnspecifiedElementToRaw(e) } cached diff --git a/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll b/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll index b8752923a8d..f35b5fdc630 100644 --- a/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll +++ b/swift/ql/lib/codeql/swift/generated/SynthConstructors.qll @@ -2,6 +2,7 @@ import codeql.swift.elements.CommentConstructor import codeql.swift.elements.DbFileConstructor import codeql.swift.elements.DbLocationConstructor +import codeql.swift.elements.UnspecifiedElementConstructor import codeql.swift.elements.decl.AccessorDeclConstructor import codeql.swift.elements.decl.AssociatedTypeDeclConstructor import codeql.swift.elements.decl.ClassDeclConstructor diff --git a/swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll b/swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll new file mode 100644 index 00000000000..8f6efd926d8 --- /dev/null +++ b/swift/ql/lib/codeql/swift/generated/UnspecifiedElement.qll @@ -0,0 +1,60 @@ +// generated by codegen/codegen.py +private import codeql.swift.generated.Synth +private import codeql.swift.generated.Raw +import codeql.swift.elements.Element +import codeql.swift.elements.Locatable + +module Generated { + class UnspecifiedElement extends Synth::TUnspecifiedElement, Locatable { + override string getAPrimaryQlClass() { result = "UnspecifiedElement" } + + /** + * Gets the parent of this unspecified element, if it exists. + * + * This includes nodes from the "hidden" AST. It can be overridden in subclasses to change the + * behavior of both the `Immediate` and non-`Immediate` versions. + */ + Element getImmediateParent() { + result = + Synth::convertElementFromRaw(Synth::convertUnspecifiedElementToRaw(this) + .(Raw::UnspecifiedElement) + .getParent()) + } + + /** + * Gets the parent of this unspecified element, if it exists. + */ + final Element getParent() { result = getImmediateParent().resolve() } + + /** + * Holds if `getParent()` exists. + */ + final predicate hasParent() { exists(getParent()) } + + /** + * Gets the property of this unspecified element. + */ + string getProperty() { + result = Synth::convertUnspecifiedElementToRaw(this).(Raw::UnspecifiedElement).getProperty() + } + + /** + * Gets the index of this unspecified element, if it exists. + */ + int getIndex() { + result = Synth::convertUnspecifiedElementToRaw(this).(Raw::UnspecifiedElement).getIndex() + } + + /** + * Holds if `getIndex()` exists. + */ + final predicate hasIndex() { exists(getIndex()) } + + /** + * Gets the error of this unspecified element. + */ + string getError() { + result = Synth::convertUnspecifiedElementToRaw(this).(Raw::UnspecifiedElement).getError() + } + } +} diff --git a/swift/ql/lib/swift.dbscheme b/swift/ql/lib/swift.dbscheme index 5483094d942..9b503cdd7fc 100644 --- a/swift/ql/lib/swift.dbscheme +++ b/swift/ql/lib/swift.dbscheme @@ -35,20 +35,20 @@ element_is_unknown( #keyset[id] callable_self_params( int id: @callable ref, - int self_param: @param_decl ref + int self_param: @param_decl_or_none ref ); #keyset[id, index] callable_params( int id: @callable ref, int index: int ref, - int param: @param_decl ref + int param: @param_decl_or_none ref ); #keyset[id] callable_bodies( int id: @callable ref, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); @file = @@ -66,12 +66,13 @@ files( @argument | @ast_node | @comment +| @unspecified_element ; #keyset[id] locatable_locations( int id: @locatable ref, - int location: @location ref + int location: @location_or_none ref ); @location = @@ -82,7 +83,7 @@ locatable_locations( #keyset[id] locations( int id: @location ref, - int file: @file ref, + int file: @file_or_none ref, int start_line: int ref, int start_column: int ref, int end_line: int ref, @@ -132,6 +133,24 @@ unknown_locations( unique int id: @unknown_location ); +unspecified_elements( + unique int id: @unspecified_element, + string property: string ref, + string error: string ref +); + +#keyset[id] +unspecified_element_parents( + int id: @unspecified_element ref, + int parent: @element ref +); + +#keyset[id] +unspecified_element_indices( + int id: @unspecified_element ref, + int index: int ref +); + @decl = @enum_case_decl | @extension_decl @@ -149,7 +168,7 @@ unknown_locations( #keyset[id] decls( //dir=decl int id: @decl ref, - int module: @module_decl ref + int module: @module_decl_or_none ref ); @generic_context = @@ -163,7 +182,7 @@ decls( //dir=decl generic_context_generic_type_params( //dir=decl int id: @generic_context ref, int index: int ref, - int generic_type_param: @generic_type_param_decl ref + int generic_type_param: @generic_type_param_decl_or_none ref ); @iterable_decl_context = @@ -175,7 +194,7 @@ generic_context_generic_type_params( //dir=decl iterable_decl_context_members( //dir=decl int id: @iterable_decl_context ref, int index: int ref, - int member: @decl ref + int member: @decl_or_none ref ); enum_case_decls( //dir=decl @@ -186,12 +205,12 @@ enum_case_decls( //dir=decl enum_case_decl_elements( //dir=decl int id: @enum_case_decl ref, int index: int ref, - int element: @enum_element_decl ref + int element: @enum_element_decl_or_none ref ); extension_decls( //dir=decl unique int id: @extension_decl, - int extended_type_decl: @nominal_type_decl ref + int extended_type_decl: @nominal_type_decl_or_none ref ); if_config_decls( //dir=decl @@ -202,7 +221,7 @@ if_config_decls( //dir=decl if_config_decl_active_elements( //dir=decl int id: @if_config_decl ref, int index: int ref, - int active_element: @ast_node ref + int active_element: @ast_node_or_none ref ); import_decls( //dir=decl @@ -217,14 +236,14 @@ import_decl_is_exported( //dir=decl #keyset[id] import_decl_imported_modules( //dir=decl int id: @import_decl ref, - int imported_module: @module_decl ref + int imported_module: @module_decl_or_none ref ); #keyset[id, index] import_decl_declarations( //dir=decl int id: @import_decl ref, int index: int ref, - int declaration: @value_decl ref + int declaration: @value_decl_or_none ref ); missing_member_decls( //dir=decl @@ -251,14 +270,14 @@ pattern_binding_decls( //dir=decl pattern_binding_decl_inits( //dir=decl int id: @pattern_binding_decl ref, int index: int ref, - int init: @expr ref + int init: @expr_or_none ref ); #keyset[id, index] pattern_binding_decl_patterns( //dir=decl int id: @pattern_binding_decl ref, int index: int ref, - int pattern: @pattern ref + int pattern: @pattern_or_none ref ); pound_diagnostic_decls( //dir=decl @@ -271,7 +290,7 @@ precedence_group_decls( //dir=decl top_level_code_decls( //dir=decl unique int id: @top_level_code_decl, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); @value_decl = @@ -284,7 +303,7 @@ top_level_code_decls( //dir=decl #keyset[id] value_decls( //dir=decl int id: @value_decl ref, - int interface_type: @type ref + int interface_type: @type_or_none ref ); @abstract_function_decl = @@ -308,7 +327,7 @@ abstract_function_decls( //dir=decl abstract_storage_decl_accessor_decls( //dir=decl int id: @abstract_storage_decl ref, int index: int ref, - int accessor_decl: @accessor_decl ref + int accessor_decl: @accessor_decl_or_none ref ); enum_element_decls( //dir=decl @@ -320,7 +339,7 @@ enum_element_decls( //dir=decl enum_element_decl_params( //dir=decl int id: @enum_element_decl ref, int index: int ref, - int param: @param_decl ref + int param: @param_decl_or_none ref ); infix_operator_decls( //dir=decl @@ -330,7 +349,7 @@ infix_operator_decls( //dir=decl #keyset[id] infix_operator_decl_precedence_groups( //dir=decl int id: @infix_operator_decl ref, - int precedence_group: @precedence_group_decl ref + int precedence_group: @precedence_group_decl_or_none ref ); postfix_operator_decls( //dir=decl @@ -357,7 +376,7 @@ type_decls( //dir=decl type_decl_base_types( //dir=decl int id: @type_decl ref, int index: int ref, - int base_type: @type ref + int base_type: @type_or_none ref ); @abstract_type_param_decl = @@ -402,26 +421,26 @@ module_decl_is_system_module( //dir=decl module_decl_imported_modules( //dir=decl int id: @module_decl ref, int index: int ref, - int imported_module: @module_decl ref + int imported_module: @module_decl_or_none ref ); #keyset[id, index] module_decl_exported_modules( //dir=decl int id: @module_decl ref, int index: int ref, - int exported_module: @module_decl ref + int exported_module: @module_decl_or_none ref ); subscript_decls( //dir=decl unique int id: @subscript_decl, - int element_type: @type ref + int element_type: @type_or_none ref ); #keyset[id, index] subscript_decl_params( //dir=decl int id: @subscript_decl ref, int index: int ref, - int param: @param_decl ref + int param: @param_decl_or_none ref ); @var_decl = @@ -433,25 +452,25 @@ subscript_decl_params( //dir=decl var_decls( //dir=decl int id: @var_decl ref, string name: string ref, - int type_: @type ref + int type_: @type_or_none ref ); #keyset[id] var_decl_attached_property_wrapper_types( //dir=decl int id: @var_decl ref, - int attached_property_wrapper_type: @type ref + int attached_property_wrapper_type: @type_or_none ref ); #keyset[id] var_decl_parent_patterns( //dir=decl int id: @var_decl ref, - int parent_pattern: @pattern ref + int parent_pattern: @pattern_or_none ref ); #keyset[id] var_decl_parent_initializers( //dir=decl int id: @var_decl ref, - int parent_initializer: @expr ref + int parent_initializer: @expr_or_none ref ); accessor_decls( //dir=decl @@ -505,7 +524,7 @@ generic_type_param_decls( //dir=decl #keyset[id] nominal_type_decls( //dir=decl int id: @nominal_type_decl ref, - int type_: @type ref + int type_: @type_or_none ref ); opaque_type_decls( //dir=decl @@ -544,7 +563,7 @@ struct_decls( //dir=decl arguments( //dir=expr unique int id: @argument, string label: string ref, - int expr: @expr ref + int expr: @expr_or_none ref ); @expr = @@ -606,7 +625,7 @@ arguments( //dir=expr #keyset[id] expr_types( //dir=expr int id: @expr ref, - int type_: @type ref + int type_: @type_or_none ref ); @abstract_closure_expr = @@ -623,7 +642,7 @@ expr_types( //dir=expr #keyset[id] any_try_exprs( //dir=expr int id: @any_try_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); applied_property_wrapper_exprs( //dir=expr @@ -641,14 +660,14 @@ applied_property_wrapper_exprs( //dir=expr #keyset[id] apply_exprs( //dir=expr int id: @apply_expr ref, - int function: @expr ref + int function: @expr_or_none ref ); #keyset[id, index] apply_expr_arguments( //dir=expr int id: @apply_expr ref, int index: int ref, - int argument: @argument ref + int argument: @argument_or_none ref ); arrow_exprs( //dir=expr @@ -657,25 +676,25 @@ arrow_exprs( //dir=expr assign_exprs( //dir=expr unique int id: @assign_expr, - int dest: @expr ref, - int source: @expr ref + int dest: @expr_or_none ref, + int source: @expr_or_none ref ); bind_optional_exprs( //dir=expr unique int id: @bind_optional_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); capture_list_exprs( //dir=expr unique int id: @capture_list_expr, - int closure_body: @closure_expr ref + int closure_body: @closure_expr_or_none ref ); #keyset[id, index] capture_list_expr_binding_decls( //dir=expr int id: @capture_list_expr ref, int index: int ref, - int binding_decl: @pattern_binding_decl ref + int binding_decl: @pattern_binding_decl_or_none ref ); code_completion_exprs( //dir=expr @@ -689,14 +708,14 @@ code_completion_exprs( //dir=expr decl_ref_exprs( //dir=expr unique int id: @decl_ref_expr, - int decl: @decl ref + int decl: @decl_or_none ref ); #keyset[id, index] decl_ref_expr_replacement_types( //dir=expr int id: @decl_ref_expr ref, int index: int ref, - int replacement_type: @type ref + int replacement_type: @type_or_none ref ); #keyset[id] @@ -716,14 +735,14 @@ decl_ref_expr_has_ordinary_semantics( //dir=expr default_argument_exprs( //dir=expr unique int id: @default_argument_expr, - int param_decl: @param_decl ref, + int param_decl: @param_decl_or_none ref, int param_index: int ref ); #keyset[id] default_argument_expr_caller_side_defaults( //dir=expr int id: @default_argument_expr ref, - int caller_side_default: @expr ref + int caller_side_default: @expr_or_none ref ); discard_assignment_exprs( //dir=expr @@ -732,13 +751,13 @@ discard_assignment_exprs( //dir=expr dot_syntax_base_ignored_exprs( //dir=expr unique int id: @dot_syntax_base_ignored_expr, - int qualifier: @expr ref, - int sub_expr: @expr ref + int qualifier: @expr_or_none ref, + int sub_expr: @expr_or_none ref ); dynamic_type_exprs( //dir=expr unique int id: @dynamic_type_expr, - int base: @expr ref + int base: @expr_or_none ref ); editor_placeholder_exprs( //dir=expr @@ -747,8 +766,8 @@ editor_placeholder_exprs( //dir=expr enum_is_case_exprs( //dir=expr unique int id: @enum_is_case_expr, - int sub_expr: @expr ref, - int element: @enum_element_decl ref + int sub_expr: @expr_or_none ref, + int element: @enum_element_decl_or_none ref ); error_exprs( //dir=expr @@ -763,12 +782,12 @@ error_exprs( //dir=expr #keyset[id] explicit_cast_exprs( //dir=expr int id: @explicit_cast_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); force_value_exprs( //dir=expr unique int id: @force_value_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); @identity_expr = @@ -781,14 +800,14 @@ force_value_exprs( //dir=expr #keyset[id] identity_exprs( //dir=expr int id: @identity_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); if_exprs( //dir=expr unique int id: @if_expr, - int condition: @expr ref, - int then_expr: @expr ref, - int else_expr: @expr ref + int condition: @expr_or_none ref, + int then_expr: @expr_or_none ref, + int else_expr: @expr_or_none ref ); @implicit_conversion_expr = @@ -829,18 +848,18 @@ if_exprs( //dir=expr #keyset[id] implicit_conversion_exprs( //dir=expr int id: @implicit_conversion_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); in_out_exprs( //dir=expr unique int id: @in_out_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); key_path_application_exprs( //dir=expr unique int id: @key_path_application_expr, - int base: @expr ref, - int key_path: @expr ref + int base: @expr_or_none ref, + int key_path: @expr_or_none ref ); key_path_dot_exprs( //dir=expr @@ -854,18 +873,18 @@ key_path_exprs( //dir=expr #keyset[id] key_path_expr_roots( //dir=expr int id: @key_path_expr ref, - int root: @type_repr ref + int root: @type_repr_or_none ref ); #keyset[id] key_path_expr_parsed_paths( //dir=expr int id: @key_path_expr ref, - int parsed_path: @expr ref + int parsed_path: @expr_or_none ref ); lazy_initializer_exprs( //dir=expr unique int id: @lazy_initializer_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); @literal_expr = @@ -886,31 +905,31 @@ lazy_initializer_exprs( //dir=expr #keyset[id] lookup_exprs( //dir=expr int id: @lookup_expr ref, - int base: @expr ref + int base: @expr_or_none ref ); #keyset[id] lookup_expr_members( //dir=expr int id: @lookup_expr ref, - int member: @decl ref + int member: @decl_or_none ref ); make_temporarily_escapable_exprs( //dir=expr unique int id: @make_temporarily_escapable_expr, - int escaping_closure: @opaque_value_expr ref, - int nonescaping_closure: @expr ref, - int sub_expr: @expr ref + int escaping_closure: @opaque_value_expr_or_none ref, + int nonescaping_closure: @expr_or_none ref, + int sub_expr: @expr_or_none ref ); obj_c_selector_exprs( //dir=expr unique int id: @obj_c_selector_expr, - int sub_expr: @expr ref, - int method: @abstract_function_decl ref + int sub_expr: @expr_or_none ref, + int method: @abstract_function_decl_or_none ref ); one_way_exprs( //dir=expr unique int id: @one_way_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); opaque_value_exprs( //dir=expr @@ -919,19 +938,19 @@ opaque_value_exprs( //dir=expr open_existential_exprs( //dir=expr unique int id: @open_existential_expr, - int sub_expr: @expr ref, - int existential: @expr ref, - int opaque_expr: @opaque_value_expr ref + int sub_expr: @expr_or_none ref, + int existential: @expr_or_none ref, + int opaque_expr: @opaque_value_expr_or_none ref ); optional_evaluation_exprs( //dir=expr unique int id: @optional_evaluation_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); other_constructor_decl_ref_exprs( //dir=expr unique int id: @other_constructor_decl_ref_expr, - int constructor_decl: @constructor_decl ref + int constructor_decl: @constructor_decl_or_none ref ); @overload_set_ref_expr = @@ -948,8 +967,8 @@ property_wrapper_value_placeholder_exprs( //dir=expr rebind_self_in_constructor_exprs( //dir=expr unique int id: @rebind_self_in_constructor_expr, - int sub_expr: @expr ref, - int self: @var_decl ref + int sub_expr: @expr_or_none ref, + int self: @var_decl_or_none ref ); sequence_exprs( //dir=expr @@ -960,29 +979,29 @@ sequence_exprs( //dir=expr sequence_expr_elements( //dir=expr int id: @sequence_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); super_ref_exprs( //dir=expr unique int id: @super_ref_expr, - int self: @var_decl ref + int self: @var_decl_or_none ref ); tap_exprs( //dir=expr unique int id: @tap_expr, - int body: @brace_stmt ref, - int var: @var_decl ref + int body: @brace_stmt_or_none ref, + int var: @var_decl_or_none ref ); #keyset[id] tap_expr_sub_exprs( //dir=expr int id: @tap_expr ref, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); tuple_element_exprs( //dir=expr unique int id: @tuple_element_expr, - int sub_expr: @expr ref, + int sub_expr: @expr_or_none ref, int index: int ref ); @@ -994,7 +1013,7 @@ tuple_exprs( //dir=expr tuple_expr_elements( //dir=expr int id: @tuple_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); type_exprs( //dir=expr @@ -1004,7 +1023,7 @@ type_exprs( //dir=expr #keyset[id] type_expr_type_reprs( //dir=expr int id: @type_expr ref, - int type_repr: @type_repr ref + int type_repr: @type_repr_or_none ref ); unresolved_decl_ref_exprs( //dir=expr @@ -1019,7 +1038,7 @@ unresolved_decl_ref_expr_names( //dir=expr unresolved_dot_exprs( //dir=expr unique int id: @unresolved_dot_expr, - int base: @expr ref, + int base: @expr_or_none ref, string name: string ref ); @@ -1030,7 +1049,7 @@ unresolved_member_exprs( //dir=expr unresolved_pattern_exprs( //dir=expr unique int id: @unresolved_pattern_expr, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); unresolved_specialize_exprs( //dir=expr @@ -1039,7 +1058,7 @@ unresolved_specialize_exprs( //dir=expr vararg_expansion_exprs( //dir=expr unique int id: @vararg_expansion_expr, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); any_hashable_erasure_exprs( //dir=expr @@ -1058,7 +1077,7 @@ array_exprs( //dir=expr array_expr_elements( //dir=expr int id: @array_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); array_to_pointer_exprs( //dir=expr @@ -1146,7 +1165,7 @@ dictionary_exprs( //dir=expr dictionary_expr_elements( //dir=expr int id: @dictionary_expr ref, int index: int ref, - int element: @expr ref + int element: @expr_or_none ref ); differentiable_function_exprs( //dir=expr @@ -1201,25 +1220,25 @@ interpolated_string_literal_exprs( //dir=expr #keyset[id] interpolated_string_literal_expr_interpolation_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int interpolation_expr: @opaque_value_expr ref + int interpolation_expr: @opaque_value_expr_or_none ref ); #keyset[id] interpolated_string_literal_expr_interpolation_count_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int interpolation_count_expr: @expr ref + int interpolation_count_expr: @expr_or_none ref ); #keyset[id] interpolated_string_literal_expr_literal_capacity_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int literal_capacity_expr: @expr ref + int literal_capacity_expr: @expr_or_none ref ); #keyset[id] interpolated_string_literal_expr_appending_exprs( //dir=expr int id: @interpolated_string_literal_expr ref, - int appending_expr: @tap_expr ref + int appending_expr: @tap_expr_or_none ref ); linear_function_exprs( //dir=expr @@ -1317,7 +1336,7 @@ reify_pack_exprs( //dir=expr #keyset[id] self_apply_exprs( //dir=expr int id: @self_apply_expr ref, - int base: @expr ref + int base: @expr_or_none ref ); string_to_pointer_exprs( //dir=expr @@ -1332,7 +1351,7 @@ subscript_exprs( //dir=expr subscript_expr_arguments( //dir=expr int id: @subscript_expr ref, int index: int ref, - int argument: @argument ref + int argument: @argument_or_none ref ); #keyset[id] @@ -1448,7 +1467,7 @@ any_patterns( //dir=pattern binding_patterns( //dir=pattern unique int id: @binding_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); bool_patterns( //dir=pattern @@ -1458,18 +1477,18 @@ bool_patterns( //dir=pattern enum_element_patterns( //dir=pattern unique int id: @enum_element_pattern, - int element: @enum_element_decl ref + int element: @enum_element_decl_or_none ref ); #keyset[id] enum_element_pattern_sub_patterns( //dir=pattern int id: @enum_element_pattern ref, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); expr_patterns( //dir=pattern unique int id: @expr_pattern, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); is_patterns( //dir=pattern @@ -1479,13 +1498,13 @@ is_patterns( //dir=pattern #keyset[id] is_pattern_cast_type_reprs( //dir=pattern int id: @is_pattern ref, - int cast_type_repr: @type_repr ref + int cast_type_repr: @type_repr_or_none ref ); #keyset[id] is_pattern_sub_patterns( //dir=pattern int id: @is_pattern ref, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); named_patterns( //dir=pattern @@ -1495,12 +1514,12 @@ named_patterns( //dir=pattern optional_some_patterns( //dir=pattern unique int id: @optional_some_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); paren_patterns( //dir=pattern unique int id: @paren_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); tuple_patterns( //dir=pattern @@ -1511,29 +1530,29 @@ tuple_patterns( //dir=pattern tuple_pattern_elements( //dir=pattern int id: @tuple_pattern ref, int index: int ref, - int element: @pattern ref + int element: @pattern_or_none ref ); typed_patterns( //dir=pattern unique int id: @typed_pattern, - int sub_pattern: @pattern ref + int sub_pattern: @pattern_or_none ref ); #keyset[id] typed_pattern_type_reprs( //dir=pattern int id: @typed_pattern ref, - int type_repr: @type_repr ref + int type_repr: @type_repr_or_none ref ); case_label_items( //dir=stmt unique int id: @case_label_item, - int pattern: @pattern ref + int pattern: @pattern_or_none ref ); #keyset[id] case_label_item_guards( //dir=stmt int id: @case_label_item ref, - int guard: @expr ref + int guard: @expr_or_none ref ); condition_elements( //dir=stmt @@ -1543,19 +1562,19 @@ condition_elements( //dir=stmt #keyset[id] condition_element_booleans( //dir=stmt int id: @condition_element ref, - int boolean_: @expr ref + int boolean_: @expr_or_none ref ); #keyset[id] condition_element_patterns( //dir=stmt int id: @condition_element ref, - int pattern: @pattern ref + int pattern: @pattern_or_none ref ); #keyset[id] condition_element_initializers( //dir=stmt int id: @condition_element ref, - int initializer: @expr ref + int initializer: @expr_or_none ref ); @stmt = @@ -1581,7 +1600,7 @@ stmt_conditions( //dir=stmt stmt_condition_elements( //dir=stmt int id: @stmt_condition ref, int index: int ref, - int element: @condition_element ref + int element: @condition_element_or_none ref ); brace_stmts( //dir=stmt @@ -1592,7 +1611,7 @@ brace_stmts( //dir=stmt brace_stmt_elements( //dir=stmt int id: @brace_stmt ref, int index: int ref, - int element: @ast_node ref + int element: @ast_node_or_none ref ); break_stmts( //dir=stmt @@ -1608,26 +1627,26 @@ break_stmt_target_names( //dir=stmt #keyset[id] break_stmt_targets( //dir=stmt int id: @break_stmt ref, - int target: @stmt ref + int target: @stmt_or_none ref ); case_stmts( //dir=stmt unique int id: @case_stmt, - int body: @stmt ref + int body: @stmt_or_none ref ); #keyset[id, index] case_stmt_labels( //dir=stmt int id: @case_stmt ref, int index: int ref, - int label: @case_label_item ref + int label: @case_label_item_or_none ref ); #keyset[id, index] case_stmt_variables( //dir=stmt int id: @case_stmt ref, int index: int ref, - int variable: @var_decl ref + int variable: @var_decl_or_none ref ); continue_stmts( //dir=stmt @@ -1643,12 +1662,12 @@ continue_stmt_target_names( //dir=stmt #keyset[id] continue_stmt_targets( //dir=stmt int id: @continue_stmt ref, - int target: @stmt ref + int target: @stmt_or_none ref ); defer_stmts( //dir=stmt unique int id: @defer_stmt, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); fail_stmts( //dir=stmt @@ -1657,8 +1676,8 @@ fail_stmts( //dir=stmt fallthrough_stmts( //dir=stmt unique int id: @fallthrough_stmt, - int fallthrough_source: @case_stmt ref, - int fallthrough_dest: @case_stmt ref + int fallthrough_source: @case_stmt_or_none ref, + int fallthrough_dest: @case_stmt_or_none ref ); @labeled_stmt = @@ -1687,12 +1706,12 @@ return_stmts( //dir=stmt #keyset[id] return_stmt_results( //dir=stmt int id: @return_stmt ref, - int result: @expr ref + int result: @expr_or_none ref ); throw_stmts( //dir=stmt unique int id: @throw_stmt, - int sub_expr: @expr ref + int sub_expr: @expr_or_none ref ); yield_stmts( //dir=stmt @@ -1703,37 +1722,37 @@ yield_stmts( //dir=stmt yield_stmt_results( //dir=stmt int id: @yield_stmt ref, int index: int ref, - int result: @expr ref + int result: @expr_or_none ref ); do_catch_stmts( //dir=stmt unique int id: @do_catch_stmt, - int body: @stmt ref + int body: @stmt_or_none ref ); #keyset[id, index] do_catch_stmt_catches( //dir=stmt int id: @do_catch_stmt ref, int index: int ref, - int catch: @case_stmt ref + int catch: @case_stmt_or_none ref ); do_stmts( //dir=stmt unique int id: @do_stmt, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); for_each_stmts( //dir=stmt unique int id: @for_each_stmt, - int pattern: @pattern ref, - int sequence: @expr ref, - int body: @brace_stmt ref + int pattern: @pattern_or_none ref, + int sequence: @expr_or_none ref, + int body: @brace_stmt_or_none ref ); #keyset[id] for_each_stmt_wheres( //dir=stmt int id: @for_each_stmt ref, - int where: @expr ref + int where: @expr_or_none ref ); @labeled_conditional_stmt = @@ -1745,46 +1764,46 @@ for_each_stmt_wheres( //dir=stmt #keyset[id] labeled_conditional_stmts( //dir=stmt int id: @labeled_conditional_stmt ref, - int condition: @stmt_condition ref + int condition: @stmt_condition_or_none ref ); repeat_while_stmts( //dir=stmt unique int id: @repeat_while_stmt, - int condition: @expr ref, - int body: @stmt ref + int condition: @expr_or_none ref, + int body: @stmt_or_none ref ); switch_stmts( //dir=stmt unique int id: @switch_stmt, - int expr: @expr ref + int expr: @expr_or_none ref ); #keyset[id, index] switch_stmt_cases( //dir=stmt int id: @switch_stmt ref, int index: int ref, - int case_: @case_stmt ref + int case_: @case_stmt_or_none ref ); guard_stmts( //dir=stmt unique int id: @guard_stmt, - int body: @brace_stmt ref + int body: @brace_stmt_or_none ref ); if_stmts( //dir=stmt unique int id: @if_stmt, - int then: @stmt ref + int then: @stmt_or_none ref ); #keyset[id] if_stmt_elses( //dir=stmt int id: @if_stmt ref, - int else: @stmt ref + int else: @stmt_or_none ref ); while_stmts( //dir=stmt unique int id: @while_stmt, - int body: @stmt ref + int body: @stmt_or_none ref ); @type = @@ -1820,12 +1839,12 @@ while_stmts( //dir=stmt types( //dir=type int id: @type ref, string name: string ref, - int canonical_type: @type ref + int canonical_type: @type_or_none ref ); type_reprs( //dir=type unique int id: @type_repr, - int type_: @type ref + int type_: @type_or_none ref ); @any_function_type = @@ -1836,14 +1855,14 @@ type_reprs( //dir=type #keyset[id] any_function_types( //dir=type int id: @any_function_type ref, - int result: @type ref + int result: @type_or_none ref ); #keyset[id, index] any_function_type_param_types( //dir=type int id: @any_function_type ref, int index: int ref, - int param_type: @type ref + int param_type: @type_or_none ref ); #keyset[id, index] @@ -1871,13 +1890,13 @@ any_function_type_is_async( //dir=type #keyset[id] any_generic_types( //dir=type int id: @any_generic_type ref, - int declaration: @decl ref + int declaration: @decl_or_none ref ); #keyset[id] any_generic_type_parents( //dir=type int id: @any_generic_type ref, - int parent: @type ref + int parent: @type_or_none ref ); @any_metatype_type = @@ -1901,13 +1920,13 @@ any_generic_type_parents( //dir=type dependent_member_types( //dir=type unique int id: @dependent_member_type, - int base_type: @type ref, - int associated_type_decl: @associated_type_decl ref + int base_type: @type_or_none ref, + int associated_type_decl: @associated_type_decl_or_none ref ); dynamic_self_types( //dir=type unique int id: @dynamic_self_type, - int static_self_type: @type ref + int static_self_type: @type_or_none ref ); error_types( //dir=type @@ -1916,22 +1935,22 @@ error_types( //dir=type existential_types( //dir=type unique int id: @existential_type, - int constraint: @type ref + int constraint: @type_or_none ref ); in_out_types( //dir=type unique int id: @in_out_type, - int object_type: @type ref + int object_type: @type_or_none ref ); l_value_types( //dir=type unique int id: @l_value_type, - int object_type: @type ref + int object_type: @type_or_none ref ); module_types( //dir=type unique int id: @module_type, - int module: @module_decl ref + int module: @module_decl_or_none ref ); pack_expansion_types( //dir=type @@ -1958,7 +1977,7 @@ protocol_composition_types( //dir=type protocol_composition_type_members( //dir=type int id: @protocol_composition_type ref, int index: int ref, - int member: @type ref + int member: @type_or_none ref ); @reference_storage_type = @@ -1970,7 +1989,7 @@ protocol_composition_type_members( //dir=type #keyset[id] reference_storage_types( //dir=type int id: @reference_storage_type ref, - int referent_type: @type ref + int referent_type: @type_or_none ref ); sil_block_storage_types( //dir=type @@ -2008,7 +2027,7 @@ tuple_types( //dir=type tuple_type_types( //dir=type int id: @tuple_type ref, int index: int ref, - int type_: @type ref + int type_: @type_or_none ref ); #keyset[id, index] @@ -2041,20 +2060,20 @@ unresolved_types( //dir=type #keyset[id] archetype_types( //dir=type int id: @archetype_type ref, - int interface_type: @type ref + int interface_type: @type_or_none ref ); #keyset[id] archetype_type_superclasses( //dir=type int id: @archetype_type ref, - int superclass: @type ref + int superclass: @type_or_none ref ); #keyset[id, index] archetype_type_protocols( //dir=type int id: @archetype_type ref, int index: int ref, - int protocol: @protocol_decl ref + int protocol: @protocol_decl_or_none ref ); builtin_bridge_object_types( //dir=type @@ -2113,7 +2132,7 @@ generic_function_types( //dir=type generic_function_type_generic_params( //dir=type int id: @generic_function_type ref, int index: int ref, - int generic_param: @generic_type_param_type ref + int generic_param: @generic_type_param_type_or_none ref ); generic_type_param_types( //dir=type @@ -2131,7 +2150,7 @@ metatype_types( //dir=type paren_types( //dir=type unique int id: @paren_type, - int type_: @type ref + int type_: @type_or_none ref ); @syntax_sugar_type = @@ -2141,7 +2160,7 @@ paren_types( //dir=type type_alias_types( //dir=type unique int id: @type_alias_type, - int decl: @type_alias_decl ref + int decl: @type_alias_decl_or_none ref ); unbound_generic_types( //dir=type @@ -2170,7 +2189,7 @@ weak_storage_types( //dir=type bound_generic_type_arg_types( //dir=type int id: @bound_generic_type ref, int index: int ref, - int arg_type: @type ref + int arg_type: @type_or_none ref ); builtin_integer_literal_types( //dir=type @@ -2189,8 +2208,8 @@ builtin_integer_type_widths( //dir=type dictionary_types( //dir=type unique int id: @dictionary_type, - int key_type: @type ref, - int value_type: @type ref + int key_type: @type_or_none ref, + int value_type: @type_or_none ref ); @nominal_type = @@ -2225,7 +2244,7 @@ sequence_archetype_types( //dir=type #keyset[id] unary_syntax_sugar_types( //dir=type int id: @unary_syntax_sugar_type ref, - int base_type: @type ref + int base_type: @type_or_none ref ); array_slice_types( //dir=type @@ -2267,3 +2286,173 @@ struct_types( //dir=type variadic_sequence_types( //dir=type unique int id: @variadic_sequence_type ); + +@abstract_function_decl_or_none = + @abstract_function_decl +| @unspecified_element +; + +@accessor_decl_or_none = + @accessor_decl +| @unspecified_element +; + +@argument_or_none = + @argument +| @unspecified_element +; + +@associated_type_decl_or_none = + @associated_type_decl +| @unspecified_element +; + +@ast_node_or_none = + @ast_node +| @unspecified_element +; + +@brace_stmt_or_none = + @brace_stmt +| @unspecified_element +; + +@case_label_item_or_none = + @case_label_item +| @unspecified_element +; + +@case_stmt_or_none = + @case_stmt +| @unspecified_element +; + +@closure_expr_or_none = + @closure_expr +| @unspecified_element +; + +@condition_element_or_none = + @condition_element +| @unspecified_element +; + +@constructor_decl_or_none = + @constructor_decl +| @unspecified_element +; + +@decl_or_none = + @decl +| @unspecified_element +; + +@enum_element_decl_or_none = + @enum_element_decl +| @unspecified_element +; + +@expr_or_none = + @expr +| @unspecified_element +; + +@file_or_none = + @file +| @unspecified_element +; + +@generic_type_param_decl_or_none = + @generic_type_param_decl +| @unspecified_element +; + +@generic_type_param_type_or_none = + @generic_type_param_type +| @unspecified_element +; + +@location_or_none = + @location +| @unspecified_element +; + +@module_decl_or_none = + @module_decl +| @unspecified_element +; + +@nominal_type_decl_or_none = + @nominal_type_decl +| @unspecified_element +; + +@opaque_value_expr_or_none = + @opaque_value_expr +| @unspecified_element +; + +@param_decl_or_none = + @param_decl +| @unspecified_element +; + +@pattern_or_none = + @pattern +| @unspecified_element +; + +@pattern_binding_decl_or_none = + @pattern_binding_decl +| @unspecified_element +; + +@precedence_group_decl_or_none = + @precedence_group_decl +| @unspecified_element +; + +@protocol_decl_or_none = + @protocol_decl +| @unspecified_element +; + +@stmt_or_none = + @stmt +| @unspecified_element +; + +@stmt_condition_or_none = + @stmt_condition +| @unspecified_element +; + +@tap_expr_or_none = + @tap_expr +| @unspecified_element +; + +@type_or_none = + @type +| @unspecified_element +; + +@type_alias_decl_or_none = + @type_alias_decl +| @unspecified_element +; + +@type_repr_or_none = + @type_repr +| @unspecified_element +; + +@value_decl_or_none = + @unspecified_element +| @value_decl +; + +@var_decl_or_none = + @unspecified_element +| @var_decl +; diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt new file mode 100644 index 00000000000..0d319d9a669 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt @@ -0,0 +1,4 @@ +// generated by codegen/codegen.py + +After a swift source file is added in this directory and codegen/codegen.py is run again, test queries +will appear and this file will be deleted diff --git a/swift/schema.py b/swift/schema.py index 737dd82adeb..11be598a8c6 100644 --- a/swift/schema.py +++ b/swift/schema.py @@ -37,6 +37,13 @@ class Location(Element): class Locatable(Element): location: optional[Location] | cpp.skip | doc("location associated with this element in the code") +@use_for_null +class UnspecifiedElement(Locatable): + parent: optional[Element] + property: string + index: optional[int] + error: string + class Comment(Locatable): text: string From d6fb6bf0367813ba1f6526d8fb7e258606b9d498 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 10:38:24 +0100 Subject: [PATCH 44/49] Swift: customize `UnspecifiedElement` --- .../swift/elements/UnspecifiedElement.qll | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll b/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll index 194ef8fc175..7680fa7cca9 100644 --- a/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll +++ b/swift/ql/lib/codeql/swift/elements/UnspecifiedElement.qll @@ -1,4 +1,22 @@ -// generated by codegen/codegen.py, remove this comment if you wish to edit this file private import codeql.swift.generated.UnspecifiedElement +import codeql.swift.elements.Location -class UnspecifiedElement extends Generated::UnspecifiedElement { } +class UnspecifiedElement extends Generated::UnspecifiedElement { + override string toString() { + exists(string source, string index | + ( + source = " from " + this.getParent().getPrimaryQlClasses() + or + not this.hasParent() and source = "" + ) and + ( + index = "[" + this.getIndex() + "]" + or + not this.hasIndex() and index = "" + ) and + result = "missing " + this.getProperty() + index + source + ) + } + + override Location getImmediateLocation() { result = this.getParent().(Locatable).getLocation() } +} From 450a4a04af8841c573b812951b42e2e3a13e078e Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 11:33:45 +0100 Subject: [PATCH 45/49] Swift: add incomplete ast test The test was inspired by locally running the query against files in https://github.com/apple/swift/tree/main/test/Parse A query for missing elements was also added to the AST tests, expecting nothing to be found. --- swift/ql/test/TestUtils.qll | 2 ++ .../UnspecifiedElement/MISSING_SOURCE.txt | 4 --- .../UnspecifiedElement.expected | 9 ++++++ .../UnspecifiedElement/UnspecifiedElement.ql | 11 +++++++ .../UnspecifiedElement_getIndex.expected | 0 .../UnspecifiedElement_getIndex.ql | 7 +++++ .../UnspecifiedElement_getParent.expected | 9 ++++++ .../UnspecifiedElement_getParent.ql | 7 +++++ .../generated/UnspecifiedElement/wrong.swift | 30 +++++++++++++++++++ .../test/library-tests/ast/Missing.expected | 0 swift/ql/test/library-tests/ast/Missing.qlref | 1 + 11 files changed, 76 insertions(+), 4 deletions(-) delete mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.expected create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql create mode 100644 swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift create mode 100644 swift/ql/test/library-tests/ast/Missing.expected create mode 100644 swift/ql/test/library-tests/ast/Missing.qlref diff --git a/swift/ql/test/TestUtils.qll b/swift/ql/test/TestUtils.qll index 265513b4f4a..da21ff87778 100644 --- a/swift/ql/test/TestUtils.qll +++ b/swift/ql/test/TestUtils.qll @@ -29,4 +29,6 @@ predicate toBeTested(Element e) { ) ) ) + or + toBeTested(e.(UnspecifiedElement).getParent()) } diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt deleted file mode 100644 index 0d319d9a669..00000000000 --- a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/MISSING_SOURCE.txt +++ /dev/null @@ -1,4 +0,0 @@ -// generated by codegen/codegen.py - -After a swift source file is added in this directory and codegen/codegen.py is run again, test queries -will appear and this file will be deleted diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected new file mode 100644 index 00000000000..25fac0d1236 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.expected @@ -0,0 +1,9 @@ +| wrong.swift:3:1:3:23 | missing extended_type_decl from ExtensionDecl | getProperty: | extended_type_decl | getError: | element was unspecified by the extractor | +| wrong.swift:9:9:9:9 | missing fallthrough_dest from FallthroughStmt | getProperty: | fallthrough_dest | getError: | element was unspecified by the extractor | +| wrong.swift:9:9:9:9 | missing fallthrough_source from FallthroughStmt | getProperty: | fallthrough_source | getError: | element was unspecified by the extractor | +| wrong.swift:12:18:12:21 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | +| wrong.swift:14:18:14:26 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | +| wrong.swift:19:18:19:19 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | +| wrong.swift:22:13:22:13 | missing fallthrough_dest from FallthroughStmt | getProperty: | fallthrough_dest | getError: | element was unspecified by the extractor | +| wrong.swift:22:13:22:13 | missing fallthrough_source from FallthroughStmt | getProperty: | fallthrough_source | getError: | element was unspecified by the extractor | +| wrong.swift:26:18:26:19 | missing element from EnumElementPattern | getProperty: | element | getError: | element was unspecified by the extractor | diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql new file mode 100644 index 00000000000..dc762fbbeb0 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql @@ -0,0 +1,11 @@ +// generated by codegen/codegen.py +import codeql.swift.elements +import TestUtils + +from UnspecifiedElement x, string getProperty, string getError +where + toBeTested(x) and + not x.isUnknown() and + getProperty = x.getProperty() and + getError = x.getError() +select x, "getProperty:", getProperty, "getError:", getError diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.expected b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql new file mode 100644 index 00000000000..d8d2a7fab69 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getIndex.ql @@ -0,0 +1,7 @@ +// generated by codegen/codegen.py +import codeql.swift.elements +import TestUtils + +from UnspecifiedElement x +where toBeTested(x) and not x.isUnknown() +select x, x.getIndex() diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected new file mode 100644 index 00000000000..870a99df44f --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.expected @@ -0,0 +1,9 @@ +| wrong.swift:3:1:3:23 | missing extended_type_decl from ExtensionDecl | extension | +| wrong.swift:9:9:9:9 | missing fallthrough_dest from FallthroughStmt | fallthrough | +| wrong.swift:9:9:9:9 | missing fallthrough_source from FallthroughStmt | fallthrough | +| wrong.swift:12:18:12:21 | missing element from EnumElementPattern | (no string representation) | +| wrong.swift:14:18:14:26 | missing element from EnumElementPattern | (no string representation) | +| wrong.swift:19:18:19:19 | missing element from EnumElementPattern | (no string representation) | +| wrong.swift:22:13:22:13 | missing fallthrough_dest from FallthroughStmt | fallthrough | +| wrong.swift:22:13:22:13 | missing fallthrough_source from FallthroughStmt | fallthrough | +| wrong.swift:26:18:26:19 | missing element from EnumElementPattern | (no string representation) | diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql new file mode 100644 index 00000000000..7cfdb4bb609 --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/UnspecifiedElement_getParent.ql @@ -0,0 +1,7 @@ +// generated by codegen/codegen.py +import codeql.swift.elements +import TestUtils + +from UnspecifiedElement x +where toBeTested(x) and not x.isUnknown() +select x, x.getParent() diff --git a/swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift new file mode 100644 index 00000000000..30c61f6b33b --- /dev/null +++ b/swift/ql/test/extractor-tests/generated/UnspecifiedElement/wrong.swift @@ -0,0 +1,30 @@ +//codeql-extractor-expected-status: 1 + +extension Undefined { } + +enum Enum { + case A, B + + func test(e: Enum) { + fallthrough + + switch e { + case .A(): + break + case .B(let x): + let _ = x + break + case Int: + break + case .C: + break + default: + fallthrough + } + + switch undefined { + case .Whatever: + break + } + } +} diff --git a/swift/ql/test/library-tests/ast/Missing.expected b/swift/ql/test/library-tests/ast/Missing.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/library-tests/ast/Missing.qlref b/swift/ql/test/library-tests/ast/Missing.qlref new file mode 100644 index 00000000000..e9db12f988f --- /dev/null +++ b/swift/ql/test/library-tests/ast/Missing.qlref @@ -0,0 +1 @@ +extractor-tests/generated/UnspecifiedElement/UnspecifiedElement.ql From 8d3e6ff8a7b83d995d29f49d169ad6bb7b188e02 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 11:27:31 +0100 Subject: [PATCH 46/49] Swift: add label iteration --- .../codegen/templates/cpp_classes_h.mustache | 37 +++++++++++++++++++ swift/codegen/templates/trap_traps_h.mustache | 16 +++++++- swift/extractor/infra/SwiftDispatcher.h | 19 +++++++--- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/swift/codegen/templates/cpp_classes_h.mustache b/swift/codegen/templates/cpp_classes_h.mustache index 5176a8209de..a4a22170b2f 100644 --- a/swift/codegen/templates/cpp_classes_h.mustache +++ b/swift/codegen/templates/cpp_classes_h.mustache @@ -17,6 +17,8 @@ namespace codeql { {{#classes}} struct {{name}}{{#has_bases}} : {{#bases}}{{^first}}, {{/first}}{{ref.name}}{{/bases}}{{/has_bases}} { + static constexpr const char* NAME = "{{name}}"; + {{#final}} explicit {{name}}(TrapLabel<{{name}}Tag> id) : id{id} {} @@ -33,6 +35,41 @@ struct {{name}}{{#has_bases}} : {{#bases}}{{^first}}, {{/first}}{{ref.name}}{{/b } {{/final}} + {{^final}} + protected: + {{/final}} + template + void forEachLabel(F f) { + {{#final}} + f("id", -1, id); + {{/final}} + {{#bases}} + {{ref.name}}::forEachLabel(f); + {{/bases}} + {{#fields}} + {{#is_label}} + {{#is_repeated}} + for (auto i = 0u; i < {{field_name}}.size(); ++i) { + {{#is_optional}} + if ({{field_name}}[i]) f("{{field_name}}", i, *{{field_name}}[i]); + {{/is_optional}} + {{^is_optional}} + f("{{field_name}}", i, {{field_name}}[i]); + {{/is_optional}} + } + {{/is_repeated}} + {{^is_repeated}} + {{#is_optional}} + if ({{field_name}}) f("{{field_name}}", -1, *{{field_name}}); + {{/is_optional}} + {{^is_optional}} + f("{{field_name}}", -1, {{field_name}}); + {{/is_optional}} + {{/is_repeated}} + {{/is_label}} + {{/fields}} + } + protected: void emit({{^final}}TrapLabel<{{name}}Tag> id, {{/final}}std::ostream& out) const; }; diff --git a/swift/codegen/templates/trap_traps_h.mustache b/swift/codegen/templates/trap_traps_h.mustache index 297acb56d87..d3bf7769bd7 100644 --- a/swift/codegen/templates/trap_traps_h.mustache +++ b/swift/codegen/templates/trap_traps_h.mustache @@ -14,12 +14,24 @@ namespace codeql { // {{table_name}} struct {{name}}Trap { -{{#fields}} + static constexpr const char* NAME = "{{name}}Trap"; + + {{#fields}} {{type}} {{field_name}}{}; -{{/fields}} + {{/fields}} + + template + void forEachLabel(F f) { + {{#fields}} + {{#is_label}} + f("{{field_name}}", -1, {{field_name}}); + {{/is_label}} + {{/fields}} + } }; std::ostream &operator<<(std::ostream &out, const {{name}}Trap &e); + {{#id}} namespace detail { diff --git a/swift/extractor/infra/SwiftDispatcher.h b/swift/extractor/infra/SwiftDispatcher.h index 127fb7c1c3e..f8920bf0860 100644 --- a/swift/extractor/infra/SwiftDispatcher.h +++ b/swift/extractor/infra/SwiftDispatcher.h @@ -60,20 +60,29 @@ class SwiftDispatcher { } template - void emit(const Entry& entry) { + void emit(Entry&& entry) { + entry.forEachLabel([&entry](const char* field, int index, auto& label) { + if (!label.valid()) { + std::cerr << entry.NAME << " has undefined " << field; + if (index >= 0) { + std::cerr << '[' << index << ']'; + } + std::cerr << '\n'; + } + }); trap.emit(entry); } template - void emit(const std::optional& entry) { + void emit(std::optional&& entry) { if (entry) { - emit(*entry); + emit(std::move(*entry)); } } template - void emit(const std::variant& entry) { - std::visit([this](const auto& e) { this->emit(e); }, entry); + void emit(std::variant&& entry) { + std::visit([this](auto&& e) { this->emit(std::move(e)); }, std::move(entry)); } // This is a helper method to emit TRAP entries for AST nodes that we don't fully support yet. From fda9d19a9709d10265180043580cde96401f6a60 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 11:45:27 +0100 Subject: [PATCH 47/49] Swift: replace undefined labels with `UnspecifiedElement` --- swift/extractor/infra/SwiftDispatcher.h | 72 ++++++++++++++++++++----- swift/extractor/trap/TrapLabel.h | 41 ++++++++++++-- 2 files changed, 94 insertions(+), 19 deletions(-) diff --git a/swift/extractor/infra/SwiftDispatcher.h b/swift/extractor/infra/SwiftDispatcher.h index f8920bf0860..c26a66b65e9 100644 --- a/swift/extractor/infra/SwiftDispatcher.h +++ b/swift/extractor/infra/SwiftDispatcher.h @@ -61,16 +61,26 @@ class SwiftDispatcher { template void emit(Entry&& entry) { - entry.forEachLabel([&entry](const char* field, int index, auto& label) { + bool valid = true; + entry.forEachLabel([&valid, &entry, this](const char* field, int index, auto& label) { + using Label = std::remove_reference_t; if (!label.valid()) { std::cerr << entry.NAME << " has undefined " << field; if (index >= 0) { std::cerr << '[' << index << ']'; } - std::cerr << '\n'; + if constexpr (std::is_base_of_v) { + std::cerr << ", replacing with unspecified element\n"; + label = emitUnspecified(idOf(entry), field, index); + } else { + std::cerr << ", skipping emission\n"; + valid = false; + } } }); - trap.emit(entry); + if (valid) { + trap.emit(entry); + } } template @@ -97,13 +107,39 @@ class SwiftDispatcher { emit(ElementIsUnknownTrap{label}); } + TrapLabel emitUnspecified(std::optional>&& parent, + const char* property, + int index) { + UnspecifiedElement entry{trap.createLabel()}; + entry.error = "element was unspecified by the extractor"; + entry.parent = std::move(parent); + entry.property = property; + if (index >= 0) { + entry.index = index; + } + trap.emit(entry); + return entry.id; + } + + template + std::optional> idOf(const E& entry) { + if constexpr (HasId::value) { + return entry.id; + } else { + return std::nullopt; + } + } + // This method gives a TRAP label for already emitted AST node. // If the AST node was not emitted yet, then the emission is dispatched to a corresponding // visitor (see `visit(T *)` methods below). template >* = nullptr> TrapLabelOf fetchLabel(const E& e, Args&&... args) { if constexpr (std::is_constructible_v) { - assert(e && "fetching a label on a null entity, maybe fetchOptionalLabel is to be used?"); + if (!e) { + // this will be treated on emission + return undefined_label; + } } // this is required so we avoid any recursive loop: a `fetchLabel` during the visit of `e` might // end up calling `fetchLabel` on `e` itself, so we want the visit of `e` to call `fetchLabel` @@ -214,17 +250,18 @@ class SwiftDispatcher { return std::nullopt; } - // map `fetchLabel` on the iterable `arg`, returning a vector of all labels + // map `fetchLabel` on the iterable `arg` // universal reference `Arg&&` is used to catch both temporary and non-const references, not // for perfect forwarding template auto fetchRepeatedLabels(Iterable&& arg) { - std::vector ret; + using Label = decltype(fetchLabel(*arg.begin())); + TrapLabelVectorWrapper ret; if constexpr (HasSize::value) { - ret.reserve(arg.size()); + ret.data.reserve(arg.size()); } for (auto&& e : arg) { - ret.push_back(fetchLabel(e)); + ret.data.push_back(fetchLabel(e)); } return ret; } @@ -279,6 +316,12 @@ class SwiftDispatcher { template struct HasSize().size(), void())> : std::true_type {}; + template + struct HasId : std::false_type {}; + + template + struct HasId().id, void())> : std::true_type {}; + void attachLocation(swift::SourceLoc start, swift::SourceLoc end, TrapLabel locatableLabel) { @@ -302,19 +345,20 @@ class SwiftDispatcher { TrapLabel fetchLabelFromUnion(const llvm::PointerUnion u) { TrapLabel ret{}; // with logical op short-circuiting, this will stop trying on the first successful fetch - // don't feel tempted to replace the variable with the expression inside the `assert`, or - // building with `NDEBUG` will not trigger the fetching bool unionCaseFound = (... || fetchLabelFromUnionCase(u, ret)); - assert(unionCaseFound && "llvm::PointerUnion not set to a known case"); + if (!unionCaseFound) { + // TODO emit error/warning here + return undefined_label; + } return ret; } template bool fetchLabelFromUnionCase(const llvm::PointerUnion u, TrapLabel& output) { // we rely on the fact that when we extract `ASTNode` instances (which only happens - // on `BraceStmt` elements), we cannot encounter a standalone `TypeRepr` there, so we skip - // this case; extracting `TypeRepr`s here would be problematic as we would not be able to - // provide the corresponding type + // on `BraceStmt`/`IfConfigDecl` elements), we cannot encounter a standalone `TypeRepr` there, + // so we skip this case; extracting `TypeRepr`s here would be problematic as we would not be + // able to provide the corresponding type if constexpr (!std::is_same_v) { if (auto e = u.template dyn_cast()) { output = fetchLabel(e); diff --git a/swift/extractor/trap/TrapLabel.h b/swift/extractor/trap/TrapLabel.h index 024c58fa829..784d322b213 100644 --- a/swift/extractor/trap/TrapLabel.h +++ b/swift/extractor/trap/TrapLabel.h @@ -4,9 +4,14 @@ #include #include #include +#include namespace codeql { +struct UndefinedTrapLabel {}; + +constexpr UndefinedTrapLabel undefined_label{}; + class UntypedTrapLabel { uint64_t id_; @@ -18,14 +23,17 @@ class UntypedTrapLabel { protected: UntypedTrapLabel() : id_{undefined} {} - UntypedTrapLabel(uint64_t id) : id_{id} {} + UntypedTrapLabel(uint64_t id) : id_{id} { assert(id != undefined); } public: + bool valid() const { return id_ != undefined; } + explicit operator bool() const { return valid(); } + friend std::ostream& operator<<(std::ostream& out, UntypedTrapLabel l) { // TODO: this is a temporary fix to catch us from outputting undefined labels to trap // this should be moved to a validity check, probably aided by code generation and carried out // by `SwiftDispatcher` - assert(l.id_ != undefined && "outputting an undefined label!"); + assert(l && "outputting an undefined label!"); out << '#' << std::hex << l.id_ << std::dec; return out; } @@ -44,14 +52,37 @@ class TrapLabel : public UntypedTrapLabel { using Tag = TagParam; TrapLabel() = default; + TrapLabel(UndefinedTrapLabel) : TrapLabel() {} + + TrapLabel& operator=(UndefinedTrapLabel) { + *this = TrapLabel{}; + return *this; + } // The caller is responsible for ensuring ID uniqueness. static TrapLabel unsafeCreateFromExplicitId(uint64_t id) { return {id}; } static TrapLabel unsafeCreateFromUntyped(UntypedTrapLabel label) { return {label.id_}; } - template - TrapLabel(const TrapLabel& other) : UntypedTrapLabel(other) { - static_assert(std::is_base_of_v, "wrong label assignment!"); + template + TrapLabel(const TrapLabel& other) : UntypedTrapLabel(other) { + static_assert(std::is_base_of_v, "wrong label assignment!"); + } +}; + +// wrapper class to allow directly assigning a vector of TrapLabel to a vector of +// TrapLabel if B is a base of A, using move semantics rather than copying +template +struct TrapLabelVectorWrapper { + using Tag = TagParam; + + std::vector> data; + + template + operator std::vector>() && { + static_assert(std::is_base_of_v, "wrong label assignment!"); + // reinterpret_cast is safe because TrapLabel instances differ only on the type, not the + // underlying data + return std::move(reinterpret_cast>&>(data)); } }; From 9731048836ee961cf6b06532c386588eba7db5ff Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 8 Nov 2022 11:35:01 +0100 Subject: [PATCH 48/49] Swift: remove an `assert` from swift headers An interesting byproduct was finding a problematic `assert` in the Swift headers. An incomplete `FallthroughStmt` was asserting on having a destination. I did not find any other sensible way of getting rid of the crash when running in debug mode than to patch the header. --- misc/bazel/workspace.bzl | 4 ++++ .../patches/remove_getFallthrougDest_assert.patch | 11 +++++++++++ 2 files changed, 15 insertions(+) create mode 100644 swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch diff --git a/misc/bazel/workspace.bzl b/misc/bazel/workspace.bzl index 9d1b533bb4e..4248a09d47a 100644 --- a/misc/bazel/workspace.bzl +++ b/misc/bazel/workspace.bzl @@ -22,6 +22,10 @@ def codeql_workspace(repository_name = "codeql"): _swift_prebuilt_version, repo_arch, ), + patches = [ + "@%s//swift/third_party/swift-llvm-support:patches/remove_getFallthrougDest_assert.patch" % repository_name, + ], + patch_args = ["-p1"], build_file = "@%s//swift/third_party/swift-llvm-support:BUILD.swift-prebuilt.bazel" % repository_name, sha256 = sha256, ) diff --git a/swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch b/swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch new file mode 100644 index 00000000000..c16942a807c --- /dev/null +++ b/swift/third_party/swift-llvm-support/patches/remove_getFallthrougDest_assert.patch @@ -0,0 +1,11 @@ +diff -ru a/include/swift/AST/Stmt.h b/include/swift/AST/Stmt.h +--- a/include/swift/AST/Stmt.h 2022-09-21 12:56:54.000000000 +0200 ++++ b/include/swift/AST/Stmt.h 2022-11-04 14:39:18.407971007 +0100 +@@ -920,7 +920,6 @@ + /// Get the CaseStmt block to which the fallthrough transfers control. + /// Set during Sema. + CaseStmt *getFallthroughDest() const { +- assert(FallthroughDest && "fallthrough dest is not set until Sema"); + return FallthroughDest; + } + void setFallthroughDest(CaseStmt *C) { From c61a9c59116ba15d8596cd3a78eb6c3edd0f66b9 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Tue, 8 Nov 2022 12:08:44 +0100 Subject: [PATCH 49/49] C++: Also taint the return value dereference in the `strcat` model --- .../cpp/models/implementations/Strcat.qll | 25 +++++++++---------- .../dataflow/taint-tests/localTaint.expected | 9 +++++++ .../dataflow/taint-tests/taint.cpp | 4 +-- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index e729c3cb0a4..7e55868b847 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -50,19 +50,18 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid } override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { - this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and - input.isParameter(2) and - output.isParameterDeref(0) - or - this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and - input.isParameter(3) and - output.isParameterDeref(0) - or - input.isParameterDeref(0) and - output.isParameterDeref(0) - or - input.isParameterDeref(1) and - output.isParameterDeref(0) + ( + this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and + input.isParameter(2) + or + this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and + input.isParameter(3) + or + input.isParameterDeref(0) + or + input.isParameterDeref(1) + ) and + (output.isParameterDeref(0) or output.isReturnValueDeref()) } override predicate hasArrayInput(int param) { diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 07bf9a85cf4..20f18d62201 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -5964,6 +5964,7 @@ | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat | | | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:172:18:172:24 | tainted | taint.cpp:172:3:172:8 | call to strcat | TAINT | | taint.cpp:172:18:172:24 | tainted | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:180:19:180:19 | p | taint.cpp:180:19:180:19 | p | | | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p | | @@ -6373,12 +6374,14 @@ | taint.cpp:561:9:561:13 | dest1 | taint.cpp:561:9:561:13 | ref arg dest1 | TAINT | | taint.cpp:561:9:561:13 | ref arg dest1 | taint.cpp:560:24:560:28 | dest1 | | | taint.cpp:561:9:561:13 | ref arg dest1 | taint.cpp:562:7:562:11 | dest1 | | +| taint.cpp:561:16:561:21 | source | taint.cpp:561:2:561:7 | call to strcat | TAINT | | taint.cpp:561:16:561:21 | source | taint.cpp:561:9:561:13 | ref arg dest1 | TAINT | | taint.cpp:562:7:562:11 | ref arg dest1 | taint.cpp:560:24:560:28 | dest1 | | | taint.cpp:564:9:564:13 | dest2 | taint.cpp:564:2:564:7 | call to strcat | | | taint.cpp:564:9:564:13 | dest2 | taint.cpp:564:9:564:13 | ref arg dest2 | TAINT | | taint.cpp:564:9:564:13 | ref arg dest2 | taint.cpp:560:37:560:41 | dest2 | | | taint.cpp:564:9:564:13 | ref arg dest2 | taint.cpp:565:7:565:11 | dest2 | | +| taint.cpp:564:16:564:20 | clean | taint.cpp:564:2:564:7 | call to strcat | TAINT | | taint.cpp:564:16:564:20 | clean | taint.cpp:564:9:564:13 | ref arg dest2 | TAINT | | taint.cpp:565:7:565:11 | ref arg dest2 | taint.cpp:560:37:560:41 | dest2 | | | taint.cpp:572:37:572:41 | dest1 | taint.cpp:572:37:572:41 | dest1 | | @@ -6405,9 +6408,12 @@ | taint.cpp:574:36:574:40 | ref arg dest1 | taint.cpp:572:37:572:41 | dest1 | | | taint.cpp:574:36:574:40 | ref arg dest1 | taint.cpp:575:7:575:11 | dest1 | | | taint.cpp:574:36:574:40 | ref arg dest1 | taint.cpp:576:8:576:12 | dest1 | | +| taint.cpp:574:43:574:45 | ptr | taint.cpp:574:25:574:34 | call to _mbsncat_l | TAINT | | taint.cpp:574:43:574:45 | ptr | taint.cpp:574:36:574:40 | ref arg dest1 | TAINT | +| taint.cpp:574:48:574:48 | n | taint.cpp:574:25:574:34 | call to _mbsncat_l | TAINT | | taint.cpp:574:48:574:48 | n | taint.cpp:574:36:574:40 | ref arg dest1 | TAINT | | taint.cpp:574:51:574:56 | ref arg source | taint.cpp:573:49:573:54 | source | | +| taint.cpp:574:51:574:56 | source | taint.cpp:574:25:574:34 | call to _mbsncat_l | TAINT | | taint.cpp:574:51:574:56 | source | taint.cpp:574:36:574:40 | ref arg dest1 | TAINT | | taint.cpp:575:7:575:11 | ref arg dest1 | taint.cpp:572:37:572:41 | dest1 | | | taint.cpp:575:7:575:11 | ref arg dest1 | taint.cpp:576:8:576:12 | dest1 | | @@ -6421,8 +6427,11 @@ | taint.cpp:580:36:580:40 | ref arg dest3 | taint.cpp:572:85:572:89 | dest3 | | | taint.cpp:580:36:580:40 | ref arg dest3 | taint.cpp:581:7:581:11 | dest3 | | | taint.cpp:580:36:580:40 | ref arg dest3 | taint.cpp:582:8:582:12 | dest3 | | +| taint.cpp:580:43:580:45 | ptr | taint.cpp:580:25:580:34 | call to _mbsncat_l | TAINT | | taint.cpp:580:43:580:45 | ptr | taint.cpp:580:36:580:40 | ref arg dest3 | TAINT | +| taint.cpp:580:48:580:48 | n | taint.cpp:580:25:580:34 | call to _mbsncat_l | TAINT | | taint.cpp:580:48:580:48 | n | taint.cpp:580:36:580:40 | ref arg dest3 | TAINT | +| taint.cpp:580:51:580:55 | clean | taint.cpp:580:25:580:34 | call to _mbsncat_l | TAINT | | taint.cpp:580:51:580:55 | clean | taint.cpp:580:36:580:40 | ref arg dest3 | TAINT | | taint.cpp:580:51:580:55 | ref arg clean | taint.cpp:573:32:573:36 | clean | | | taint.cpp:581:7:581:11 | ref arg dest3 | taint.cpp:572:85:572:89 | dest3 | | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index 2ae093098d2..0271b8205e4 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -574,8 +574,8 @@ void test__mbsncat_l(unsigned char* dest1, unsigned const char* ptr, unsigned ch unsigned char* dest2 = _mbsncat_l(dest1, ptr, n, source); sink(dest1); // $ SPURIOUS: ast,ir sink(*dest1); // $ ast,ir - sink(dest2); // $ SPURIOUS: ir - sink(*dest2); // $ ir + sink(dest2); // $ SPURIOUS: ast,ir + sink(*dest2); // $ ast,ir unsigned char* dest4 = _mbsncat_l(dest3, ptr, n, clean); sink(dest3);