Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: Fix jump steps generated by IIFEs and exception flow #18043

Open
wants to merge 19 commits into
base: js/shared-dataflow-branch
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion javascript/ql/lib/semmle/javascript/StandardLibrary.qll
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ private class ArrayIterationCallbackAsPartialInvoke extends DataFlow::PartialInv
* A flow step propagating the exception thrown from a callback to a method whose name coincides
* a built-in Array iteration method, such as `forEach` or `map`.
*/
private class IteratorExceptionStep extends DataFlow::SharedFlowStep {
private class IteratorExceptionStep extends DataFlow::LegacyFlowStep {
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
exists(DataFlow::MethodCallNode call |
call.getMethodName() = ["forEach", "each", "map", "filter", "some", "every", "fold", "reduce"] and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ private module ConsistencyConfig implements InputSig<Location, JSDataFlow> {
or
n instanceof FlowSummaryDynamicParameterArrayNode
or
n instanceof FlowSummaryDefaultExceptionalReturn
or
n instanceof GenericSynthesizedNode
or
n = DataFlow::globalAccessPathRootPseudoNode()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ private module Cached {
// So it doesn't cause negative recursion but it might look a bit surprising.
FlowSummaryPrivate::Steps::summaryStoreStep(sn, MkAwaited(), _)
} or
TFlowSummaryDefaultExceptionalReturn(FlowSummaryImpl::Public::SummarizedCallable callable) {
not DataFlowPrivate::mentionsExceptionalReturn(callable)
} or
TSynthCaptureNode(VariableCapture::VariableCaptureOutput::SynthesizedCaptureNode node) or
TGenericSynthesizedNode(AstNode node, string tag, DataFlowPrivate::DataFlowCallable container) {
any(AdditionalFlowInternal flow).needsSynthesizedNode(node, tag, container)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,33 @@ class FlowSummaryIntermediateAwaitStoreNode extends DataFlow::Node,
}
}

predicate mentionsExceptionalReturn(FlowSummaryImpl::Public::SummarizedCallable callable) {
exists(FlowSummaryImpl::Private::SummaryNode node | node.getSummarizedCallable() = callable |
FlowSummaryImpl::Private::summaryReturnNode(node, MkExceptionalReturnKind())
or
FlowSummaryImpl::Private::summaryOutNode(_, node, MkExceptionalReturnKind())
)
}

/**
* Exceptional return node in a summarized callable whose summary does not mention `ReturnValue[exception]`.
*
* By default, every call inside such a callable will forward their exceptional return to the caller's
* exceptional return, i.e. exceptions are not caught.
*/
class FlowSummaryDefaultExceptionalReturn extends DataFlow::Node,
TFlowSummaryDefaultExceptionalReturn
{
private FlowSummaryImpl::Public::SummarizedCallable callable;

FlowSummaryDefaultExceptionalReturn() { this = TFlowSummaryDefaultExceptionalReturn(callable) }

FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = callable }

cached
override string toString() { result = "[default exceptional return] " + callable }
}

class CaptureNode extends DataFlow::Node, TSynthCaptureNode {
/** Gets the underlying node from the variable-capture library. */
VariableCaptureOutput::SynthesizedCaptureNode getNode() {
Expand Down Expand Up @@ -296,6 +323,9 @@ private predicate returnNodeImpl(DataFlow::Node node, ReturnKind kind) {
)
or
FlowSummaryImpl::Private::summaryReturnNode(node.(FlowSummaryNode).getSummaryNode(), kind)
or
node instanceof FlowSummaryDefaultExceptionalReturn and
kind = MkExceptionalReturnKind()
}

private DataFlow::Node getAnOutNodeImpl(DataFlowCall call, ReturnKind kind) {
Expand All @@ -311,6 +341,10 @@ private DataFlow::Node getAnOutNodeImpl(DataFlowCall call, ReturnKind kind) {
or
FlowSummaryImpl::Private::summaryOutNode(call.(SummaryCall).getReceiver(),
result.(FlowSummaryNode).getSummaryNode(), kind)
or
kind = MkExceptionalReturnKind() and
result.(FlowSummaryDefaultExceptionalReturn).getSummarizedCallable() =
call.(SummaryCall).getSummarizedCallable()
}

class ReturnNode extends DataFlow::Node {
Expand Down Expand Up @@ -404,6 +438,19 @@ abstract class LibraryCallable extends string {
DataFlow::InvokeNode getACallSimple() { none() }
}

/** Internal subclass of `LibraryCallable`, whose member predicates should not be visible on `SummarizedCallable`. */
abstract class LibraryCallableInternal extends LibraryCallable {
bindingset[this]
LibraryCallableInternal() { any() }

/**
* Gets a call to this library callable.
*
* Same as `getACall()` but is evaluated later and may depend negatively on `getACall()`.
*/
DataFlow::InvokeNode getACallStage2() { none() }
}

private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosition pos) {
exists(Parameter parameter |
parameter = c.asSourceCallable().(Function).getParameter(pos.asPositional()) and
Expand Down Expand Up @@ -505,6 +552,8 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) {
or
result.asLibraryCallable() = node.(FlowSummaryIntermediateAwaitStoreNode).getSummarizedCallable()
or
result.asLibraryCallable() = node.(FlowSummaryDefaultExceptionalReturn).getSummarizedCallable()
or
node = TGenericSynthesizedNode(_, _, result)
}

Expand Down Expand Up @@ -865,6 +914,8 @@ class SummaryCall extends DataFlowCall, MkSummaryCall {

/** Gets the receiver node. */
FlowSummaryImpl::Private::SummaryNode getReceiver() { result = receiver }

FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = enclosingCallable }
}

/**
Expand Down Expand Up @@ -976,7 +1027,11 @@ DataFlowCallable viableCallable(DataFlowCall node) {
or
exists(LibraryCallable callable |
result = MkLibraryCallable(callable) and
node.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
node.asOrdinaryCall() =
[
callable.getACall(), callable.getACallSimple(),
callable.(LibraryCallableInternal).getACallStage2()
]
)
or
result.asSourceCallableNotExterns() = node.asImpliedLambdaCall()
Expand Down Expand Up @@ -1217,14 +1272,29 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {

predicate localMustFlowStep(Node node1, Node node2) { node1 = node2.getImmediatePredecessor() }

/**
* Holds if `node1 -> node2` should be removed as a jump step.
*
* Currently this is done as a workaround for the local steps generated from IIFEs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Currently"?
Is that hinting towards plans for a another solution in the future?

Copy link
Contributor Author

@asgerf asgerf Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this predicate can populated with jump steps that should be excluded, and currently the only use-case for this is the workaround mentioned. So it was meant to imply that other things could get added to the predicate as well.

*/
private predicate excludedJumpStep(Node node1, Node node2) {
exists(ImmediatelyInvokedFunctionExpr iife |
iife.argumentPassing(node2.asExpr(), node1.asExpr())
or
node1 = iife.getAReturnedExpr().flow() and
node2 = iife.getInvocation().flow()
)
}

/**
* Holds if data can flow from `node1` to `node2` through a non-local step
* that does not follow a call edge. For example, a step through a global
* variable.
*/
predicate jumpStep(Node node1, Node node2) {
valuePreservingStep(node1, node2) and
node1.getContainer() != node2.getContainer()
node1.getContainer() != node2.getContainer() and
not excludedJumpStep(node1, node2)
or
FlowSummaryPrivate::Steps::summaryJumpStep(node1.(FlowSummaryNode).getSummaryNode(),
node2.(FlowSummaryNode).getSummaryNode())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ private import sharedlib.DataFlowImplCommon
private import sharedlib.FlowSummaryImpl::Private as Private
private import sharedlib.FlowSummaryImpl::Public
private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
private import semmle.javascript.internal.flow_summaries.ExceptionFlow

/**
* A class of callables that are candidates for flow summary modeling.
Expand Down Expand Up @@ -131,7 +132,11 @@ ReturnKind getStandardReturnValueKind() { result = MkNormalReturnKind() and Stag
private module FlowSummaryStepInput implements Private::StepsInputSig {
DataFlowCall getACall(SummarizedCallable sc) {
exists(LibraryCallable callable | callable = sc |
result.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
result.asOrdinaryCall() =
[
callable.getACall(), callable.getACallSimple(),
callable.(LibraryCallableInternal).getACallStage2()
]
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
private js::Function getLambdaFromVariable(js::LocalVariable variable) {
result.getVariable() = variable
or
result = variable.getAnAssignedExpr()
result = variable.getAnAssignedExpr().getUnderlyingValue()
or
exists(js::ClassDeclStmt cls |
result = cls.getConstructor().getBody() and
Expand All @@ -23,35 +23,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
or
isTopLevelLike(container.(js::ImmediatelyInvokedFunctionExpr).getEnclosingContainer())
or
// Functions declared in a top-level with no parameters and can't generate flow-through, except through 'this'
// which we rule out with a few syntactic checks. In this case we treat its captured variables as singletons.
// NOTE: This was done to prevent a blow-up in fiddlesalad where a function called 'Runtime' captures 7381 variables but is only called once.
exists(js::Function fun |
container = fun and
fun.getNumParameter() = 0 and
isTopLevelLike(fun.getEnclosingContainer()) and
not mayHaveFlowThroughThisArgument(fun)
)
or
// Container declaring >100 captured variables tend to be singletons and are too expensive anyway
// Containers declaring >100 captured variables tend to be singletons and are too expensive anyway
strictcount(js::LocalVariable v | v.isCaptured() and v.getDeclaringContainer() = container) >
100
}

private predicate hasLocalConstructorCall(js::Function fun) {
fun = getLambdaFromVariable(any(js::NewExpr e).getCallee().(js::VarAccess).getVariable())
}

private predicate mayHaveFlowThroughThisArgument(js::Function fun) {
any(js::ThisExpr e).getBinder() = fun and
not hasLocalConstructorCall(fun) and // 'this' argument is assumed to be a fresh object
(
exists(fun.getAReturnedExpr())
or
exists(js::YieldExpr e | e.getContainer() = fun)
)
}

class CapturedVariable extends LocalVariableOrThis {
CapturedVariable() {
DataFlowImplCommon::forceCachingInSameStage() and
Expand Down Expand Up @@ -172,9 +148,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
predicate hasAliasedAccess(Expr e) {
e = this
or
e.(js::Expr).getUnderlyingValue() = this
or
exists(js::LocalVariable variable |
this = getLambdaFromVariable(variable) and
e = variable.getAnAccess()
e.(js::Expr).getUnderlyingValue() = variable.getAnAccess()
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ predicate isTestFile(File f) {
)
or
f.getAbsolutePath().regexpMatch(".*/__(mocks|tests)__/.*")
or
f.getBaseName().matches("%.test.%")
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
private import AmbiguousCoreMethods
private import Arrays
private import AsyncAwait
private import ExceptionFlow
private import ForOfLoops
private import Generators
private import Iterators
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* Contains a summary for propagating exceptions out of callbacks
*/

private import javascript
private import FlowSummaryUtil
private import semmle.javascript.dataflow.internal.AdditionalFlowInternal
private import semmle.javascript.dataflow.internal.DataFlowPrivate
private import semmle.javascript.dataflow.FlowSummary
private import semmle.javascript.internal.flow_summaries.Promises

private predicate isCallback(DataFlow::SourceNode node) {
node instanceof DataFlow::FunctionNode
or
node instanceof DataFlow::PartialInvokeNode
or
exists(DataFlow::SourceNode prev |
isCallback(prev) and
DataFlow::argumentPassingStep(_, prev.getALocalUse(), _, node)
)
}

/**
* Summary that propagates exceptions out of callbacks back to the caller.
*
* This summary only applies to calls that have no other call targets.
* See also `FlowSummaryDefaultExceptionalReturn`, which handles calls that have a summary target,
* but where the summary does not mention `ReturnValue[exception]`.
*/
private class ExceptionFlowSummary extends SummarizedCallable, LibraryCallableInternal {
ExceptionFlowSummary() { this = "Exception propagator" }

override DataFlow::CallNode getACallStage2() {
not exists(result.getACallee()) and
not exists(SummarizedCallable c | result = [c.getACall(), c.getACallSimple()]) and
// Avoid a few common cases where the exception should not propagate back
not result.getCalleeName() = ["addEventListener", EventEmitter::on()] and
not result = promiseConstructorRef().getAnInvocation() and
// Restrict to cases where a callback is known to flow in, as lambda flow in DataFlowImplCommon blows up otherwise
isCallback(result.getAnArgument().getALocalSource())
}

override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
preservesValue = true and
input = "Argument[0..].ReturnValue[exception]" and
output = "ReturnValue[exception]"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ private import javascript
private import semmle.javascript.dataflow.FlowSummary
private import FlowSummaryUtil

private DataFlow::SourceNode promiseConstructorRef() {
DataFlow::SourceNode promiseConstructorRef() {
result = Promises::promiseConstructorRef()
or
result = DataFlow::moduleImport("bluebird")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import javascript
private import semmle.javascript.security.SensitiveActions
import InsecureRandomnessCustomizations::InsecureRandomness
private import InsecureRandomnessCustomizations::InsecureRandomness as InsecureRandomness
private import semmle.javascript.filters.ClassifyFiles as ClassifyFiles

/**
* A taint tracking configuration for random values that are not cryptographically secure.
Expand All @@ -20,7 +21,11 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig {

predicate isSink(DataFlow::Node sink) { sink instanceof Sink }

predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
predicate isBarrier(DataFlow::Node node) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be tested?
A test should probably also test d1c9e47.

node instanceof Sanitizer
or
ClassifyFiles::isTestFile(node.getFile())
}

predicate isBarrierOut(DataFlow::Node node) {
// stop propagation at the sinks to avoid double reporting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ legacyDataFlowDifference
| callbacks.js:44:17:44:24 | source() | callbacks.js:38:35:38:35 | x | only flow with NEW data flow library |
| capture-flow.js:89:13:89:20 | source() | capture-flow.js:89:6:89:21 | test3c(source()) | only flow with NEW data flow library |
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:102:6:102:20 | test5("safe")() | only flow with OLD data flow library |
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint | only flow with OLD data flow library |
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:40:8:40:14 | e.taint | only flow with NEW data flow library |
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library |
| constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library |
Expand Down Expand Up @@ -109,7 +110,6 @@ flow
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:101:6:101:22 | test5(source())() |
| capture-flow.js:110:12:110:19 | source() | capture-flow.js:106:14:106:14 | x |
| capture-flow.js:118:37:118:44 | source() | capture-flow.js:114:14:114:14 | x |
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint |
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:129:14:129:26 | orderingTaint |
| capture-flow.js:177:26:177:33 | source() | capture-flow.js:173:14:173:14 | x |
| capture-flow.js:187:34:187:41 | source() | capture-flow.js:183:14:183:14 | x |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ legacyDataFlowDifference
| callbacks.js:44:17:44:24 | source() | callbacks.js:38:35:38:35 | x | only flow with NEW data flow library |
| capture-flow.js:89:13:89:20 | source() | capture-flow.js:89:6:89:21 | test3c(source()) | only flow with NEW data flow library |
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:102:6:102:20 | test5("safe")() | only flow with OLD data flow library |
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint | only flow with OLD data flow library |
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:40:8:40:14 | e.taint | only flow with NEW data flow library |
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library |
| constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library |
Expand Down Expand Up @@ -84,7 +85,6 @@ flow
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:101:6:101:22 | test5(source())() |
| capture-flow.js:110:12:110:19 | source() | capture-flow.js:106:14:106:14 | x |
| capture-flow.js:118:37:118:44 | source() | capture-flow.js:114:14:114:14 | x |
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint |
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:129:14:129:26 | orderingTaint |
| capture-flow.js:177:26:177:33 | source() | capture-flow.js:173:14:173:14 | x |
| capture-flow.js:187:34:187:41 | source() | capture-flow.js:183:14:183:14 | x |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ global.doEscape(testEscapeViaReturn(source()));
function ordering() {
var orderingTaint;
global.addEventListener('click', () => {
sink(orderingTaint); // NOT OK
sink(orderingTaint); // NOT OK [INCONSISTENCY]
});
global.addEventListener('load', () => {
orderingTaint = source();
Expand Down
Loading