From d91e6a48932e31d8c50ab5d4a1a665532e0ff6e2 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 25 Feb 2020 10:55:23 +0000 Subject: [PATCH 1/4] JavaScript: Avoid a few bad join orders. --- .../javascript/dataflow/Configuration.qll | 51 +++++++++++++++---- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 75ad502ad9a8..e9a153b42563 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -945,18 +945,31 @@ private predicate reachableFromStoreBase( s2.getEndLabel()) ) or - exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary | - reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and - ( - flowStep(mid, cfg, nd, newSummary) - or - isAdditionalLoadStoreStep(mid, nd, prop, cfg) and - newSummary = PathSummary::level() - ) and + exists(PathSummary oldSummary, PathSummary newSummary | + reachableFromStoreBaseStep(prop, rhs, nd, cfg, oldSummary, newSummary) and summary = oldSummary.appendValuePreserving(newSummary) ) } +/** + * Holds if `rhs` is the right-hand side of a write to property `prop`, and `nd` is reachable + * from the base of that write under configuration `cfg` (possibly through callees) along a + * path whose last step is summarized by `newSummary`, and the previous steps are summarized + * by `oldSummary`. + */ +pragma[noinline] +private predicate reachableFromStoreBaseStep( + string prop, DataFlow::Node rhs, DataFlow::Node nd, DataFlow::Configuration cfg, + PathSummary oldSummary, PathSummary newSummary +) { + exists(DataFlow::Node mid | reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) | + flowStep(mid, cfg, nd, newSummary) + or + isAdditionalLoadStoreStep(mid, nd, prop, cfg) and + newSummary = PathSummary::level() + ) +} + /** * Holds if the value of `pred` is written to a property of some base object, and that base * object may flow into the base of property read `succ` under configuration `cfg` along @@ -968,13 +981,29 @@ pragma[noinline] private predicate flowThroughProperty( DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary summary ) { - exists(string prop, DataFlow::Node base, PathSummary oldSummary, PathSummary newSummary | - reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and - loadStep(base, succ, prop, cfg, newSummary) and + exists(PathSummary oldSummary, PathSummary newSummary | + storeToLoad(pred, succ, cfg, oldSummary, newSummary) and summary = oldSummary.append(newSummary) ) } +/** + * Holds if the value of `pred` is written to a property of some base object, and that base + * object may flow into the base of property read `succ` under configuration `cfg` along + * a path whose last step is summarized by `newSummary`, and the previous steps are summarized + * by `oldSummary`. + */ +pragma[noinline] +private predicate storeToLoad( + DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary oldSummary, + PathSummary newSummary +) { + exists(string prop, DataFlow::Node base | + reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and + loadStep(base, succ, prop, cfg, newSummary) + ) +} + /** * Holds if `arg` and `cb` are passed as arguments to a function which in turn * invokes `cb`, passing `arg` as its `i`th argument. From ee62706ad2161201bfcd707b9ec0015fba728797 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 18 Mar 2020 17:39:51 +0000 Subject: [PATCH 2/4] JavaScript: Split up a predicate to avoid bad join order. --- javascript/ql/src/semmle/javascript/dataflow/Nodes.qll | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll index c988bc9362e4..f1914e3986df 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll @@ -157,10 +157,12 @@ class InvokeNode extends DataFlow::SourceNode { * `name` is set to `result`. */ DataFlow::ValueNode getOptionArgument(int i, string name) { - exists(ObjectLiteralNode obj | - obj.flowsTo(getArgument(i)) and - obj.hasPropertyWrite(name, result) - ) + getOptionsArgument(i).hasPropertyWrite(name, result) + } + + pragma[noinline] + private ObjectLiteralNode getOptionsArgument(int i) { + result.flowsTo(getArgument(i)) } /** Gets an abstract value representing possible callees of this call site. */ From b13e6141a28ba359a91a53da96803f3863b1fe16 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 23 Mar 2020 11:56:50 +0000 Subject: [PATCH 3/4] JavaScript: Inline `promiseStep/4`. --- javascript/ql/src/semmle/javascript/Promises.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll index 867fab13d66e..6efa26143587 100644 --- a/javascript/ql/src/semmle/javascript/Promises.qll +++ b/javascript/ql/src/semmle/javascript/Promises.qll @@ -184,6 +184,7 @@ module PromiseTypeTracking { * Gets the result from a single step through a promise, from `pred` with tracker `t2` to `result` with tracker `t`. * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another. */ + pragma[inline] DataFlow::SourceNode promiseStep( DataFlow::SourceNode pred, DataFlow::TypeTracker t, DataFlow::TypeTracker t2 ) { From 55e7b22cdfdb8ea975f0c5afaa873ebef7195a46 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 23 Mar 2020 14:37:04 +0000 Subject: [PATCH 4/4] JavaScript: Autoformat. --- javascript/ql/src/semmle/javascript/dataflow/Nodes.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll index f1914e3986df..961ac6d6a904 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll @@ -161,9 +161,7 @@ class InvokeNode extends DataFlow::SourceNode { } pragma[noinline] - private ObjectLiteralNode getOptionsArgument(int i) { - result.flowsTo(getArgument(i)) - } + private ObjectLiteralNode getOptionsArgument(int i) { result.flowsTo(getArgument(i)) } /** Gets an abstract value representing possible callees of this call site. */ final AbstractValue getACalleeValue() { result = getCalleeNode().analyze().getAValue() }