From de429752d190493451ad10e2ce5fb8f61994f977 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 23 Nov 2018 18:00:01 +0000 Subject: [PATCH] JavaScript: Restructure implementation of `DataFlow::SourceNode`. It now uses a facade pattern similar to `InvokeNode`: the range of the class is defined by an abstract class `DataFlow::SourceNode::Range`, while the actual behaviour is defined by the (no longer abstract) `SourceNode` class itself. Clients that want to add new source nodes need to extend `DataFlow::SourceNode::Range`, those that want to refine the behaviour of existing source nodes should extend `DataFlow::SourceNode` itself. While this is technically a breaking API change, I think separating the two aspects in this way is cleaner and makes it easier to use, and improves performance as well. --- change-notes/1.20/analysis-javascript.md | 3 + .../ql/src/semmle/javascript/EmailClients.qll | 2 +- .../src/semmle/javascript/StandardLibrary.qll | 2 +- .../semmle/javascript/dataflow/DataFlow.qll | 14 +++ .../src/semmle/javascript/dataflow/Nodes.qll | 23 ++-- .../semmle/javascript/dataflow/Sources.qll | 111 ++++++++++-------- .../frameworks/AngularJS/AngularJSCore.qll | 2 +- .../src/semmle/javascript/frameworks/HTTP.qll | 4 + .../frameworks/LodashUnderscore.qll | 4 + .../javascript/frameworks/ReactNative.qll | 2 +- 10 files changed, 106 insertions(+), 61 deletions(-) diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index ddc519a440fb..0fc074c80064 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -33,3 +33,6 @@ | Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. | ## Changes to QL libraries + +* `DataFlow::SourceNode` is no longer an abstract class; to add new source nodes, extend `DataFlow::SourceNode::Range` instead. +* Subclasses of `DataFlow::PropRead` are no longer automatically made source nodes; you now need to additionally define a corresponding subclass of `DataFlow::SourceNode::Range` to achieve this. diff --git a/javascript/ql/src/semmle/javascript/EmailClients.qll b/javascript/ql/src/semmle/javascript/EmailClients.qll index 7df4939bc9df..e84148c81521 100644 --- a/javascript/ql/src/semmle/javascript/EmailClients.qll +++ b/javascript/ql/src/semmle/javascript/EmailClients.qll @@ -3,7 +3,7 @@ import javascript /** * An operation that sends an email. */ -abstract class EmailSender extends DataFlow::DefaultSourceNode { +abstract class EmailSender extends DataFlow::SourceNode { /** * Gets a data flow node holding the plaintext version of the email body. */ diff --git a/javascript/ql/src/semmle/javascript/StandardLibrary.qll b/javascript/ql/src/semmle/javascript/StandardLibrary.qll index 69d23de9932c..c49dc821e2a4 100644 --- a/javascript/ql/src/semmle/javascript/StandardLibrary.qll +++ b/javascript/ql/src/semmle/javascript/StandardLibrary.qll @@ -76,7 +76,7 @@ private class AnalyzedThisInArrayIterationFunction extends AnalyzedNode, DataFlo /** * A definition of a `Promise` object. */ -abstract class PromiseDefinition extends DataFlow::DefaultSourceNode { +abstract class PromiseDefinition extends DataFlow::SourceNode { /** Gets the executor function of this promise object. */ abstract DataFlow::FunctionNode getExecutor(); diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index ac27a0653262..ca559766ac64 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll @@ -515,6 +515,20 @@ module DataFlow { */ abstract class PropRead extends PropRef, SourceNode { } + /** + * A property read, considered as a source node. + * + * Note that we cannot simplify the characteristic predicate to `this instanceof PropRead`, + * since `PropRead` is itself a subclass of `SourceNode`. + */ + private class PropReadAsSourceNode extends SourceNode::Range { + PropReadAsSourceNode() { + this = TPropNode(any(PropertyPattern p)) or + this instanceof RestPatternNode or + this instanceof ElementPatternNode + } + } + /** * A property access in rvalue position. */ diff --git a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll index 998c28b11ade..ebeca2da208c 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll @@ -7,7 +7,7 @@ import javascript /** A data flow node corresponding to a parameter. */ -class ParameterNode extends DataFlow::DefaultSourceNode { +class ParameterNode extends DataFlow::SourceNode { Parameter p; ParameterNode() { DataFlow::parameterNode(this, p) } @@ -20,7 +20,7 @@ class ParameterNode extends DataFlow::DefaultSourceNode { } /** A data flow node corresponding to a function invocation (with or without `new`). */ -class InvokeNode extends DataFlow::DefaultSourceNode { +class InvokeNode extends DataFlow::SourceNode { DataFlow::Impl::InvokeNodeDef impl; InvokeNode() { this = impl } @@ -168,7 +168,7 @@ class MethodCallNode extends CallNode { class NewNode extends InvokeNode { override DataFlow::Impl::NewNodeDef impl; } /** A data flow node corresponding to the `this` parameter in a function or `this` at the top-level. */ -class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode { +class ThisNode extends DataFlow::Node, DataFlow::SourceNode { ThisNode() { DataFlow::thisNode(this, _) } /** @@ -202,7 +202,7 @@ class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode { } /** A data flow node corresponding to a global variable access. */ -class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { +class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::SourceNode { override GlobalVarAccess astNode; /** Gets the name of the global variable. */ @@ -216,7 +216,10 @@ class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode */ DataFlow::SourceNode globalObjectRef() { // top-level `this` - exists(ThisNode globalThis | result = globalThis | not exists(globalThis.getBinder())) + exists(StmtContainer sc | + sc = result.(ThisNode).getBindingContainer() and + not sc instanceof Function + ) or // DOM result = globalVarRef("window") @@ -244,7 +247,7 @@ DataFlow::SourceNode globalVarRef(string name) { } /** A data flow node corresponding to a function definition. */ -class FunctionNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { +class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode { override Function astNode; /** Gets the `i`th parameter of this function. */ @@ -287,12 +290,12 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { } /** A data flow node corresponding to an object literal expression. */ -class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { +class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode { override ObjectExpr astNode; } /** A data flow node corresponding to an array literal expression. */ -class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { +class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode { override ArrayExpr astNode; /** Gets the `i`th element of this array literal. */ @@ -323,7 +326,7 @@ class ArrayConstructorInvokeNode extends DataFlow::InvokeNode { * A data flow node corresponding to the creation or a new array, either through an array literal * or an invocation of the `Array` constructor. */ -class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { +class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::SourceNode { ArrayCreationNode() { this instanceof ArrayLiteralNode or this instanceof ArrayConstructorInvokeNode @@ -346,7 +349,7 @@ class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode * For compatibility with old transpilers, we treat `import * from '...'` * as a default import as well. */ -class ModuleImportNode extends DataFlow::DefaultSourceNode { +class ModuleImportNode extends DataFlow::SourceNode { string path; ModuleImportNode() { diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll index aaca401dc684..1890d3435a19 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll @@ -9,15 +9,15 @@ import javascript /** - * A source node for local data flow, that is, a node for which local - * data flow cannot provide any information about its inputs. + * A source node for local data flow, that is, a node from which local data flow is tracked. * - * By default, functions, object and array expressions and JSX nodes - * are considered sources, as well as expressions that have non-local - * flow (such as calls and property accesses). Additional sources - * can be modelled by extending this class with additional subclasses. + * Examples include function parameters, imports and property accesses; see + * `DataFlow::SourceNode::DefaultRange` for details. You can introduce new kinds of + * source nodes by defining new subclasses of `DataFlow::SourceNode::Range`. */ -abstract class SourceNode extends DataFlow::Node { +class SourceNode extends DataFlow::Node { + SourceNode() { this instanceof SourceNode::Range } + /** * Holds if this node flows into `sink` in zero or more local (that is, * intra-procedural) steps. @@ -167,45 +167,62 @@ abstract class SourceNode extends DataFlow::Node { } } -/** - * A data flow node that is considered a source node by default. - * - * Currently, the following nodes are source nodes: - * - import specifiers - * - non-destructuring function parameters - * - property accesses - * - function invocations - * - `this` expressions - * - global variable accesses - * - function definitions - * - class definitions - * - object expressions - * - array expressions - * - JSX literals. - */ -class DefaultSourceNode extends SourceNode { - DefaultSourceNode() { - exists(ASTNode astNode | this = DataFlow::valueNode(astNode) | - astNode instanceof PropAccess or - astNode instanceof Function or - astNode instanceof ClassDefinition or - astNode instanceof ObjectExpr or - astNode instanceof ArrayExpr or - astNode instanceof JSXNode or - astNode instanceof GlobalVarAccess or - astNode instanceof ExternalModuleReference - ) - or - exists(SsaExplicitDefinition ssa, VarDef def | - this = DataFlow::ssaDefinitionNode(ssa) and def = ssa.getDef() - | - def instanceof ImportSpecifier - ) - or - DataFlow::parameterNode(this, _) - or - this instanceof DataFlow::Impl::InvokeNodeDef - or - DataFlow::thisNode(this, _) +module SourceNode { + /** + * A data flow node that should be considered a source node. + * + * Subclass this class to introduce new kinds of source nodes. If you want to refine + * the definition of existing source nodes, subclass `DataFlow::SourceNode` instead. + */ + cached + abstract class Range extends DataFlow::Node { } + + /** + * A data flow node that is considered a source node by default. + * + * Currently, the following nodes are source nodes: + * - import specifiers + * - function parameters + * - `this` nodes + * - property accesses + * - function invocations + * - global variable accesses + * - function definitions + * - class definitions + * - object expressions + * - array expressions + * - JSX literals + * + * This class is for internal use only and should not normally be used directly. + */ + class DefaultRange extends Range { + DefaultRange() { + exists(ASTNode astNode | this = DataFlow::valueNode(astNode) | + astNode instanceof PropAccess or + astNode instanceof Function or + astNode instanceof ClassDefinition or + astNode instanceof ObjectExpr or + astNode instanceof ArrayExpr or + astNode instanceof JSXNode or + astNode instanceof GlobalVarAccess or + astNode instanceof ExternalModuleReference + ) + or + exists(SsaExplicitDefinition ssa, VarDef def | + this = DataFlow::ssaDefinitionNode(ssa) and def = ssa.getDef() + | + def instanceof ImportSpecifier + ) + or + DataFlow::parameterNode(this, _) + or + this instanceof DataFlow::Impl::InvokeNodeDef + or + DataFlow::thisNode(this, _) + } } } + +deprecated class DefaultSourceNode extends SourceNode { + DefaultSourceNode() { this instanceof SourceNode::DefaultRange } +} diff --git a/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll b/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll index f5eb22ab8d5c..41e4e0ec0a5f 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll @@ -45,7 +45,7 @@ private predicate isAngularString(Expr s) { * String literals in Angular code are often used as identifiers or references, so we * want to track them. */ -private class TrackStringsInAngularCode extends DataFlow::SourceNode, DataFlow::ValueNode { +private class TrackStringsInAngularCode extends DataFlow::SourceNode::Range, DataFlow::ValueNode { TrackStringsInAngularCode() { isAngularString(astNode) } } diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll index 66d5bae18c4c..a9c7813f2dad 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll @@ -435,6 +435,10 @@ 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 7ab3e2f8d20b..51f4e94ee902 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll @@ -14,6 +14,10 @@ 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. */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/ReactNative.qll b/javascript/ql/src/semmle/javascript/frameworks/ReactNative.qll index 8d554bf55f1a..f639a163f60e 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ReactNative.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ReactNative.qll @@ -6,7 +6,7 @@ import javascript module ReactNative { /** A `WebView` JSX element. */ - class WebViewElement extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { + class WebViewElement extends DataFlow::ValueNode, DataFlow::SourceNode { override JSXElement astNode; WebViewElement() {