Skip to content

Commit fd9975d

Browse files
committed
JS: Address comments
1 parent 3ccdaa9 commit fd9975d

File tree

1 file changed

+13
-4
lines changed

1 file changed

+13
-4
lines changed

javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ class DynamicPropRead extends DataFlow::SourceNode, DataFlow::ValueNode {
163163

164164
/**
165165
* Holds if `output` is the result of `base[key]`, either directly or through
166-
* one or more function calls.
166+
* one or more function calls, ignoring reads that can't access the prototype chain.
167167
*/
168168
predicate dynamicPropReadStep(Node base, Node key, SourceNode output) {
169169
exists(DynamicPropRead read |
@@ -217,7 +217,12 @@ predicate isPotentiallyObjectPrototype(SourceNode node) {
217217
exists(Node base, Node key |
218218
dynamicPropReadStep(base, key, node) and
219219
isEnumeratedPropName(key) and
220-
not arePropertiesEnumerated(base.getALocalSource()) // ignore `for (let key in src) { ... src[key] ... }`
220+
221+
// Ignore cases where the properties of `base` are enumerated, to avoid FPs
222+
// where the key came from that enumeration (and thus will not return Object.prototype).
223+
// For example, `src[key]` in `for (let key in src) { ... src[key] ... }` will generally
224+
// not return Object.prototype because `key` is an enumerable property of `src`.
225+
not arePropertiesEnumerated(base.getALocalSource())
221226
)
222227
or
223228
exists(Node use |
@@ -241,11 +246,15 @@ predicate dynamicPropWrite(DataFlow::Node base, DataFlow::Node prop, DataFlow::N
241246
exists(AssignExpr write, IndexExpr index |
242247
index = write.getLhs() and
243248
base = index.getBase().flow() and
244-
isPotentiallyObjectPrototype(base.getALocalSource()) and
245249
prop = index.getPropertyNameExpr().flow() and
246250
rhs = write.getRhs().flow() and
247251
not exists(prop.getStringValue()) and
248-
not arePropertiesEnumerated(base.getALocalSource())
252+
not arePropertiesEnumerated(base.getALocalSource()) and
253+
254+
// Prune writes that are unlikely to modify Object.prototype.
255+
// This is mainly for performance, but may block certain results due to
256+
// not tracking out of function returns and into callbacks.
257+
isPotentiallyObjectPrototype(base.getALocalSource())
249258
)
250259
}
251260

0 commit comments

Comments
 (0)