Skip to content
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Improved sanitizer-guard handling for JavaScript and TypeScript variables that are assigned a safe fallback value, reducing false-positive security alerts.
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,8 @@ deprecated private predicate barrierGuardBlocksEdge(
|
bb = getADominatedBasicBlock(guard, cond) and
pred = DataFlow::ssaDefinitionNode(input) and
succ = DataFlow::ssaDefinitionNode(phi) and
succ.(DataFlowPrivate::SsaPhiInputNode).getPhi() = phi and
succ.(DataFlowPrivate::SsaPhiInputNode).getInputBlock() = bb and
input = phi.getInputFromBlock(bb) and
outcome = cond.getOutcome() and
barrierGuardBlocksExpr(guard, outcome, input.getAUse(), label)
Expand Down
24 changes: 20 additions & 4 deletions javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
Original file line number Diff line number Diff line change
Expand Up @@ -1740,10 +1740,26 @@ module DataFlow {
succ = TSsaDefNode(ssaCapture)
)
or
// Flow through implicit SSA nodes
exists(SsaImplicitDefinition ssa | succ = TSsaDefNode(ssa) |
// from the inputs of phi and pi nodes into the node itself
pred = TSsaDefNode(ssa.(SsaPseudoDefinition).getAnInput().getDefinition())
// Flow into a phi through a separate node when an incoming branch can be
// controlled by a guard, allowing barriers to block just the checked input.
// Other phi inputs keep the direct edge to avoid unnecessary flow nodes.
exists(SsaPhiNode phi, BasicBlock bb |
pred = TSsaDefNode(phi.getInputFromBlock(bb).getDefinition()) and
succ = TSsaPhiInputNode(phi, bb)
or
pred = TSsaPhiInputNode(phi, bb) and
succ = TSsaDefNode(phi)
or
pred = TSsaDefNode(phi.getInputFromBlock(bb).getDefinition()) and
succ = TSsaDefNode(phi) and
not exists(TSsaPhiInputNode(phi, bb))
)
or
// Other pseudo-definitions, such as SSA refinements, retain their usual flow.
exists(SsaPseudoDefinition ssa |
not ssa instanceof SsaPhiNode and
pred = TSsaDefNode(ssa.getAnInput().getDefinition()) and
succ = TSsaDefNode(ssa)
)
or
exists(Expr predExpr, Expr succExpr |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,21 @@ module MakeStateBarrierGuard<
)
}

/** Blocks only the incoming phi branch where the checked value is safe. */
pragma[nomagic]
private predicate barrierGuardBlocksPhiInput(
BarrierGuard guard, SsaPhiNode phi, BasicBlock bb, FlowState state
) {
exists(ConditionGuardNode cond, boolean outcome, SsaVariable input |
barrierGuardIsRelevant(guard) and
guard.getEnclosingExpr() = cond.getTest() and
cond.dominates(bb) and
outcome = cond.getOutcome() and
input = phi.getInputFromBlock(bb) and
barrierGuardBlocksExpr(guard, outcome, input.getAUse(), state)
)
}

/**
* Holds if the result of `guard` is used in the branching condition `cond`.
*
Expand Down Expand Up @@ -389,6 +404,11 @@ module MakeStateBarrierGuard<
nd = DataFlow::valueNode(use)
)
or
exists(BarrierGuard guard, DataFlowPrivate::SsaPhiInputNode input |
nd = input and
barrierGuardBlocksPhiInput(guard, input.getPhi(), input.getInputBlock(), state)
)
or
ssa2BlocksNode(nd, state)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ private module Cached {
TValueNode(AST::ValueNode nd) or
/** An SSA node from the legacy SSA library */
TSsaDefNode(SsaDefinition d) or
/** The input to a phi node from one predecessor block, for branch-specific barriers. */
TSsaPhiInputNode(SsaPhiNode phi, BasicBlock predecessor) {
exists(phi.getInputFromBlock(predecessor)) and
// Only split inputs whose incoming branch is controlled by a condition.
// Other inputs retain their direct SSA flow edge.
exists(ConditionGuardNode guard | guard.dominates(predecessor))
} or
/** Use of a variable or 'this', with flow from a post-update node (from an earlier use) */
TSsaUseNode(ControlFlowNode use) { use = any(Ssa2::SsaConfig::SourceVariable v).getAUse() } or
/** A synthesized variable read (new SSA library). Definitions are represented by TSsaDefNode. */
Expand Down Expand Up @@ -130,8 +137,8 @@ private module Cached {
import Cached

private class TEarlyStageNode =
TValueNode or TSsaDefNode or TCapturedVariableNode or TPropNode or TRestPatternNode or
TElementPatternNode or TElementNode or TReflectiveCallNode or TThisNode or
TValueNode or TSsaDefNode or TSsaPhiInputNode or TCapturedVariableNode or TPropNode or
TRestPatternNode or TElementPatternNode or TElementNode or TReflectiveCallNode or TThisNode or
TFunctionSelfReferenceNode or TDestructuredModuleImportNode or THtmlAttributeNode or
TFunctionReturnNode or TExceptionalFunctionReturnNode or TExceptionalInvocationReturnNode or
TGlobalAccessPathRoot or TTemplatePlaceholderTag or TReflectiveParametersNode or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,27 @@ class SsaUseNode extends DataFlow::Node, TSsaUseNode {
override Location getLocation() { result = expr.getLocation() }
}

/** One incoming edge of an SSA phi, represented as a node so guards can block it. */
class SsaPhiInputNode extends DataFlow::Node, TSsaPhiInputNode {
private SsaPhiNode phi;
private BasicBlock predecessor;

SsaPhiInputNode() { this = TSsaPhiInputNode(phi, predecessor) }

SsaPhiNode getPhi() { result = phi }

BasicBlock getInputBlock() { result = predecessor }

cached
override string toString() { result = "[ssa-phi-input] " + phi.toString() }

cached
override StmtContainer getContainer() { result = phi.getSourceVariable().getDeclaringContainer() }

cached
override Location getLocation() { result = predecessor.getLocation() }
}

class SsaSynthReadNode extends DataFlow::Node, TSsaSynthReadNode {
private Ssa2::SsaSynthReadNode read;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ legacyDataFlowDifference
| object-bypass-sanitizer.js:35:29:35:36 | source() | object-bypass-sanitizer.js:23:14:23:20 | obj.foo | only flow with OLD data flow library |
| object-bypass-sanitizer.js:35:29:35:36 | source() | object-bypass-sanitizer.js:28:10:28:30 | sanitiz ... bj).foo | only flow with OLD data flow library |
| promise.js:12:20:12:27 | source() | promise.js:13:8:13:23 | resolver.promise | only flow with OLD data flow library |
| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x | only flow with NEW data flow library |
| spread.js:4:15:4:22 | source() | spread.js:18:8:18:8 | y | only flow with NEW data flow library |
| spread.js:4:15:4:22 | source() | spread.js:24:8:24:8 | y | only flow with NEW data flow library |
| tst.js:2:13:2:20 | source() | tst.js:17:10:17:10 | a | only flow with OLD data flow library |
Expand Down Expand Up @@ -265,13 +264,15 @@ flow
| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:26:9:26:14 | this.x |
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x |
| sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:81:8:81:8 | x |
| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:84:10:84:10 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:93:8:93:8 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:98:7:98:7 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:104:7:104:7 | x |
| sanitizer-guards.js:102:9:102:16 | source() | sanitizer-guards.js:104:8:104:8 | x |
| sanitizer-guards.js:108:11:108:18 | source() | sanitizer-guards.js:112:8:112:8 | x |
| sanitizer-guards.js:116:11:116:18 | source() | sanitizer-guards.js:121:8:121:8 | x |
| sanitizer-guards.js:135:11:135:18 | source() | sanitizer-guards.js:137:8:137:8 | x |
| sanitizer-guards.js:135:11:135:18 | source() | sanitizer-guards.js:140:10:140:10 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:149:8:149:8 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:154:7:154:7 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:160:7:160:7 | x |
| spread.js:4:15:4:22 | source() | spread.js:6:8:6:19 | { ...taint } |
| spread.js:4:15:4:22 | source() | spread.js:7:8:7:43 | { f: 'h ... orld' } |
| spread.js:4:15:4:22 | source() | spread.js:9:8:9:19 | [ ...taint ] |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ legacyDataFlowDifference
| nested-props.js:14:15:14:22 | source() | nested-props.js:15:10:15:16 | obj.x.y | only flow with NEW data flow library |
| nested-props.js:27:18:27:25 | source() | nested-props.js:28:10:28:14 | obj.x | only flow with NEW data flow library |
| nested-props.js:51:22:51:29 | source() | nested-props.js:52:10:52:16 | obj.x.y | only flow with NEW data flow library |
| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x | only flow with NEW data flow library |
| spread.js:4:15:4:22 | source() | spread.js:18:8:18:8 | y | only flow with NEW data flow library |
| spread.js:4:15:4:22 | source() | spread.js:24:8:24:8 | y | only flow with NEW data flow library |
| tst.js:2:13:2:20 | source() | tst.js:35:14:35:16 | ary | only flow with NEW data flow library |
Expand Down Expand Up @@ -174,16 +173,18 @@ flow
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:52:10:52:10 | x |
| sanitizer-guards.js:57:11:57:18 | source() | sanitizer-guards.js:64:8:64:8 | x |
| sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:81:8:81:8 | x |
| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:84:10:84:10 | x |
| sanitizer-guards.js:79:11:79:18 | source() | sanitizer-guards.js:86:9:86:9 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:93:8:93:8 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:96:10:96:10 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:98:7:98:7 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:102:10:102:10 | x |
| sanitizer-guards.js:91:11:91:18 | source() | sanitizer-guards.js:104:7:104:7 | x |
| sanitizer-guards.js:102:9:102:16 | source() | sanitizer-guards.js:104:8:104:8 | x |
| sanitizer-guards.js:108:11:108:18 | source() | sanitizer-guards.js:112:8:112:8 | x |
| sanitizer-guards.js:116:11:116:18 | source() | sanitizer-guards.js:121:8:121:8 | x |
| sanitizer-guards.js:135:11:135:18 | source() | sanitizer-guards.js:137:8:137:8 | x |
| sanitizer-guards.js:135:11:135:18 | source() | sanitizer-guards.js:140:10:140:10 | x |
| sanitizer-guards.js:135:11:135:18 | source() | sanitizer-guards.js:142:9:142:9 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:149:8:149:8 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:152:10:152:10 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:154:7:154:7 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:158:10:158:10 | x |
| sanitizer-guards.js:147:11:147:18 | source() | sanitizer-guards.js:160:7:160:7 | x |
| spread.js:4:15:4:22 | source() | spread.js:18:8:18:8 | y |
| spread.js:4:15:4:22 | source() | spread.js:24:8:24:8 | y |
| thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ function phi() {
} else {
x = null;
}
sink(x); // OK [INCONSISTENCY] - dataflow2 cannot block the phi edge
sink(x); // OK
}

function phi2() {
Expand All @@ -75,6 +75,62 @@ function phi2() {
sink(x); // NOT OK
}

function fallbackAssignment() {
let x = source();

if (!isSafe(x)) {
x = "safe";
}
sink(x); // OK
}

function fallbackAssignmentElse() {
let x = source();

if (isSafe(x)) {
// The checked value is safe.
} else {
x = "safe";
}
sink(x); // OK
}

function fallbackUnsafe() {
let x = source();

if (!isSafe(x)) {
x = source();
}
sink(x); // NOT OK: the fallback is tainted.
}

function fallbackWrongPolarity() {
let x = source();
if (isSafe(x)) {
x = "safe";
}
sink(x); // NOT OK - the fall-through edge is unchecked
}

function fallbackGuardOnOtherVariable() {
let x = source();
let y = source();
if (!isSafe(y)) {
x = "safe";
}
sink(x); // NOT OK - the guard checks y, not x
}

function fallbackLoopInsideGuard() {
let x = source();
if (isSafe(x)) {
while (cond()) {
x = "safe";
}
sink(x); // OK
}
}

function falsy() {
let x = source();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
| lib/lib.js:228:10:228:25 | "rm -rf " + name | lib/lib.js:227:39:227:42 | name | lib/lib.js:228:22:228:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:227:39:227:42 | name | library input | lib/lib.js:228:2:228:26 | cp.exec ... + name) | shell command |
| lib/lib.js:236:10:236:25 | "rm -rf " + name | lib/lib.js:227:39:227:42 | name | lib/lib.js:236:22:236:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:227:39:227:42 | name | library input | lib/lib.js:236:2:236:26 | cp.exec ... + name) | shell command |
| lib/lib.js:249:10:249:25 | "rm -rf " + name | lib/lib.js:248:42:248:45 | name | lib/lib.js:249:22:249:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:248:42:248:45 | name | library input | lib/lib.js:249:2:249:26 | cp.exec ... + name) | shell command |
| lib/lib.js:253:10:253:28 | "rm -rf " + cleaned | lib/lib.js:248:42:248:45 | name | lib/lib.js:253:22:253:28 | cleaned | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:248:42:248:45 | name | library input | lib/lib.js:253:2:253:29 | cp.exec ... leaned) | shell command |
| lib/lib.js:258:10:258:25 | "rm -rf " + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:258:22:258:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:258:2:258:26 | cp.exec ... + name) | shell command |
| lib/lib.js:261:11:261:33 | "rm -rf ... + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command |
| lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command |
Expand Down Expand Up @@ -157,13 +156,7 @@ edges
| lib/lib.js:216:39:216:42 | name | lib/lib.js:224:22:224:25 | name | provenance | |
| lib/lib.js:227:39:227:42 | name | lib/lib.js:228:22:228:25 | name | provenance | |
| lib/lib.js:227:39:227:42 | name | lib/lib.js:236:22:236:25 | name | provenance | |
| lib/lib.js:239:28:239:28 | s | lib/lib.js:245:9:245:9 | s | provenance | |
| lib/lib.js:248:42:248:45 | name | lib/lib.js:249:22:249:25 | name | provenance | |
| lib/lib.js:248:42:248:45 | name | lib/lib.js:251:27:251:30 | name | provenance | |
| lib/lib.js:251:6:251:12 | cleaned | lib/lib.js:253:22:253:28 | cleaned | provenance | |
| lib/lib.js:251:16:251:31 | cleanInput(name) | lib/lib.js:251:6:251:12 | cleaned | provenance | |
| lib/lib.js:251:27:251:30 | name | lib/lib.js:239:28:239:28 | s | provenance | |
| lib/lib.js:251:27:251:30 | name | lib/lib.js:251:16:251:31 | cleanInput(name) | provenance | |
| lib/lib.js:257:35:257:38 | name | lib/lib.js:258:22:258:25 | name | provenance | |
| lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | provenance | |
| lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:24 | obj | provenance | |
Expand Down Expand Up @@ -329,14 +322,8 @@ nodes
| lib/lib.js:227:39:227:42 | name | semmle.label | name |
| lib/lib.js:228:22:228:25 | name | semmle.label | name |
| lib/lib.js:236:22:236:25 | name | semmle.label | name |
| lib/lib.js:239:28:239:28 | s | semmle.label | s |
| lib/lib.js:245:9:245:9 | s | semmle.label | s |
| lib/lib.js:248:42:248:45 | name | semmle.label | name |
| lib/lib.js:249:22:249:25 | name | semmle.label | name |
| lib/lib.js:251:6:251:12 | cleaned | semmle.label | cleaned |
| lib/lib.js:251:16:251:31 | cleanInput(name) | semmle.label | cleanInput(name) |
| lib/lib.js:251:27:251:30 | name | semmle.label | name |
| lib/lib.js:253:22:253:28 | cleaned | semmle.label | cleaned |
| lib/lib.js:257:35:257:38 | name | semmle.label | name |
| lib/lib.js:258:22:258:25 | name | semmle.label | name |
| lib/lib.js:261:30:261:33 | name | semmle.label | name |
Expand Down Expand Up @@ -452,6 +439,5 @@ nodes
| lib/subLib/index.js:13:44:13:46 | arr | semmle.label | arr |
| lib/subLib/index.js:14:22:14:24 | arr | semmle.label | arr |
subpaths
| lib/lib.js:251:27:251:30 | name | lib/lib.js:239:28:239:28 | s | lib/lib.js:245:9:245:9 | s | lib/lib.js:251:16:251:31 | cleanInput(name) |
| lib/lib.js:340:25:340:25 | n | lib/lib.js:329:13:329:13 | x | lib/lib.js:330:9:330:9 | x | lib/lib.js:340:22:340:26 | id(n) |
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last | lib/lib.js:437:9:437:11 | arr [ArrayElement] | lib/lib.js:428:14:428:58 | build(" ... + '-') |
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ module.exports.goodSanitizer = function (name) { // $ Source

var cleaned = cleanInput(name);

cp.exec("rm -rf " + cleaned); // $ SPURIOUS: Alert - SanitizingRegExpTest is not able to generate a barrier edge for an edge into a phi node.
cp.exec("rm -rf " + cleaned); // OK
}

var fs = require("fs");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,18 @@ app.get('/user/:id', function(req, res) {
res.send(escapeHtml2(url));
});

app.get('/language', function(req, res) {
let lang = req.query.lang;
if (!["en", "de"].includes(lang)) {
lang = "en";
}
res.send("<p>" + lang + "</p>"); // OK - allowlisted or replaced with a safe default
});

app.get('/greeting', function(req, res) {
let name = req.query.name;
if (!/^\w+$/.test(name)) {
name = "guest";
}
res.send("<p>Hello " + name + "</p>"); // OK - checked or replaced with a safe default
});
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,12 @@ function browser() {
window.onmessage = (e) => {
setTimeout(() => {}, e.data); // OK - source is client side
}
}
}

function clampedAllocation(req) {
let n = parseInt(req.query.n);
if (n > 1024) {
n = 1024;
}
Buffer.alloc(n); // OK - n is at most 1024 (or an invalid allocation size)
}