From d0d650654d0fef1a262d1a217828acf6829a9f82 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Tue, 21 Dec 2021 16:44:34 -0800 Subject: [PATCH 1/8] JS: Improve performance of ClassifyFiles::isTestFile One of the heuristics for test files looks for source files of the form `base.ext`, then looks for sibling test files of the form `base.test.ext` or `base.spec.ext`. On large databases, the result join order computed all source files, the containers of those files, then all other files within those containers, before computing the test file names and filtering using those names. The product of all files with all other files in the same containers is of the same order of magnitude as the product of the `files` table with itself, which on large DBs like Node can be 12M+ tuples. As a performance optimisation, factor out a helper predicate that computes the likely test file names for each source file, so these can be determined with a single join against the files table. This results in much better join orders, such as computing the set of files and their containers, then the test file names, then the sibling files with those names. This loses some flexibility because the set of 'test' extension names is hardcoded in the library rather than provided by the caller predicate. The original predicate remains to avoid breaking other callers, but could eventually be deprecated. --- .../javascript/filters/ClassifyFiles.qll | 4 +-- .../semmle/javascript/frameworks/Testing.qll | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll b/javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll index aa3aa81bffe8..45a132e088a2 100644 --- a/javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll +++ b/javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll @@ -56,9 +56,7 @@ predicate isGeneratedCodeFile(File f) { isGenerated(f.getATopLevel()) } predicate isTestFile(File f) { exists(Test t | t.getFile() = f) or - exists(string stemExt | stemExt = "test" or stemExt = "spec" | - f = getTestFile(any(File orig), stemExt) - ) + f = getATestFile(_) or f.getAbsolutePath().regexpMatch(".*/__(mocks|tests)__/.*") } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Testing.qll b/javascript/ql/lib/semmle/javascript/frameworks/Testing.qll index fb2d85523d4f..262d90e84994 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Testing.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Testing.qll @@ -40,7 +40,7 @@ class BDDTest extends Test, @call_expr { /** * Gets the test file for `f` with stem extension `stemExt`. - * That is, a file named file named `..` in the + * That is, a file named `..` in the * same directory as `f` which is named `.`. */ bindingset[stemExt] @@ -48,6 +48,33 @@ File getTestFile(File f, string stemExt) { result = f.getParentContainer().getFile(f.getStem() + "." + stemExt + "." + f.getExtension()) } +/** + * Gets a test file for `f`. + * That is, a file named `..` in the + * same directory as `f`, where `f` is named `.` and + * `` is a well-known test file identifier, such as `test` or `spec`. + */ +File getATestFile(File f) { + result = f.getParentContainer().getFile(getATestFileName(f)) +} + +/** + * Gets a name of a test file for `f`. + * That is, `..` where + * `f` is named `.` and `` is + * a well-known test file identifier, such as `test` or `spec`. + */ +// Helper predicate factored out for performance. +// This predicate is linear in the size of f, and forces +// callers to join only once against f rather than two separate joins +// when computing the stem and the extension. +// This loses some flexibility because callers cannot specify +// an arbitrary stemExt. +pragma[nomagic] +private string getATestFileName(File f) { + result = f.getStem() + "." + ["test", "spec"] + "." + f.getExtension() +} + /** * A Jest test, that is, an invocation of a global function named * `test` where the first argument is a string and the second From 103c8edfb91149a65402203a130a584ecef9affd Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Tue, 21 Dec 2021 19:57:06 -0800 Subject: [PATCH 2/8] JS: Improve performance of Xss::isOptionallySanitizedEdge When join-ordering and evaluating this conjunction, it is preferable to start with the relatively small set of `sanitizer` calls, then compute the set of SSA variables accessed as the arguments of those sanitizer calls, then reason about how those variables are used in phi nodes. Use directional binding pragmas to encourage this join order by picking `sanitizer` first, and discourage picking the opposite join order starting with `phi`. This impacts performance of the ATM XSS queries on large databases like Node, where computing all variable accesses from phi nodes leads to 435M+ tuples. --- .../javascript/security/dataflow/Xss.qll | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll index 501eed347c55..2f27bb69b9bf 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll @@ -437,8 +437,27 @@ module DomBasedXss { b = phi.getAnInput().getDefinition() and count(phi.getAnInput()) = 2 and not a = b and - sanitizer = DataFlow::valueNode(a.getDef().getSource()) and - sanitizer.getAnArgument().asExpr().(VarAccess).getVariable() = b.getSourceVariable() + /* + * Performance optimisation: + * + * When join-ordering and evaluating this conjunction, + * it is preferable to start with the relatively small set of + * `sanitizer` calls, then compute the set of SSA variables accessed + * as the arguments of those sanitizer calls, then reason about how + * those variables are used in phi nodes. + * + * Use directional binding pragmas to encourage this join order, + * starting with `sanitizer`. + * + * Without these pragmas, the join orderer may choose the opposite order: + * start with all `phi` nodes, then compute the set of SSA variables involved, + * then the (potentially large) set of accesses to those variables, + * then the set of accesses used as the argument of a sanitizer call. + */ + + pragma[only_bind_out](sanitizer) = DataFlow::valueNode(a.getDef().getSource()) and + pragma[only_bind_out](sanitizer.getAnArgument().asExpr()) = + b.getSourceVariable().getAnAccess() | pred = DataFlow::ssaDefinitionNode(b) and succ = DataFlow::ssaDefinitionNode(phi) From eab318c2ef98722fddc4521a96e63b89d16a5936 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Wed, 22 Dec 2021 12:07:07 -0800 Subject: [PATCH 3/8] JS: Add `this` reference for clarity No behaviour change. --- .../javascript/dataflow/internal/VariableTypeInference.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll index 8155ad813190..6f611da31532 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll @@ -217,7 +217,7 @@ private class AnalyzedImplicitInit extends AnalyzedSsaDefinition, SsaImplicitIni */ private class AnalyzedVariableCapture extends AnalyzedSsaDefinition, SsaVariableCapture { override AbstractValue getAnRhsValue() { - exists(LocalVariable v | v = getSourceVariable() | + exists(LocalVariable v | v = this.getSourceVariable() | result = v.(AnalyzedCapturedVariable).getALocalValue() or result = any(AnalyzedExplicitDefinition def | def.getSourceVariable() = v).getAnRhsValue() From 1f77aab6bf9601693948a209db1ab7ccb060e0f6 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Wed, 22 Dec 2021 12:11:02 -0800 Subject: [PATCH 4/8] JS: Tweak performance of CorsOriginHeaderWithAssociatedCredentialHeader On databases with a large number of Exprs, it can be better to start with the set of route handlers, then find their response headers, then find the expression values set in those headers. --- .../CorsMisconfigurationForCredentialsCustomizations.qll | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll index 867494fc0a36..37b1830018e7 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll @@ -50,8 +50,12 @@ module CorsMisconfigurationForCredentials { | routeHandler.getAResponseHeader(_) = origin and routeHandler.getAResponseHeader(_) = credentials and - origin.definesExplicitly("access-control-allow-origin", this.asExpr()) and - credentials.definesExplicitly("access-control-allow-credentials", credentialsValue) + // Performance optimisation: start with the set of all route handlers + // rather than the set of all exprs. + pragma[only_bind_into](origin) + .definesExplicitly("access-control-allow-origin", this.asExpr()) and + pragma[only_bind_into](credentials) + .definesExplicitly("access-control-allow-credentials", credentialsValue) | credentialsValue.mayHaveBooleanValue(true) or credentialsValue.mayHaveStringValue("true") From c5be2770d1efbe69a88ea7f70146507b5eab7c9c Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Wed, 22 Dec 2021 11:16:32 -0800 Subject: [PATCH 5/8] JS: Improve performance of StandardEndpointFilters::isNumeric Factor the regex-independent logic of `isReadFrom` into its own predicate. Call this predicate directly from `isNumeric`, which doesn't have much restrictive context on the set of starting nodes. Use a binding hint to discourage starting with all expr nodes in this case. Other callers may have more restrictive context on the set of nodes, so they are not changed. --- .../StandardEndpointFilters.qll | 6 +++++- .../heuristics/SyntacticHeuristics.qll | 18 +++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll index 38d339a85278..8b4acbee61eb 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll @@ -53,7 +53,11 @@ predicate isSomeModeledArgument(DataFlow::Node n) { /** * Holds if `n` appears to be a numeric value. */ -predicate isNumeric(DataFlow::Node n) { isReadFrom(n, ".*index.*") } +// Performance optimisation: This predicate operates on a large set of +// starting nodes, so use binding hints to suggest computing that set last. +predicate isNumeric(DataFlow::Node n) { + getAnAccessedName(pragma[only_bind_into](n)).regexpMatch(".*index.*") +} /** * Holds if `n` is an argument to a library without sinks. diff --git a/javascript/ql/lib/semmle/javascript/heuristics/SyntacticHeuristics.qll b/javascript/ql/lib/semmle/javascript/heuristics/SyntacticHeuristics.qll index 12356d1bf42b..de7ca2a852e5 100644 --- a/javascript/ql/lib/semmle/javascript/heuristics/SyntacticHeuristics.qll +++ b/javascript/ql/lib/semmle/javascript/heuristics/SyntacticHeuristics.qll @@ -16,15 +16,23 @@ import javascript */ bindingset[regexp] predicate isReadFrom(DataFlow::Node read, string regexp) { + getAnAccessedName(read).regexpMatch(regexp) +} + +/** + * Gets the "name" accessed by `read`. The "name" is one of: + * - the name of the read variable, if `read` is a variable read + * - the name of the read property, if `read` is a property read + * - the suffix of the getter-method name, if `read` is a getter invocation, for example "Number" in "getNumber" + */ +string getAnAccessedName(DataFlow::Node read) { exists(DataFlow::Node actualRead | actualRead = read.asExpr().getUnderlyingValue().(LogOrExpr).getAnOperand().flow() or // unfold `x || y` once actualRead = read | - exists(string name | name.regexpMatch(regexp) | - actualRead.asExpr().getUnderlyingValue().(VarAccess).getName() = name or - actualRead.(DataFlow::PropRead).getPropertyName() = name or - actualRead.(DataFlow::InvokeNode).getCalleeName() = "get" + name - ) + actualRead.asExpr().getUnderlyingValue().(VarAccess).getName() = result or + actualRead.(DataFlow::PropRead).getPropertyName() = result or + actualRead.(DataFlow::InvokeNode).getCalleeName() = "get" + result ) } From f7c70c11fc14c8f89c630326e89e632d02077f46 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Wed, 22 Dec 2021 16:03:01 -0800 Subject: [PATCH 6/8] JS: Fix possible typo in DominatingPaths::hasWrite Should most likely refer to `AccessPathWrite` and look for write nodes. This also improves the performance of `rankedAccessPath`, since the set of candidate blocks is now limited to blocks with both a read and a write. --- javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 924e8f0a1664..3f94bcc36ebb 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -492,7 +492,7 @@ module AccessPath { */ pragma[noinline] private predicate hasWrite(ReachableBasicBlock bb) { - bb = getAccessTo(_, _, AccessPathRead()).getBasicBlock() + bb = getAccessTo(_, _, AccessPathWrite()).getBasicBlock() } /** From 0eaba2b9e42f6062d9254cd5986c39692c1c339a Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Wed, 22 Dec 2021 16:04:51 -0800 Subject: [PATCH 7/8] JS: Minor simplication of ranked basic block calculation We have to look up the node index within the block anyway, so include it as an aggregation variable. --- javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 3f94bcc36ebb..18719b3a15b6 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -462,15 +462,15 @@ module AccessPath { ReachableBasicBlock bb, Root root, string path, int ranking, AccessPathKind type ) { result = - rank[ranking](ControlFlowNode ref | + rank[ranking](ControlFlowNode ref, int i | ref = getAccessTo(root, path, _) and - ref.getBasicBlock() = bb and + ref = bb.getNode(i) and // Prunes the accesses where there does not exists a read and write within the same basicblock. // This could be more precise, but doing it like this avoids massive joins. hasRead(bb) and hasWrite(bb) | - ref order by any(int i | ref = bb.getNode(i)) + ref order by i ) and result = getAccessTo(root, path, type) } From b43891a80c6b3cbec9627883d9cef914ea49488e Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Wed, 22 Dec 2021 16:06:03 -0800 Subject: [PATCH 8/8] JS: Improve performance of DominatingPaths::hasDominatingWrite Check that the read node is in a *reachable* basic block before looking for a dominating write block. --- javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 18719b3a15b6..34c34c7f46e0 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -565,9 +565,12 @@ module AccessPath { ) or // across basic blocks. - exists(Root root, string path | + exists(Root root, string path, ReachableBasicBlock readBlock | read.asExpr() = getAccessTo(root, path, AccessPathRead()) and - getAWriteBlock(root, path).strictlyDominates(read.getBasicBlock()) + readBlock = read.getBasicBlock() and + // Performance optimisation: check that `read` is in a *reachable* basic block + // before looking for a dominating write block. + getAWriteBlock(root, path).strictlyDominates(pragma[only_bind_out](readBlock)) ) } }