From 7345df63c02d8da25d8266076555e4a82da19640 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Mon, 26 Oct 2020 23:25:23 +0000 Subject: [PATCH 01/14] JS: Include DataFlow::AdditionalFlowStep in TaintSteps metric --- .../src/meta/analysis-quality/TaintSteps.ql | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/meta/analysis-quality/TaintSteps.ql b/javascript/ql/src/meta/analysis-quality/TaintSteps.ql index fdafa5197ab..5e40d63fa3d 100644 --- a/javascript/ql/src/meta/analysis-quality/TaintSteps.ql +++ b/javascript/ql/src/meta/analysis-quality/TaintSteps.ql @@ -11,12 +11,22 @@ import javascript import CallGraphQuality -class BasicTaintConfiguration extends TaintTracking::Configuration { - BasicTaintConfiguration() { this = "BasicTaintConfiguration" } -} - predicate relevantStep(DataFlow::Node pred, DataFlow::Node succ) { - any(BasicTaintConfiguration cfg).isAdditionalFlowStep(pred, succ) and + ( + any(TaintTracking::AdditionalTaintStep dts).step(pred, succ) + or + any(DataFlow::AdditionalFlowStep cfg).step(pred, succ) + or + any(DataFlow::AdditionalFlowStep cfg).step(pred, succ, _, _) + or + any(DataFlow::AdditionalFlowStep cfg).loadStep(pred, succ, _) + or + any(DataFlow::AdditionalFlowStep cfg).storeStep(pred, succ, _) + or + any(DataFlow::AdditionalFlowStep cfg).loadStoreStep(pred, succ, _, _) + or + any(DataFlow::AdditionalFlowStep cfg).loadStoreStep(pred, succ, _) + ) and not pred.getFile() instanceof IgnoredFile and not succ.getFile() instanceof IgnoredFile } From 9fc5c0bdb87db5967d35547dc61c7e9d8e41e625 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Tue, 27 Oct 2020 14:31:50 +0000 Subject: [PATCH 02/14] JS: Update ComposedFunctions --- .../frameworks/ComposedFunctions.qll | 138 ++++++++++++++---- .../ComposedFunctions/compose.expected | 20 +-- .../frameworks/ComposedFunctions/tst.js | 35 +++-- 3 files changed, 138 insertions(+), 55 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll index 7f56771b8a8..9427581b9b8 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll @@ -5,31 +5,110 @@ import javascript /** - * A function composed from a collection of functions. + * A call to a function that constructs a function composition `f(g(h(...)))` from a + * series functions `f, g, h, ...`. */ -private class ComposedFunction extends DataFlow::CallNode { - ComposedFunction() { - exists(string name | - name = "just-compose" or - name = "compose-function" - | - this = DataFlow::moduleImport(name).getACall() - ) - or - this = LodashUnderscore::member("flow").getACall() +class FunctionCompositionCall extends DataFlow::CallNode { + FunctionCompositionCall::Range range; + + FunctionCompositionCall() { this = range } + + /** + * Gets the `i`th function in the composition `f(g(h(...)))`, counting from left to right. + * + * Note that this is the opposite of the order in which the function are invoked, + * that is, `g` occurs later than `f` in `f(g(...))` but is invoked before `f`. + */ + DataFlow::Node getOperandNode(int i) { result = range.getOperandNode(i) } + + /** Gets a node holding one of the functions to be composed. */ + final DataFlow::Node getAnOperandNode() { result = getOperandNode(_) } + + /** + * Gets the function flowing into the `i`th function in the composition `f(g(h(...)))`. + * + * Note that this is the opposite of the order in which the function are invoked, + * that is, `g` occurs later than `f` in `f(g(...))` but is invoked before `f`. + */ + final DataFlow::FunctionNode getOperandFunction(int i) { + result = getOperandNode(i).getALocalSource() + } + + /** Gets any of the functions being composed. */ + final DataFlow::Node getAnOperandFunction() { result = getOperandFunction(_) } + + /** Gets the number of functions being composed. */ + int getNumOperand() { result = range.getNumOperand() } +} + +module FunctionCompositionCall { + abstract class Range extends DataFlow::CallNode { + /** + * Gets the function flowing into the `i`th function in the composition `f(g(h(...)))`. + */ + abstract DataFlow::Node getOperandNode(int i); + + /** Gets the number of functions being composed. */ + abstract int getNumOperand(); } /** - * Gets the ith function in this composition. + * A function composition call that accepts its operands in an array or + * via the arguments list. + * + * For simplicity, we model every composition function as if it supported this. */ - DataFlow::FunctionNode getFunction(int i) { result.flowsTo(getArgument(i)) } + private abstract class WithArrayOverloading extends Range { + /** Gets the `i`th argument to the call or the `i`th array element passed into the call. */ + DataFlow::Node getEffectiveArgument(int i) { + result = getArgument(0).(DataFlow::ArrayCreationNode).getElement(i) + or + not getArgument(0) instanceof DataFlow::ArrayCreationNode and + result = getArgument(i) + } + + override int getNumOperand() { + result = getArgument(0).(DataFlow::ArrayCreationNode).getSize() + or + not getArgument(0) instanceof DataFlow::ArrayCreationNode and + result = getNumArgument() + } + } + + /** A call whose arguments are functions `f,g,h` which are composed into `f(g(h(...))` */ + private class RightToLeft extends WithArrayOverloading { + RightToLeft() { + this = DataFlow::moduleImport(["compose-function"]).getACall() + or + this = DataFlow::moduleMember(["redux", "ramda"], "compose").getACall() + or + this = LodashUnderscore::member("flowRight").getACall() + } + + override DataFlow::Node getOperandNode(int i) { + result = getEffectiveArgument(i) + } + } + + /** A call whose arguments are functions `f,g,h` which are composed into `f(g(h(...))` */ + private class LeftToRight extends WithArrayOverloading { + LeftToRight() { + this = DataFlow::moduleImport("just-compose").getACall() + or + this = LodashUnderscore::member("flow").getACall() + } + + override DataFlow::Node getOperandNode(int i) { + result = getEffectiveArgument(getNumOperand() - i - 1) + } + } } /** * A taint step for a composed function. */ private class ComposedFunctionTaintStep extends TaintTracking::AdditionalTaintStep { - ComposedFunction composed; + FunctionCompositionCall composed; DataFlow::CallNode call; ComposedFunctionTaintStep() { @@ -38,25 +117,24 @@ private class ComposedFunctionTaintStep extends TaintTracking::AdditionalTaintSt } override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(int fnIndex, DataFlow::FunctionNode fn | fn = composed.getFunction(fnIndex) | + exists(int fnIndex, DataFlow::FunctionNode fn | fn = composed.getOperandFunction(fnIndex) | // flow out of the composed call - fnIndex = composed.getNumArgument() - 1 and - pred = fn.getAReturn() and + fnIndex = 0 and + pred = fn.getReturnNode() and succ = this or - if fnIndex = 0 - then - // flow into the first composed function - exists(int callArgIndex | - pred = call.getArgument(callArgIndex) and - succ = fn.getParameter(callArgIndex) - ) - else - // flow through the composed functions - exists(DataFlow::FunctionNode predFn | predFn = composed.getFunction(fnIndex - 1) | - pred = predFn.getAReturn() and - succ = fn.getParameter(0) - ) + // flow into the first function + fnIndex = composed.getNumOperand() - 1 and + exists(int callArgIndex | + pred = call.getArgument(callArgIndex) and + succ = fn.getParameter(callArgIndex) + ) + or + // flow through the composed functions + exists(DataFlow::FunctionNode predFn | predFn = composed.getOperandFunction(fnIndex + 1) | + pred = predFn.getReturnNode() and + succ = fn.getParameter(0) + ) ) } } diff --git a/javascript/ql/test/library-tests/frameworks/ComposedFunctions/compose.expected b/javascript/ql/test/library-tests/frameworks/ComposedFunctions/compose.expected index 52b552b3d38..932f4ea6d43 100644 --- a/javascript/ql/test/library-tests/frameworks/ComposedFunctions/compose.expected +++ b/javascript/ql/test/library-tests/frameworks/ComposedFunctions/compose.expected @@ -1,13 +1,13 @@ | tst.js:10:10:10:15 | source | | tst.js:15:10:15:13 | f1() | -| tst.js:20:10:20:23 | compose1(f2)() | -| tst.js:28:10:28:27 | compose1(f3, f4)() | -| tst.js:33:10:33:28 | compose1(o.f, f5)() | -| tst.js:41:10:41:27 | compose1(f6, f7)() | -| tst.js:49:10:49:33 | compose ... source) | -| tst.js:61:10:61:40 | compose ... source) | -| tst.js:66:10:66:30 | compose ... source) | +| tst.js:20:10:20:24 | lcompose1(f2)() | +| tst.js:28:10:28:28 | lcompose1(f3, f4)() | +| tst.js:33:10:33:29 | lcompose1(o.f, f5)() | +| tst.js:41:10:41:28 | lcompose1(f6, f7)() | +| tst.js:49:10:49:34 | lcompos ... source) | +| tst.js:61:10:61:41 | lcompos ... source) | +| tst.js:66:10:66:31 | lcompos ... source) | | tst.js:89:10:89:31 | f18(und ... source) | -| tst.js:94:10:94:24 | compose2(f19)() | -| tst.js:99:10:99:24 | compose3(f20)() | -| tst.js:104:10:104:24 | compose4(f21)() | \ No newline at end of file +| tst.js:94:10:94:30 | rcompos ... o.f)() | +| tst.js:99:10:99:30 | lcompos ... f20)() | +| tst.js:104:10:104:30 | lcompos ... f21)() | diff --git a/javascript/ql/test/library-tests/frameworks/ComposedFunctions/tst.js b/javascript/ql/test/library-tests/frameworks/ComposedFunctions/tst.js index e6c7a6804d4..95426079a03 100644 --- a/javascript/ql/test/library-tests/frameworks/ComposedFunctions/tst.js +++ b/javascript/ql/test/library-tests/frameworks/ComposedFunctions/tst.js @@ -1,8 +1,8 @@ -import compose1 from 'just-compose'; -import compose2 from 'compose-function'; -import compose3 from 'lodash.flow'; +import lcompose1 from 'just-compose'; +import rcompose2 from 'compose-function'; +import lcompose3 from 'lodash.flow'; import _ from 'lodash'; -var compose4 = _.flow; +var lcompose4 = _.flow; (function(){ var source = SOURCE(); @@ -17,7 +17,7 @@ var compose4 = _.flow; function f2(){ return source; } - SINK(compose1(f2)()); + SINK(lcompose1(f2)()); function f3(){ @@ -25,12 +25,12 @@ var compose4 = _.flow; function f4(){ return source; } - SINK(compose1(f3, f4)()); + SINK(lcompose1(f3, f4)()); function f5(){ return source; } - SINK(compose1(o.f, f5)()); + SINK(lcompose1(o.f, f5)()); function f6(){ return source; @@ -38,7 +38,7 @@ var compose4 = _.flow; function f7(x){ return x; } - SINK(compose1(f6, f7)()); + SINK(lcompose1(f6, f7)()); function f8(x){ return x; @@ -46,7 +46,7 @@ var compose4 = _.flow; function f9(x){ return x; } - SINK(compose1(f8, f9)(source)); + SINK(lcompose1(f8, f9)(source)); function f10(x){ @@ -58,12 +58,12 @@ var compose4 = _.flow; function f12(x){ return x; } - SINK(compose1(f10, f11, f12)(source)); + SINK(lcompose1(f10, f11, f12)(source)); function f13(x){ return x + 'foo' ; } - SINK(compose1(f13)(source)); + SINK(lcompose1(f13)(source)); function f14(){ return undefined; @@ -76,7 +76,7 @@ var compose4 = _.flow; function f16(){ return undefined; } - SINK(compose1(f15, f16)()); // NO FLOW + SINK(lcompose1(f15, f16)()); // NO FLOW function f17(x, y){ return y; @@ -91,16 +91,21 @@ var compose4 = _.flow; function f19(){ return source; } - SINK(compose2(f19)()); + SINK(rcompose2(f19, o.f)()); function f20(){ return source; } - SINK(compose3(f20)()); + SINK(lcompose3(f16, f20)()); function f21(){ return source; } - SINK(compose4(f21)()); + SINK(lcompose4(f16, f21)()); + + function f22(){ + return source; + } + SINK(lcompose3(f22, f16)()); // NO FLOW })(); From a9adb2912ad14f84f4c19bf538179f0656a14055 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Tue, 27 Oct 2020 15:30:13 +0000 Subject: [PATCH 03/14] JS: Improve lodash model --- .../Security/CWE-94/ServerSideTemplateInjection.ql | 6 ++++++ .../src/semmle/javascript/frameworks/LodashUnderscore.qll | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql b/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql index b4f728b6b15..4e63f4efdbe 100644 --- a/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql +++ b/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql @@ -57,6 +57,12 @@ class SSTINunjucksSink extends ServerSideTemplateInjectionSink { } } +class LodashTemplateSink extends ServerSideTemplateInjectionSink { + LodashTemplateSink() { + this = LodashUnderscore::member("template").getACall().getArgument(0) + } +} + from DataFlow::PathNode source, DataFlow::PathNode sink, ServerSideTemplateInjectionConfiguration c where c.hasFlowPath(source, sink) select sink.getNode(), source, sink, diff --git a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll index 3e2b3b777b0..75066172874 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll @@ -407,7 +407,12 @@ module LodashUnderscore { "shuffle", "sample", "toArray", "partition", "compact", "first", "initial", "last", "rest", "flatten", "without", "difference", "uniq", "unique", "unzip", "transpose", "object", "chunk", "values", "mapObject", "pick", "omit", "defaults", "clone", "tap", - "identity"] and + "identity", + // String category + "camelCase", "capitalize", "deburr", "kebabCase", "lowerCase", "lowerFirst", "pad", + "padEnd", "padStart", "repeat", "replace", "snakeCase", "split", "startCase", "toLower", + "toUpper", "trim", "trimEnd", "trimStart", "truncate", "unescape", "upperCase", + "upperFirst", "words"] and pred = call.getArgument(0) and succ = call or From 42c03ab2fd539f4d52567e0fdb71d0f26b127dea Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Tue, 27 Oct 2020 12:04:15 +0000 Subject: [PATCH 04/14] JS: Add flow steps through dynamic imports --- .../ql/src/semmle/javascript/Promises.qll | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll index 4f730b36f0a..c8e47b4e318 100644 --- a/javascript/ql/src/semmle/javascript/Promises.qll +++ b/javascript/ql/src/semmle/javascript/Promises.qll @@ -641,3 +641,39 @@ private module ClosurePromise { override predicate step(DataFlow::Node src, DataFlow::Node dst) { src = pred and dst = this } } } + +private module DynamicImportSteps { + /** + * A step from an export value to its uses via dynamic imports. + * + * For example: + * ```js + * // foo.js + * export default Foo + * + * // bar.js + * let Foo = await import('./foo'); + * ``` + */ + class DynamicImportStep extends PreCallGraphStep { + override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { + exists(DynamicImportExpr imprt | + pred = imprt.getImportedModule().getAnExportedValue("default") and + succ = imprt.flow() and + prop = Promises::valueProp() + ) + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + // Special-case code like `(await import(x)).Foo` to boost type tracking depth. + exists( + DynamicImportExpr imprt, string name, DataFlow::Node mid, DataFlow::SourceNode awaited + | + pred = imprt.getImportedModule().getAnExportedValue(name) and + mid.getALocalSource() = imprt.flow() and + PromiseFlow::loadStep(mid, awaited, Promises::valueProp()) and + succ = awaited.getAPropertyRead(name) + ) + } + } +} From d116b424f42599019086fdd31f3e0aa7eddb75c2 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Mon, 26 Oct 2020 15:09:53 +0000 Subject: [PATCH 05/14] JS: Add model of react hooks and react-router --- .../semmle/javascript/dataflow/Sources.qll | 3 +- .../javascript/dataflow/TaintTracking.qll | 4 +- .../semmle/javascript/frameworks/React.qll | 227 ++++++++++++++++++ .../ReactJS/higherOrderComponent.jsx | 24 ++ .../frameworks/ReactJS/tests.expected | 6 + .../ReactJS/useHigherOrderComponent.jsx | 18 ++ .../Security/CWE-079/DomBasedXss/Xss.expected | 53 ++++ .../XssWithAdditionalSources.expected | 48 ++++ .../DomBasedXss/react-create-context.js | 3 + .../DomBasedXss/react-provide-context.js | 5 + .../CWE-079/DomBasedXss/react-use-context.js | 11 + .../CWE-079/DomBasedXss/react-use-state.js | 33 +++ 12 files changed, 432 insertions(+), 3 deletions(-) create mode 100644 javascript/ql/test/library-tests/frameworks/ReactJS/higherOrderComponent.jsx create mode 100644 javascript/ql/test/library-tests/frameworks/ReactJS/useHigherOrderComponent.jsx create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-create-context.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-provide-context.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-state.js diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll index c3017585c71..25390f3c7f9 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll @@ -307,7 +307,8 @@ module SourceNode { astNode instanceof FunctionBindExpr or astNode instanceof DynamicImportExpr or astNode instanceof ImportSpecifier or - astNode instanceof ImportMetaExpr + astNode instanceof ImportMetaExpr or + astNode instanceof TaggedTemplateExpr ) or DataFlow::parameterNode(this, _) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 6698800918c..04626e88106 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -327,7 +327,7 @@ module TaintTracking { * taint to flow from `v` to any read of `c2.state.p`, where `c2` * also is an instance of `C`. */ - private class ReactComponentStateTaintStep extends AdditionalTaintStep, DataFlow::ValueNode { + private class ReactComponentStateTaintStep extends AdditionalTaintStep { DataFlow::Node source; ReactComponentStateTaintStep() { @@ -358,7 +358,7 @@ module TaintTracking { * taint to flow from `v` to any read of `c2.props.p`, where `c2` * also is an instance of `C`. */ - private class ReactComponentPropsTaintStep extends AdditionalTaintStep, DataFlow::ValueNode { + private class ReactComponentPropsTaintStep extends AdditionalTaintStep { DataFlow::Node source; ReactComponentPropsTaintStep() { diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index 3cf93056a8c..e854f90d375 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -3,6 +3,8 @@ */ import javascript +private import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps +private import semmle.javascript.dataflow.internal.PreCallGraphStep /** * Gets a reference to the 'React' object. @@ -548,3 +550,228 @@ private class ReactJSXElement extends JSXElement { */ ReactComponent getComponent() { result = component } } + +/** + * Step through the state variable of a `useState` call. + * + * It returns a pair of the current state, and a callback to change the state. + * + * For example: + * ```js + * let [state, setState] = useState(initialValue); + * let [state, setState] = useState(() => initialValue); // lazy initial state + * + * setState(newState); + * setState(prevState => { ... }); + * ``` + */ +private class UseStateStep extends PreCallGraphStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode call | call = react().getAMemberCall("useState") | + pred = + [call.getArgument(0), // initial state + call.getCallback(0).getReturnNode(), // lazy initial state + call.getAPropertyRead("1").getACall().getArgument(0), // setState invocation + call.getAPropertyRead("1").getACall().getCallback(0).getReturnNode()] and // setState with callback + succ = call.getAPropertyRead("0") + or + // Propagate current state into the callback argument of `setState(prevState => { ... })` + pred = call.getAPropertyRead("0") and + succ = call.getAPropertyRead("1").getACall().getCallback(0).getParameter(0) + ) + } +} + +/** + * A step through a React context object. + * + * For example: + * ```js + * let MyContext = React.createContext('foo'); + * + * + * + * + * + * function Foo() { + * let succ = useContext(MyContext); + * } + */ +private class UseContextStep extends PreCallGraphStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode context | + pred = getAContextInput(context) and + succ = getAContextOutput(context) + ) + } +} + +/** + * Gets a data flow node referring to the result of the given `createContext` call. + */ +private DataFlow::SourceNode getAContextRef(DataFlow::CallNode createContext) { + createContext = react().getAMemberCall("createContext") and + result = createContext + or + // Track through imports/exports, but not full type tracking, so this can be used as a PreCallGraphStep. + exists(DataFlow::Node mid | + getAContextRef(createContext).flowsTo(mid) and + FlowSteps::propertyFlowStep(mid, result) + ) +} + +/** + * Gets a data flow node whose value is provided to the given context object. + * + * For example: + * ```jsx + * React.createContext(x); + * + * ``` + */ +pragma[nomagic] +private DataFlow::Node getAContextInput(DataFlow::CallNode createContext) { + result = createContext.getArgument(0) // initial value + or + exists(JSXElement provider | + getAContextRef(createContext) + .getAPropertyRead("Provider") + .flowsTo(provider.getNameExpr().flow()) and + result = provider.getAttributeByName("value").getValue().flow() + ) +} + +/** + * Gets a data flow node whose value is obtained from the given context object. + * + * For example: + * ```js + * let value = useContext(MyContext); + * ``` + */ +pragma[nomagic] +private DataFlow::CallNode getAContextOutput(DataFlow::CallNode createContext) { + result = react().getAMemberCall("useContext") and + getAContextRef(createContext).flowsTo(result.getArgument(0)) + or + exists(DataFlow::ClassNode cls | + getAContextRef(createContext).flowsTo(cls.getAPropertyWrite("contextType").getRhs()) and + result = cls.getAReceiverNode().getAPropertyRead("context") + ) +} + +/** + * A step through a `useMemo` call; for example: + * ```js + * let succ = useMemo(() => pred, []); + * ``` + */ +private class UseMemoStep extends PreCallGraphStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode call | + call = react().getAMemberCall("useMemo") + | + pred = call.getCallback(0).getReturnNode() and + succ = call + ) + } +} + +private DataFlow::SourceNode reactRouterDom() { + result = DataFlow::moduleImport("react-router-dom") +} + +private class ReactRouterSource extends RemoteFlowSource { + ReactRouterSource() { + this = reactRouterDom().getAMemberCall("useParams") + or + this = reactRouterDom().getAMemberCall("useRouteMatch").getAPropertyRead(["params", "url"]) + } + + override string getSourceType() { + result = "react-router path parameters" + } +} + +/** + * Holds if `mod` transitively depends on `react-router-dom`. + * + * We assume any React component in such a file may be used in a context where react-router + * injects the `location` property in its `props` object. + */ +private predicate dependsOnReactRouter(Module mod) { + mod.getAnImport().getImportedPath().getValue() = "react-router-dom" + or + dependsOnReactRouter(mod.getAnImportedModule()) +} + +/** + * A reference to the DOM location obtained through `react-router-dom` + * + * For example: + * ```js + * let location = useLocation(); + * + * function MyComponent(props) { + * props.location; + * } + * export default withRouter(MyComponent); + */ +private class ReactRouterLocationSource extends DOM::LocationSource::Range { + ReactRouterLocationSource() { + this = reactRouterDom().getAMemberCall("useLocation") + or + exists(ReactComponent component | + dependsOnReactRouter(component.getTopLevel()) and + this = component.getAPropRead("location") + ) + } +} + +/** + * Gets a reference to a function which, if called with a React component, returns wrapped + * version of that component, which we model as a direct reference to the underlying component. + */ +private DataFlow::SourceNode higherOrderComponentBuilder() { + result = react().getAPropertyRead("memo") + or + result = DataFlow::moduleMember("react-redux", "connect").getACall() + or + result = reactRouterDom().getAPropertyRead("withRouter") + or + exists(FunctionCompositionCall compose | + higherOrderComponentBuilder().flowsTo(compose.getAnOperandNode()) and + result = compose + ) +} + +private class HigherOrderComponentStep extends PreCallGraphStep { + override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + // `lazy(() => P)` returns a proxy for the component eventually returned by + // the promise P. We model this call as simply returning the value in P. + // It is primarily used for lazy-loading of React components. + exists(DataFlow::CallNode call | + call = react().getAMemberCall("lazy") and + pred = call.getCallback(0).getReturnNode() and + succ = call and + prop = Promises::valueProp() + ) + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + // `memo(f)` returns a function behaves as `f` but caches results + // It is sometimes used to wrap an entire functional component. + exists(DataFlow::CallNode call | + call = higherOrderComponentBuilder().getACall() and + pred = call.getArgument(0) and + succ = call + ) + or + exists(TaggedTemplateExpr expr, DataFlow::CallNode call | + call = DataFlow::moduleImport("styled-components").getACall() and + pred = call.getArgument(0) and + call.flowsTo(expr.getTag().flow()) and + succ = expr.flow() + ) + } +} diff --git a/javascript/ql/test/library-tests/frameworks/ReactJS/higherOrderComponent.jsx b/javascript/ql/test/library-tests/frameworks/ReactJS/higherOrderComponent.jsx new file mode 100644 index 00000000000..62dbe3d85f9 --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/ReactJS/higherOrderComponent.jsx @@ -0,0 +1,24 @@ +import { memo } from 'react'; +import { connect } from 'react-redux'; +import { compose } from 'redux'; +import styled from 'styled-components'; +import unknownFunction from 'somewhere'; + +import { MyComponent } from './exportedComponent'; + +const StyledComponent = styled(MyComponent)` + color: red; +`; + +function mapStateToProps(x) { + return x; +} +function mapDispatchToProps(x) { + return x; +} + +const withConnect = connect(mapStateToProps, mapDispatchToProps); + +const ConnectedComponent = compose(withConnect, unknownFunction)(StyledComponent); + +export default memo(ConnectedComponent); diff --git a/javascript/ql/test/library-tests/frameworks/ReactJS/tests.expected b/javascript/ql/test/library-tests/frameworks/ReactJS/tests.expected index 5f738e573f0..fba8084a937 100644 --- a/javascript/ql/test/library-tests/frameworks/ReactJS/tests.expected +++ b/javascript/ql/test/library-tests/frameworks/ReactJS/tests.expected @@ -227,6 +227,9 @@ test_ReactComponent_getACandidatePropsValue | props.js:30:46:30:67 | "propFr ... tProps" | | props.js:32:22:32:34 | "propFromJSX" | | props.js:34:33:34:53 | "propFr ... ructor" | +| useHigherOrderComponent.jsx:5:33:5:37 | "red" | +| useHigherOrderComponent.jsx:11:39:11:44 | "lazy" | +| useHigherOrderComponent.jsx:17:40:17:46 | "lazy2" | test_ReactComponent | es5.js:1:31:11:1 | {\\n dis ... ;\\n }\\n} | | es5.js:18:33:22:1 | {\\n ren ... ;\\n }\\n} | @@ -285,6 +288,9 @@ test_JSXname | thisAccesses.js:60:19:60:41 | | thisAccesses.js:60:20:60:28 | this.name | this.name | dot | | thisAccesses.js:61:19:61:41 | | thisAccesses.js:61:20:61:28 | this.this | this.this | dot | | thisAccesses_importedMappers.js:13:16:13:21 |
| thisAccesses_importedMappers.js:13:17:13:19 | div | div | Identifier | +| useHigherOrderComponent.jsx:5:12:5:39 | | useHigherOrderComponent.jsx:5:13:5:25 | SomeComponent | SomeComponent | Identifier | +| useHigherOrderComponent.jsx:11:12:11:46 | | useHigherOrderComponent.jsx:11:13:11:31 | LazyLoadedComponent | LazyLoadedComponent | Identifier | +| useHigherOrderComponent.jsx:17:12:17:48 | | useHigherOrderComponent.jsx:17:13:17:32 | LazyLoadedComponent2 | LazyLoadedComponent2 | Identifier | test_JSXName_this | es5.js:4:12:4:45 |
He ... }
| es5.js:4:24:4:27 | this | | es5.js:20:12:20:44 |

Hel ... e}

| es5.js:20:24:20:27 | this | diff --git a/javascript/ql/test/library-tests/frameworks/ReactJS/useHigherOrderComponent.jsx b/javascript/ql/test/library-tests/frameworks/ReactJS/useHigherOrderComponent.jsx new file mode 100644 index 00000000000..a57c5aa70ba --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/ReactJS/useHigherOrderComponent.jsx @@ -0,0 +1,18 @@ +import SomeComponent from './higherOrderComponent'; +import { lazy } from 'react'; + +function foo() { + return +} + +const LazyLoadedComponent = lazy(() => import('./higherOrderComponent')); + +function bar() { + return +} + +const LazyLoadedComponent2 = lazy(() => import('./exportedComponent').then(m => m.MyComponent)); + +function barz() { + return +} diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected index 1cbde553e3a..60ebc15c973 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected @@ -144,6 +144,33 @@ nodes | react-native.js:8:18:8:24 | tainted | | react-native.js:9:27:9:33 | tainted | | react-native.js:9:27:9:33 | tainted | +| react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:10:22:10:32 | window.name | +| react-use-state.js:4:9:4:49 | state | +| react-use-state.js:4:10:4:14 | state | +| react-use-state.js:4:38:4:48 | window.name | +| react-use-state.js:4:38:4:48 | window.name | +| react-use-state.js:5:51:5:55 | state | +| react-use-state.js:5:51:5:55 | state | +| react-use-state.js:9:9:9:43 | state | +| react-use-state.js:9:10:9:14 | state | +| react-use-state.js:10:14:10:24 | window.name | +| react-use-state.js:10:14:10:24 | window.name | +| react-use-state.js:11:51:11:55 | state | +| react-use-state.js:11:51:11:55 | state | +| react-use-state.js:15:9:15:43 | state | +| react-use-state.js:15:10:15:14 | state | +| react-use-state.js:16:20:16:30 | window.name | +| react-use-state.js:16:20:16:30 | window.name | +| react-use-state.js:17:51:17:55 | state | +| react-use-state.js:17:51:17:55 | state | +| react-use-state.js:21:10:21:14 | state | +| react-use-state.js:22:14:22:17 | prev | +| react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:25:20:25:30 | window.name | +| react-use-state.js:25:20:25:30 | window.name | | sanitiser.js:16:7:16:27 | tainted | | sanitiser.js:16:17:16:27 | window.name | | sanitiser.js:16:17:16:27 | window.name | @@ -683,6 +710,27 @@ edges | react-native.js:7:7:7:33 | tainted | react-native.js:9:27:9:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | +| react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | +| react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | +| react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | +| react-use-state.js:4:10:4:14 | state | react-use-state.js:4:9:4:49 | state | +| react-use-state.js:4:38:4:48 | window.name | react-use-state.js:4:10:4:14 | state | +| react-use-state.js:4:38:4:48 | window.name | react-use-state.js:4:10:4:14 | state | +| react-use-state.js:9:9:9:43 | state | react-use-state.js:11:51:11:55 | state | +| react-use-state.js:9:9:9:43 | state | react-use-state.js:11:51:11:55 | state | +| react-use-state.js:9:10:9:14 | state | react-use-state.js:9:9:9:43 | state | +| react-use-state.js:10:14:10:24 | window.name | react-use-state.js:9:10:9:14 | state | +| react-use-state.js:10:14:10:24 | window.name | react-use-state.js:9:10:9:14 | state | +| react-use-state.js:15:9:15:43 | state | react-use-state.js:17:51:17:55 | state | +| react-use-state.js:15:9:15:43 | state | react-use-state.js:17:51:17:55 | state | +| react-use-state.js:15:10:15:14 | state | react-use-state.js:15:9:15:43 | state | +| react-use-state.js:16:20:16:30 | window.name | react-use-state.js:15:10:15:14 | state | +| react-use-state.js:16:20:16:30 | window.name | react-use-state.js:15:10:15:14 | state | +| react-use-state.js:21:10:21:14 | state | react-use-state.js:22:14:22:17 | prev | +| react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | +| react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:23:29:23:35 | tainted | | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:30:29:30:35 | tainted | | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted | @@ -1080,6 +1128,11 @@ edges | optionalSanitizer.js:45:18:45:56 | sanitiz ... target | optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:45:18:45:56 | sanitiz ... target | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:26:16:26:32 | document.location | user-provided value | | react-native.js:8:18:8:24 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:18:8:24 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value | | react-native.js:9:27:9:33 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:9:27:9:33 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value | +| react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | Cross-site scripting vulnerability due to $@. | react-use-context.js:10:22:10:32 | window.name | user-provided value | +| react-use-state.js:5:51:5:55 | state | react-use-state.js:4:38:4:48 | window.name | react-use-state.js:5:51:5:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:4:38:4:48 | window.name | user-provided value | +| react-use-state.js:11:51:11:55 | state | react-use-state.js:10:14:10:24 | window.name | react-use-state.js:11:51:11:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:10:14:10:24 | window.name | user-provided value | +| react-use-state.js:17:51:17:55 | state | react-use-state.js:16:20:16:30 | window.name | react-use-state.js:17:51:17:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:16:20:16:30 | window.name | user-provided value | +| react-use-state.js:23:35:23:38 | prev | react-use-state.js:25:20:25:30 | window.name | react-use-state.js:23:35:23:38 | prev | Cross-site scripting vulnerability due to $@. | react-use-state.js:25:20:25:30 | window.name | user-provided value | | sanitiser.js:23:21:23:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:23:21:23:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | | sanitiser.js:30:21:30:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:30:21:30:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | | sanitiser.js:33:21:33:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:33:21:33:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected index 47a8a8737dc..9ff31455179 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected @@ -144,6 +144,33 @@ nodes | react-native.js:8:18:8:24 | tainted | | react-native.js:9:27:9:33 | tainted | | react-native.js:9:27:9:33 | tainted | +| react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:10:22:10:32 | window.name | +| react-use-state.js:4:9:4:49 | state | +| react-use-state.js:4:10:4:14 | state | +| react-use-state.js:4:38:4:48 | window.name | +| react-use-state.js:4:38:4:48 | window.name | +| react-use-state.js:5:51:5:55 | state | +| react-use-state.js:5:51:5:55 | state | +| react-use-state.js:9:9:9:43 | state | +| react-use-state.js:9:10:9:14 | state | +| react-use-state.js:10:14:10:24 | window.name | +| react-use-state.js:10:14:10:24 | window.name | +| react-use-state.js:11:51:11:55 | state | +| react-use-state.js:11:51:11:55 | state | +| react-use-state.js:15:9:15:43 | state | +| react-use-state.js:15:10:15:14 | state | +| react-use-state.js:16:20:16:30 | window.name | +| react-use-state.js:16:20:16:30 | window.name | +| react-use-state.js:17:51:17:55 | state | +| react-use-state.js:17:51:17:55 | state | +| react-use-state.js:21:10:21:14 | state | +| react-use-state.js:22:14:22:17 | prev | +| react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:25:20:25:30 | window.name | +| react-use-state.js:25:20:25:30 | window.name | | sanitiser.js:16:7:16:27 | tainted | | sanitiser.js:16:17:16:27 | window.name | | sanitiser.js:16:17:16:27 | window.name | @@ -687,6 +714,27 @@ edges | react-native.js:7:7:7:33 | tainted | react-native.js:9:27:9:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | +| react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | +| react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | +| react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | +| react-use-state.js:4:10:4:14 | state | react-use-state.js:4:9:4:49 | state | +| react-use-state.js:4:38:4:48 | window.name | react-use-state.js:4:10:4:14 | state | +| react-use-state.js:4:38:4:48 | window.name | react-use-state.js:4:10:4:14 | state | +| react-use-state.js:9:9:9:43 | state | react-use-state.js:11:51:11:55 | state | +| react-use-state.js:9:9:9:43 | state | react-use-state.js:11:51:11:55 | state | +| react-use-state.js:9:10:9:14 | state | react-use-state.js:9:9:9:43 | state | +| react-use-state.js:10:14:10:24 | window.name | react-use-state.js:9:10:9:14 | state | +| react-use-state.js:10:14:10:24 | window.name | react-use-state.js:9:10:9:14 | state | +| react-use-state.js:15:9:15:43 | state | react-use-state.js:17:51:17:55 | state | +| react-use-state.js:15:9:15:43 | state | react-use-state.js:17:51:17:55 | state | +| react-use-state.js:15:10:15:14 | state | react-use-state.js:15:9:15:43 | state | +| react-use-state.js:16:20:16:30 | window.name | react-use-state.js:15:10:15:14 | state | +| react-use-state.js:16:20:16:30 | window.name | react-use-state.js:15:10:15:14 | state | +| react-use-state.js:21:10:21:14 | state | react-use-state.js:22:14:22:17 | prev | +| react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | +| react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | +| react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:23:29:23:35 | tainted | | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:30:29:30:35 | tainted | | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-create-context.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-create-context.js new file mode 100644 index 00000000000..f07db51c411 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-create-context.js @@ -0,0 +1,3 @@ +import { createContext } from 'react'; + +export let MyContext = createContext({root: null}); diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-provide-context.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-provide-context.js new file mode 100644 index 00000000000..8c2f9bdb557 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-provide-context.js @@ -0,0 +1,5 @@ +import { MyContext } from './react-create-context'; + +export function renderMain() { + return +} diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js new file mode 100644 index 00000000000..819362f44fb --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js @@ -0,0 +1,11 @@ +import { useContext } from 'react'; +import { MyContext } from './react-create-context'; + +function useMyContext() { + return useContext(MyContext); +} + +export function useDoc1() { + let { root } = useMyContext(); + root.appendChild(window.name); // NOT OK +} diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-state.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-state.js new file mode 100644 index 00000000000..672cd3bd689 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-state.js @@ -0,0 +1,33 @@ +import { useState } from 'react'; + +function initialState() { + let [state, setState] = useState(window.name); + return
; // NOT OK +} + +function setStateValue() { + let [state, setState] = useState('foo'); + setState(window.name); + return
; // NOT OK +} + +function setStateValueLazy() { + let [state, setState] = useState('foo'); + setState(() => window.name); + return
; // NOT OK +} + +function setStateValueLazy() { + let [state, setState] = useState('foo'); + setState(prev => { + document.body.innerHTML = prev; // NOT OK + }) + setState(() => window.name); +} + +function setStateValueSafe() { + let [state, setState] = useState('foo'); + setState('safe'); + setState(() => 'also safe'); + return
; // OK +} From c03532edfb861bebf961ca119cf910c40c2420c3 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 28 Oct 2020 11:06:13 +0000 Subject: [PATCH 06/14] JS: Change note --- change-notes/1.26/analysis-javascript.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 9d6f2af5970..90e1a6b7032 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -4,6 +4,10 @@ * Angular-specific taint sources and sinks are now recognized by the security queries. +* Support for React has improved, with better handling of react hooks, react-router path parameters, lazy-loaded components, and components transformed using `react-redux` and/or `styled-components`. + +* Dynamic imports are now analyzed more precisely. + * Support for the following frameworks and libraries has been improved: - [@angular/*](https://www.npmjs.com/package/@angular/core) - [AWS Serverless](https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-resource-function.html) @@ -27,7 +31,11 @@ - [needle](https://www.npmjs.com/package/needle) - [object-inspect](https://www.npmjs.com/package/object-inspect) - [pretty-format](https://www.npmjs.com/package/pretty-format) + - [react](https://www.npmjs.com/package/react) + - [react-router-dom](https://www.npmjs.com/package/react-router-dom) + - [react-redux](https://www.npmjs.com/package/react-redux) - [stringify-object](https://www.npmjs.com/package/stringify-object) + - [styled-components](https://www.npmjs.com/package/styled-components) - [throttle-debounce](https://www.npmjs.com/package/throttle-debounce) - [underscore](https://www.npmjs.com/package/underscore) From 7a3f0095f6aac4105ef66ad25a60183f4e467019 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 28 Oct 2020 11:49:36 +0000 Subject: [PATCH 07/14] JS: Autoformat --- .../Security/CWE-94/ServerSideTemplateInjection.ql | 4 +--- javascript/ql/src/semmle/javascript/Promises.qll | 2 +- .../semmle/javascript/frameworks/ComposedFunctions.qll | 6 ++---- .../ql/src/semmle/javascript/frameworks/React.qll | 10 +++------- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql b/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql index 4e63f4efdbe..9edce4d1932 100644 --- a/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql +++ b/javascript/ql/src/experimental/Security/CWE-94/ServerSideTemplateInjection.ql @@ -58,9 +58,7 @@ class SSTINunjucksSink extends ServerSideTemplateInjectionSink { } class LodashTemplateSink extends ServerSideTemplateInjectionSink { - LodashTemplateSink() { - this = LodashUnderscore::member("template").getACall().getArgument(0) - } + LodashTemplateSink() { this = LodashUnderscore::member("template").getACall().getArgument(0) } } from DataFlow::PathNode source, DataFlow::PathNode sink, ServerSideTemplateInjectionConfiguration c diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll index c8e47b4e318..650ed280b5e 100644 --- a/javascript/ql/src/semmle/javascript/Promises.qll +++ b/javascript/ql/src/semmle/javascript/Promises.qll @@ -650,7 +650,7 @@ private module DynamicImportSteps { * ```js * // foo.js * export default Foo - * + * * // bar.js * let Foo = await import('./foo'); * ``` diff --git a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll index 9427581b9b8..7d1c3998961 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll @@ -58,7 +58,7 @@ module FunctionCompositionCall { * * For simplicity, we model every composition function as if it supported this. */ - private abstract class WithArrayOverloading extends Range { + abstract private class WithArrayOverloading extends Range { /** Gets the `i`th argument to the call or the `i`th array element passed into the call. */ DataFlow::Node getEffectiveArgument(int i) { result = getArgument(0).(DataFlow::ArrayCreationNode).getElement(i) @@ -85,9 +85,7 @@ module FunctionCompositionCall { this = LodashUnderscore::member("flowRight").getACall() } - override DataFlow::Node getOperandNode(int i) { - result = getEffectiveArgument(i) - } + override DataFlow::Node getOperandNode(int i) { result = getEffectiveArgument(i) } } /** A call whose arguments are functions `f,g,h` which are composed into `f(g(h(...))` */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index e854f90d375..38130de15a6 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -668,9 +668,7 @@ private DataFlow::CallNode getAContextOutput(DataFlow::CallNode createContext) { */ private class UseMemoStep extends PreCallGraphStep { override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(DataFlow::CallNode call | - call = react().getAMemberCall("useMemo") - | + exists(DataFlow::CallNode call | call = react().getAMemberCall("useMemo") | pred = call.getCallback(0).getReturnNode() and succ = call ) @@ -688,9 +686,7 @@ private class ReactRouterSource extends RemoteFlowSource { this = reactRouterDom().getAMemberCall("useRouteMatch").getAPropertyRead(["params", "url"]) } - override string getSourceType() { - result = "react-router path parameters" - } + override string getSourceType() { result = "react-router path parameters" } } /** @@ -711,7 +707,7 @@ private predicate dependsOnReactRouter(Module mod) { * For example: * ```js * let location = useLocation(); - * + * * function MyComponent(props) { * props.location; * } From 7ee3846142be1685c9b1f1b63e65f749bf298e82 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 28 Oct 2020 12:43:48 +0000 Subject: [PATCH 08/14] JS: Add missing qldoc --- .../semmle/javascript/frameworks/ComposedFunctions.qll | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll index 7d1c3998961..9fc6262e999 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll @@ -41,7 +41,15 @@ class FunctionCompositionCall extends DataFlow::CallNode { int getNumOperand() { result = range.getNumOperand() } } +/** + * Companion module to the `FunctionCompositionCall` class. + */ module FunctionCompositionCall { + /** + * Class that determines the set of values in `FunctionCompositionCall`. + * + * May be subclassed to classify more calls as function compositions. + */ abstract class Range extends DataFlow::CallNode { /** * Gets the function flowing into the `i`th function in the composition `f(g(h(...)))`. From f99db23e7b100faedcdda75df14b4ba844dac9a5 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Wed, 28 Oct 2020 16:23:36 +0000 Subject: [PATCH 09/14] JS: Add test and fix for contextType --- .../ql/src/semmle/javascript/frameworks/React.qll | 9 ++++++--- .../Security/CWE-079/DomBasedXss/Xss.expected | 5 +++++ .../DomBasedXss/XssWithAdditionalSources.expected | 4 ++++ .../Security/CWE-079/DomBasedXss/react-use-context.js | 11 ++++++++++- 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index 38130de15a6..c7c8ccf7cb4 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -650,9 +650,12 @@ private DataFlow::Node getAContextInput(DataFlow::CallNode createContext) { * ``` */ pragma[nomagic] -private DataFlow::CallNode getAContextOutput(DataFlow::CallNode createContext) { - result = react().getAMemberCall("useContext") and - getAContextRef(createContext).flowsTo(result.getArgument(0)) +private DataFlow::SourceNode getAContextOutput(DataFlow::CallNode createContext) { + exists(DataFlow::CallNode call | + call = react().getAMemberCall("useContext") and + getAContextRef(createContext).flowsTo(call.getArgument(0)) and + result = call + ) or exists(DataFlow::ClassNode cls | getAContextRef(createContext).flowsTo(cls.getAPropertyWrite("contextType").getRhs()) and diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected index 60ebc15c973..55a590cc489 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected @@ -147,6 +147,9 @@ nodes | react-use-context.js:10:22:10:32 | window.name | | react-use-context.js:10:22:10:32 | window.name | | react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:16:26:16:36 | window.name | +| react-use-context.js:16:26:16:36 | window.name | +| react-use-context.js:16:26:16:36 | window.name | | react-use-state.js:4:9:4:49 | state | | react-use-state.js:4:10:4:14 | state | | react-use-state.js:4:38:4:48 | window.name | @@ -711,6 +714,7 @@ edges | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:16:26:16:36 | window.name | react-use-context.js:16:26:16:36 | window.name | | react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | | react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | | react-use-state.js:4:10:4:14 | state | react-use-state.js:4:9:4:49 | state | @@ -1129,6 +1133,7 @@ edges | react-native.js:8:18:8:24 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:18:8:24 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value | | react-native.js:9:27:9:33 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:9:27:9:33 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value | | react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | Cross-site scripting vulnerability due to $@. | react-use-context.js:10:22:10:32 | window.name | user-provided value | +| react-use-context.js:16:26:16:36 | window.name | react-use-context.js:16:26:16:36 | window.name | react-use-context.js:16:26:16:36 | window.name | Cross-site scripting vulnerability due to $@. | react-use-context.js:16:26:16:36 | window.name | user-provided value | | react-use-state.js:5:51:5:55 | state | react-use-state.js:4:38:4:48 | window.name | react-use-state.js:5:51:5:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:4:38:4:48 | window.name | user-provided value | | react-use-state.js:11:51:11:55 | state | react-use-state.js:10:14:10:24 | window.name | react-use-state.js:11:51:11:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:10:14:10:24 | window.name | user-provided value | | react-use-state.js:17:51:17:55 | state | react-use-state.js:16:20:16:30 | window.name | react-use-state.js:17:51:17:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:16:20:16:30 | window.name | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected index 9ff31455179..c9a7a244e85 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected @@ -147,6 +147,9 @@ nodes | react-use-context.js:10:22:10:32 | window.name | | react-use-context.js:10:22:10:32 | window.name | | react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:16:26:16:36 | window.name | +| react-use-context.js:16:26:16:36 | window.name | +| react-use-context.js:16:26:16:36 | window.name | | react-use-state.js:4:9:4:49 | state | | react-use-state.js:4:10:4:14 | state | | react-use-state.js:4:38:4:48 | window.name | @@ -715,6 +718,7 @@ edges | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-use-context.js:10:22:10:32 | window.name | react-use-context.js:10:22:10:32 | window.name | +| react-use-context.js:16:26:16:36 | window.name | react-use-context.js:16:26:16:36 | window.name | | react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | | react-use-state.js:4:9:4:49 | state | react-use-state.js:5:51:5:55 | state | | react-use-state.js:4:10:4:14 | state | react-use-state.js:4:9:4:49 | state | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js index 819362f44fb..6d7e20ec6eb 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/react-use-context.js @@ -1,4 +1,4 @@ -import { useContext } from 'react'; +import { useContext, Component } from 'react'; import { MyContext } from './react-create-context'; function useMyContext() { @@ -9,3 +9,12 @@ export function useDoc1() { let { root } = useMyContext(); root.appendChild(window.name); // NOT OK } + +class C extends Component { + foo() { + let { root } = this.context; + root.appendChild(window.name); // NOT OK + } +} + +C.contextType = MyContext; From 43336ff5a90fa8181a691dfa5cffb04758f1f8fb Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 28 Oct 2020 16:29:07 +0000 Subject: [PATCH 10/14] Update change-notes/1.26/analysis-javascript.md Co-authored-by: Erik Krogh Kristensen --- change-notes/1.26/analysis-javascript.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 90e1a6b7032..4ea2e3767b5 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -34,6 +34,8 @@ - [react](https://www.npmjs.com/package/react) - [react-router-dom](https://www.npmjs.com/package/react-router-dom) - [react-redux](https://www.npmjs.com/package/react-redux) + - [redis](https://www.npmjs.com/package/redis) + - [redux](https://www.npmjs.com/package/redux) - [stringify-object](https://www.npmjs.com/package/stringify-object) - [styled-components](https://www.npmjs.com/package/styled-components) - [throttle-debounce](https://www.npmjs.com/package/throttle-debounce) From 581441d5855448302d38049f2b6360913eb35d76 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 28 Oct 2020 16:29:15 +0000 Subject: [PATCH 11/14] Update javascript/ql/src/semmle/javascript/frameworks/React.qll Co-authored-by: Erik Krogh Kristensen --- javascript/ql/src/semmle/javascript/frameworks/React.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index c7c8ccf7cb4..204f53b4f03 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -596,6 +596,7 @@ private class UseStateStep extends PreCallGraphStep { * function Foo() { * let succ = useContext(MyContext); * } + * ``` */ private class UseContextStep extends PreCallGraphStep { override predicate step(DataFlow::Node pred, DataFlow::Node succ) { From a4a6e3beb3bcc68a0d9c3dfe27d041fb5e5a89fc Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 29 Oct 2020 10:38:09 +0000 Subject: [PATCH 12/14] JS: Update dataflow tests --- javascript/ql/test/library-tests/DataFlow/tests.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/test/library-tests/DataFlow/tests.expected b/javascript/ql/test/library-tests/DataFlow/tests.expected index 26d212bcf6f..b65daf255c0 100644 --- a/javascript/ql/test/library-tests/DataFlow/tests.expected +++ b/javascript/ql/test/library-tests/DataFlow/tests.expected @@ -1508,6 +1508,7 @@ sources | tst.js:50:14:53:3 | return of constructor of class A | | tst.js:51:5:51:13 | super(42) | | tst.js:58:1:58:3 | tag | +| tst.js:58:1:58:13 | tag `x: ${x}` | | tst.js:61:1:61:5 | ::o.m | | tst.js:61:3:61:5 | o.m | | tst.js:62:1:62:4 | o::g | From 6ab7846e81954a2f5dc58ac4505c51b9a024fdba Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 30 Oct 2020 09:28:06 +0000 Subject: [PATCH 13/14] JS: Restrict getAContextInput --- javascript/ql/src/semmle/javascript/frameworks/React.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index 204f53b4f03..cb7c21b3440 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -632,6 +632,7 @@ private DataFlow::SourceNode getAContextRef(DataFlow::CallNode createContext) { */ pragma[nomagic] private DataFlow::Node getAContextInput(DataFlow::CallNode createContext) { + createContext = react().getAMemberCall("createContext") and result = createContext.getArgument(0) // initial value or exists(JSXElement provider | From c7667d372e63c97bc8d9a5be751f437206181c15 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 30 Oct 2020 16:25:30 +0000 Subject: [PATCH 14/14] JS: Address review comments --- .../semmle/javascript/frameworks/ComposedFunctions.qll | 10 +++++----- .../ql/src/semmle/javascript/frameworks/React.qll | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll index 9fc6262e999..df0b3ce73e3 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll @@ -124,11 +124,6 @@ private class ComposedFunctionTaintStep extends TaintTracking::AdditionalTaintSt override predicate step(DataFlow::Node pred, DataFlow::Node succ) { exists(int fnIndex, DataFlow::FunctionNode fn | fn = composed.getOperandFunction(fnIndex) | - // flow out of the composed call - fnIndex = 0 and - pred = fn.getReturnNode() and - succ = this - or // flow into the first function fnIndex = composed.getNumOperand() - 1 and exists(int callArgIndex | @@ -141,6 +136,11 @@ private class ComposedFunctionTaintStep extends TaintTracking::AdditionalTaintSt pred = predFn.getReturnNode() and succ = fn.getParameter(0) ) + or + // flow out of the composed call + fnIndex = 0 and + pred = fn.getReturnNode() and + succ = this ) } } diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index cb7c21b3440..cfc77682291 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -734,6 +734,8 @@ private class ReactRouterLocationSource extends DOM::LocationSource::Range { * version of that component, which we model as a direct reference to the underlying component. */ private DataFlow::SourceNode higherOrderComponentBuilder() { + // `memo(f)` returns a function that behaves as `f` but caches results + // It is sometimes used to wrap an entire functional component. result = react().getAPropertyRead("memo") or result = DataFlow::moduleMember("react-redux", "connect").getACall() @@ -760,8 +762,6 @@ private class HigherOrderComponentStep extends PreCallGraphStep { } override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - // `memo(f)` returns a function behaves as `f` but caches results - // It is sometimes used to wrap an entire functional component. exists(DataFlow::CallNode call | call = higherOrderComponentBuilder().getACall() and pred = call.getArgument(0) and