diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowImplConsistency.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowImplConsistency.qll index 5b166b89119..aa892c810db 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowImplConsistency.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowImplConsistency.qll @@ -24,6 +24,8 @@ private module ConsistencyConfig implements InputSig { or n instanceof FlowSummaryIntermediateAwaitStoreNode or + n instanceof FlowSummaryDynamicParameterArrayNode + or n instanceof GenericSynthesizedNode or n = DataFlow::globalAccessPathRootPseudoNode() @@ -43,6 +45,10 @@ private module ConsistencyConfig implements InputSig { node instanceof TStaticArgumentArrayNode or node instanceof TDynamicArgumentArrayNode } + + predicate reverseReadExclude(DataFlow::Node node) { + node instanceof FlowSummaryDynamicParameterArrayNode + } } module Consistency = MakeConsistency; diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll index 7db6e3a3c71..59e0b65e58f 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll @@ -80,6 +80,7 @@ private module Cached { TConstructorThisArgumentNode(InvokeExpr e) { e instanceof NewExpr or e instanceof SuperCall } or TConstructorThisPostUpdate(Constructor ctor) or TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or + TFlowSummaryDynamicParameterArrayNode(FlowSummaryImpl::Public::SummarizedCallable callable) or TFlowSummaryIntermediateAwaitStoreNode(FlowSummaryImpl::Private::SummaryNode sn) { // NOTE: This dependency goes through the 'Steps' module whose instantiation depends on the call graph, // but the specific predicate we're referering to does not use that information. diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index 99929776e71..fe7027fe705 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -30,6 +30,19 @@ class FlowSummaryNode extends DataFlow::Node, TFlowSummaryNode { override string toString() { result = this.getSummaryNode().toString() } } +class FlowSummaryDynamicParameterArrayNode extends DataFlow::Node, + TFlowSummaryDynamicParameterArrayNode +{ + private FlowSummaryImpl::Public::SummarizedCallable callable; + + FlowSummaryDynamicParameterArrayNode() { this = TFlowSummaryDynamicParameterArrayNode(callable) } + + FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = callable } + + cached + override string toString() { result = "[dynamic parameter array] " + callable } +} + class FlowSummaryIntermediateAwaitStoreNode extends DataFlow::Node, TFlowSummaryIntermediateAwaitStoreNode { @@ -342,6 +355,12 @@ private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosit FlowSummaryImpl::Private::summaryParameterNode(summaryNode.getSummaryNode(), pos) and c.asLibraryCallable() = summaryNode.getSummarizedCallable() ) + or + exists(FlowSummaryImpl::Public::SummarizedCallable callable | + c.asLibraryCallable() = callable and + pos.isDynamicArgumentArray() and + p = TFlowSummaryDynamicParameterArrayNode(callable) + ) } predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition pos) { @@ -410,6 +429,8 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) { or result.asLibraryCallable() = node.(FlowSummaryNode).getSummarizedCallable() or + result.asLibraryCallable() = node.(FlowSummaryDynamicParameterArrayNode).getSummarizedCallable() + or result.asLibraryCallable() = node.(FlowSummaryIntermediateAwaitStoreNode).getSummarizedCallable() or node = TGenericSynthesizedNode(_, _, result) @@ -1118,6 +1139,17 @@ predicate readStep(Node node1, ContentSet c, Node node2) { storeContent.isUnknownArrayElement() else storeContent.asArrayIndex() = n + c.asArrayIndex() ) + or + exists(FlowSummaryNode parameter, ParameterPosition pos | + FlowSummaryImpl::Private::summaryParameterNode(parameter.getSummaryNode(), pos) and + node1 = TFlowSummaryDynamicParameterArrayNode(parameter.getSummarizedCallable()) and + node2 = parameter and + ( + c.asArrayIndex() = pos.asPositional() + or + c = ContentSet::arrayElementLowerBound(pos.asPositionalLowerBound()) + ) + ) } /** Gets the post-update node for which `node` is the corresponding pre-update node. */ diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll index ca38d30e901..3611d34667e 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll @@ -35,8 +35,6 @@ private predicate positionName(ParameterPosition pos, string operand) { or pos.isFunctionSelfReference() and operand = "function" or - pos.isDynamicArgumentArray() and operand = "arguments-array" // TODO: remove and handle automatically - or operand = pos.asPositionalLowerBound() + ".." } diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll index 8ab74e217b1..0723cbf3767 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll @@ -101,17 +101,11 @@ class ArrayConstructorSummary extends SummarizedCallable { override predicate propagatesFlowExt(string input, string output, boolean preservesValue) { preservesValue = true and - ( - input = "Argument[0..]" and - output = "ReturnValue.ArrayElement" - or - input = "Argument[arguments-array].WithArrayElement" and - output = "ReturnValue" - ) + input = "Argument[0..]" and + output = "ReturnValue.ArrayElement" or - // TODO: workaround for WithArrayElement not being converted to a taint step preservesValue = false and - input = "Argument[arguments-array]" and + input = "Argument[0..]" and output = "ReturnValue" } } @@ -437,8 +431,7 @@ class PushLike extends SummarizedCallable { override predicate propagatesFlowExt(string input, string output, boolean preservesValue) { preservesValue = true and - // TODO: make it so `arguments-array` is handled without needing to reference it explicitly in every flow-summary - input = ["Argument[0..]", "Argument[arguments-array].ArrayElement"] and + input = "Argument[0..]" and output = "Argument[this].ArrayElement" } } diff --git a/javascript/ql/test/library-tests/FlowSummary/test.expected b/javascript/ql/test/library-tests/FlowSummary/test.expected index 28b528767d8..e69de29bb2d 100644 --- a/javascript/ql/test/library-tests/FlowSummary/test.expected +++ b/javascript/ql/test/library-tests/FlowSummary/test.expected @@ -1,7 +0,0 @@ -| library-tests/FlowSummary/tst.js:278 | expected an alert, but found none | NOT OK | ConsistencyConfig | -| library-tests/FlowSummary/tst.js:282 | expected an alert, but found none | NOT OK | ConsistencyConfig | -| library-tests/FlowSummary/tst.js:283 | expected an alert, but found none | NOT OK | ConsistencyConfig | -| library-tests/FlowSummary/tst.js:286 | expected an alert, but found none | NOT OK | ConsistencyConfig | -| library-tests/FlowSummary/tst.js:287 | expected an alert, but found none | NOT OK | ConsistencyConfig | -| library-tests/FlowSummary/tst.js:290 | expected an alert, but found none | NOT OK | ConsistencyConfig | -| library-tests/FlowSummary/tst.js:291 | expected an alert, but found none | NOT OK | ConsistencyConfig | diff --git a/javascript/ql/test/library-tests/FlowSummary/tst.js b/javascript/ql/test/library-tests/FlowSummary/tst.js index ac0ffb0003e..264c304f0f3 100644 --- a/javascript/ql/test/library-tests/FlowSummary/tst.js +++ b/javascript/ql/test/library-tests/FlowSummary/tst.js @@ -275,19 +275,27 @@ function m18() { const dynamicParam0 = mkSummary("Argument[0..]", "ReturnValue"); const dynamicParam1 = mkSummary("Argument[1..]", "ReturnValue"); - sink(staticParam0(...source())); // NOT OK - sink(staticParam0("safe", ...source())); // OK + sink(staticParam0(...[source()])); // NOT OK + sink(staticParam0(...["safe", source()])); // OK + sink(staticParam0(...[source(), "safe", ])); // NOT OK + sink(staticParam0("safe", ...[source()])); // OK sink(staticParam0(source(), ...["safe"])); // NOT OK - sink(staticParam1(...source())); // NOT OK - sink(staticParam1("safe", ...source())); // NOT OK + sink(staticParam1(...[source()])); // OK + sink(staticParam1(...["safe", source()])); // NOT OK + sink(staticParam1(...[source(), "safe", ])); // OK + sink(staticParam1("safe", ...[source()])); // NOT OK sink(staticParam1(source(), ...["safe"])); // OK - sink(dynamicParam0(...source())); // NOT OK - sink(dynamicParam0("safe", ...source())); // NOT OK + sink(dynamicParam0(...[source()])); // NOT OK + sink(dynamicParam0(...["safe", source()])); // NOT OK + sink(dynamicParam0(...[source(), "safe", ])); // NOT OK + sink(dynamicParam0("safe", ...[source()])); // NOT OK sink(dynamicParam0(source(), ...["safe"])); // NOT OK - sink(dynamicParam1(...source())); // NOT OK - sink(dynamicParam1("safe", ...source())); // NOT OK + sink(dynamicParam1(...[source()])); // OK + sink(dynamicParam1(...["safe", source()])); // NOT OK + sink(dynamicParam1(...[source(), "safe", ])); // OK + sink(dynamicParam1("safe", ...[source()])); // NOT OK sink(dynamicParam1(source(), ...["safe"])); // OK }