From ee106b1103f36f41a9c4669dd507d5e9fc7641b9 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 2 Apr 2020 12:21:10 +0100 Subject: [PATCH 01/10] JS: Remove tautological SourceNode::Range subclasses --- javascript/ql/src/semmle/javascript/frameworks/HTTP.qll | 4 ---- .../ql/src/semmle/javascript/frameworks/LodashUnderscore.qll | 4 ---- 2 files changed, 8 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll index fd28cd436665..6110bb92aca7 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll @@ -467,10 +467,6 @@ module HTTP { abstract DataFlow::Node getASecretKey(); } - private class CookieMiddlewareInstanceAsSourceNode extends DataFlow::SourceNode::Range { - CookieMiddlewareInstanceAsSourceNode() { this instanceof CookieMiddlewareInstance } - } - /** * A key used for signed cookies, viewed as a `CryptographicKey`. */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll index 16c8fd851bfc..5461231365f3 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll @@ -14,10 +14,6 @@ module LodashUnderscore { abstract string getName(); } - private class MemberAsSourceNode extends DataFlow::SourceNode::Range { - MemberAsSourceNode() { this instanceof Member } - } - /** * An import of `lodash` or `underscore` accessing a given member of that package. */ From 8f930fc3e65c0d9485be0939f469790296a9c41f Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 2 Apr 2020 12:25:33 +0100 Subject: [PATCH 02/10] JS: Remove recursive SourceNode from AngularJS --- .../frameworks/AngularJS/AngularJSCore.qll | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll b/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll index 6eede4a1aa85..09445534253c 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll @@ -23,14 +23,33 @@ DataFlow::SourceNode angular() { result = DataFlow::moduleImport("angular") } -pragma[noopt] +/** + * Holds if `tl` appears to be a top-level using the AngularJS library. + * + * Should not depend on the `SourceNode` class. + */ +pragma[nomagic] +private predicate isAngularTopLevel(TopLevel tl) { + exists(Import imprt | + imprt.getTopLevel() = tl and + imprt.getImportedPath().getValue() = "angular" + ) + or + exists(GlobalVarAccess global | + global.getName() = "angular" and + global.getTopLevel() = tl + ) +} + +/** + * Holds if `s` is a string in a top-level using the AngularJS library. + * + * Should not depend on the `SourceNode` class. + */ +pragma[nomagic] private predicate isAngularString(Expr s) { - exists(DataFlow::SourceNode angular, StmtContainer sc, TopLevel tl | - angular = angular() and - sc = angular.getContainer() and - tl = sc.getTopLevel() and - tl = s.getTopLevel() - | + isAngularTopLevel(s.getTopLevel()) and + ( s instanceof StringLiteral or s instanceof TemplateLiteral ) From 3804d3fcfdaeed6c2ecb6b250ae1f6561fc00e32 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 2 Apr 2020 13:21:34 +0100 Subject: [PATCH 03/10] JS: Remove Import->SourceNode dependency from lazy cache --- .../javascript/frameworks/LazyCache.qll | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll b/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll index b8c1c1e0c1fb..0477c14acac8 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll @@ -6,8 +6,11 @@ import javascript module LazyCache { /** + * DEPRECATED. DO NOT USE. + * * A lazy-cache object, usually created through an expression of form `require('lazy-cache')(require)`. */ + deprecated class LazyCacheObject extends DataFlow::SourceNode { LazyCacheObject() { // Use `require` directly instead of `moduleImport` to avoid recursion. @@ -19,13 +22,26 @@ module LazyCache { } } + /** + * A variable containing a lazy-cache object. + */ + class LazyCacheVariable extends LocalVariable { + LazyCacheVariable() { + // To avoid recursion, this should not depend on `SourceNode`. + exists(Require req | + req.getArgument(0).getStringValue() = "lazy-cache" and + getAnAssignedExpr().(CallExpr).getCallee() = req + ) + } + } + /** * An import through `lazy-cache`. */ class LazyCacheImport extends CallExpr, Import { - LazyCacheObject cache; + LazyCacheVariable cache; - LazyCacheImport() { this = cache.getACall().asExpr() } + LazyCacheImport() { getCallee() = cache.getAnAccess() } /** Gets the name of the package as it's exposed on the lazy-cache object. */ string getLocalAlias() { @@ -39,10 +55,23 @@ module LazyCache { override PathExpr getImportedPath() { result = getArgument(0) } + private LazyCacheVariable getVariable() { + result = cache + } + + pragma[noopt] override DataFlow::Node getImportedModuleNode() { + this instanceof LazyCacheImport and result = this.flow() or - result = cache.getAPropertyRead(getLocalAlias()) + exists(LazyCacheVariable variable, Expr base, PropAccess access, string localName | + variable = getVariable() and + base = variable.getAnAccess() and + access.getBase() = base and + localName = getLocalAlias() and + access.getPropertyName() = localName and + result = access.flow() + ) } } From 346867f42574381c73e1f4da67b73ee58c68608c Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 2 Apr 2020 15:30:17 +0100 Subject: [PATCH 04/10] JS: Remove Import->SourceNode dependency from AMD --- javascript/ql/src/semmle/javascript/AMD.qll | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/AMD.qll b/javascript/ql/src/semmle/javascript/AMD.qll index 38d049c088d6..2beee7f6ce99 100644 --- a/javascript/ql/src/semmle/javascript/AMD.qll +++ b/javascript/ql/src/semmle/javascript/AMD.qll @@ -56,10 +56,16 @@ class AmdModuleDefinition extends CallExpr { */ pragma[nomagic] DataFlow::SourceNode getFactoryNode() { - result.flowsToExpr(getLastArgument()) and + result = getFactoryNodeInternal() and result instanceof DataFlow::ValueNode } + private + DataFlow::Node getFactoryNodeInternal() { + result = DataFlow::valueNode(getLastArgument()) or + result = getFactoryNodeInternal().getAPredecessor() + } + /** Gets the expression defining this module. */ Expr getModuleExpr() { exists(DataFlow::Node f | f = getFactoryNode() | @@ -108,7 +114,7 @@ class AmdModuleDefinition extends CallExpr { * Gets the `i`th parameter of the factory function of this module. */ private SimpleParameter getFactoryParameter(int i) { - getFactoryNode().(DataFlow::FunctionNode).getParameter(i) = DataFlow::parameterNode(result) + getFactoryNodeInternal().asExpr().(Function).getParameter(i) = result } /** From 93971e943342dd606fda9a97f26b86fc5aad953e Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 2 Apr 2020 15:30:33 +0100 Subject: [PATCH 05/10] JS: Make local flow not depend on SourceNode --- javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index 5d7ce03e467b..d7bf1e304ea0 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll @@ -1369,18 +1369,18 @@ module DataFlow { */ private predicate lvalueDefaultFlowStep(Node pred, Node succ) { exists(PropertyPattern pattern | - pred = valueNode(pattern.getDefault()) and + pred = TValueNode(pattern.getDefault()) and succ = lvalueNode(pattern.getValuePattern()) ) or exists(ArrayPattern array, int i | - pred = valueNode(array.getDefault(i)) and + pred = TValueNode(array.getDefault(i)) and succ = lvalueNode(array.getElement(i)) ) or exists(Parameter param | - pred = valueNode(param.getDefault()) and - succ = parameterNode(param) + pred = TValueNode(param.getDefault()) and + parameterNode(succ, param) ) } From ffbbdd777974d61797c78ea0e99aa56a125fd056 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 2 Apr 2020 23:04:24 +0100 Subject: [PATCH 06/10] JS: Autoformat --- javascript/ql/src/semmle/javascript/AMD.qll | 3 +-- .../ql/src/semmle/javascript/frameworks/LazyCache.qll | 7 ++----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/AMD.qll b/javascript/ql/src/semmle/javascript/AMD.qll index 2beee7f6ce99..6d86b4d03a64 100644 --- a/javascript/ql/src/semmle/javascript/AMD.qll +++ b/javascript/ql/src/semmle/javascript/AMD.qll @@ -60,8 +60,7 @@ class AmdModuleDefinition extends CallExpr { result instanceof DataFlow::ValueNode } - private - DataFlow::Node getFactoryNodeInternal() { + private DataFlow::Node getFactoryNodeInternal() { result = DataFlow::valueNode(getLastArgument()) or result = getFactoryNodeInternal().getAPredecessor() } diff --git a/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll b/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll index 0477c14acac8..510d22dbddd2 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll @@ -10,8 +10,7 @@ module LazyCache { * * A lazy-cache object, usually created through an expression of form `require('lazy-cache')(require)`. */ - deprecated - class LazyCacheObject extends DataFlow::SourceNode { + deprecated class LazyCacheObject extends DataFlow::SourceNode { LazyCacheObject() { // Use `require` directly instead of `moduleImport` to avoid recursion. // For the same reason, avoid `Import.getImportedPath`. @@ -55,9 +54,7 @@ module LazyCache { override PathExpr getImportedPath() { result = getArgument(0) } - private LazyCacheVariable getVariable() { - result = cache - } + private LazyCacheVariable getVariable() { result = cache } pragma[noopt] override DataFlow::Node getImportedModuleNode() { From 171b131eb16820818a6b28276cb4e25602090ef6 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 8 Apr 2020 10:23:47 +0100 Subject: [PATCH 07/10] JS: Add test for SourceNode not depending on flowsTo --- .../SourceNodeFlowsTo.expected | 1 + .../RecursionPrevention/SourceNodeFlowsTo.ql | 19 +++++++++++++++++++ .../library-tests/RecursionPrevention/tst.js | 2 ++ 3 files changed, 22 insertions(+) create mode 100644 javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.expected create mode 100644 javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql create mode 100644 javascript/ql/test/library-tests/RecursionPrevention/tst.js diff --git a/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.expected b/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.expected new file mode 100644 index 000000000000..59f6fd6e79b5 --- /dev/null +++ b/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.expected @@ -0,0 +1 @@ +| Success | diff --git a/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql b/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql new file mode 100644 index 000000000000..2bc3990c1e76 --- /dev/null +++ b/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql @@ -0,0 +1,19 @@ +/** + * Test that fails to compile if the domain of `SourceNode` depends on `SourceNode.flowsTo` (recursively). + * + * This tests adds a negative dependency `flowsTo --!--> SourceNode` + * so that the undesired edge `SourceNode --> flowsTo` completes a negative cycle. + */ +import javascript + +class BadSourceNode extends DataFlow::SourceNode { + BadSourceNode() { + this.(DataFlow::PropRead).getPropertyName() = "foo" + } + + override predicate flowsTo(DataFlow::Node node) { + not node instanceof DataFlow::SourceNode + } +} + +select "Success" diff --git a/javascript/ql/test/library-tests/RecursionPrevention/tst.js b/javascript/ql/test/library-tests/RecursionPrevention/tst.js new file mode 100644 index 000000000000..d5a82a682bcf --- /dev/null +++ b/javascript/ql/test/library-tests/RecursionPrevention/tst.js @@ -0,0 +1,2 @@ +// The contents of this file don't matter +let x = 1; From 4acb9da2cf532d4050c8c0e5dcc99a6ae7dad048 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 8 Apr 2020 10:30:21 +0100 Subject: [PATCH 08/10] Update javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll Co-Authored-By: Esben Sparre Andreasen --- javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll b/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll index 510d22dbddd2..c7efb210ab75 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LazyCache.qll @@ -62,6 +62,7 @@ module LazyCache { result = this.flow() or exists(LazyCacheVariable variable, Expr base, PropAccess access, string localName | + // To avoid recursion, this should not depend on `SourceNode`. variable = getVariable() and base = variable.getAnAccess() and access.getBase() = base and From 4ca3ac5ee9e7c17bd537ecefc113d0ef5297f094 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 8 Apr 2020 10:30:45 +0100 Subject: [PATCH 09/10] JS: Add another warning --- javascript/ql/src/semmle/javascript/AMD.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/src/semmle/javascript/AMD.qll b/javascript/ql/src/semmle/javascript/AMD.qll index 6d86b4d03a64..799db5e0fb5d 100644 --- a/javascript/ql/src/semmle/javascript/AMD.qll +++ b/javascript/ql/src/semmle/javascript/AMD.qll @@ -61,6 +61,7 @@ class AmdModuleDefinition extends CallExpr { } private DataFlow::Node getFactoryNodeInternal() { + // To avoid recursion, this should not depend on `SourceNode`. result = DataFlow::valueNode(getLastArgument()) or result = getFactoryNodeInternal().getAPredecessor() } From 5ab595da2eba203f1908098432e0a1b655187b2d Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 8 Apr 2020 12:40:00 +0100 Subject: [PATCH 10/10] JS: Autoformat --- .../RecursionPrevention/SourceNodeFlowsTo.ql | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql b/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql index 2bc3990c1e76..9a5350948fe2 100644 --- a/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql +++ b/javascript/ql/test/library-tests/RecursionPrevention/SourceNodeFlowsTo.ql @@ -4,16 +4,13 @@ * This tests adds a negative dependency `flowsTo --!--> SourceNode` * so that the undesired edge `SourceNode --> flowsTo` completes a negative cycle. */ + import javascript class BadSourceNode extends DataFlow::SourceNode { - BadSourceNode() { - this.(DataFlow::PropRead).getPropertyName() = "foo" - } + BadSourceNode() { this.(DataFlow::PropRead).getPropertyName() = "foo" } - override predicate flowsTo(DataFlow::Node node) { - not node instanceof DataFlow::SourceNode - } + override predicate flowsTo(DataFlow::Node node) { not node instanceof DataFlow::SourceNode } } select "Success"