mirror of
https://github.com/github/codeql.git
synced 2026-04-29 02:35:15 +02:00
JavaScript: Add new query UnvalidatedDynamicMethodCall.
This commit is contained in:
Max Schaefer
@@ -0,0 +1,86 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
<p>
|
||||
JavaScript makes it easy to look up object properties dynamically at runtime. In particular, methods
|
||||
can be looked up by name and then called. However, if he method name is user controlled, an attacker
|
||||
could choose a name that makes the application invoke an unexpected method, which may cause a runtime
|
||||
exception. If this exception is not handled, it could be used to mount a denial-of-service attack.
|
||||
</p>
|
||||
<p>
|
||||
For example, there might not be a method of the given name or the result of the lookup might not be
|
||||
a function, which would cause the method call to throw a <code>TypeError</code> at runtime.
|
||||
</p>
|
||||
<p>
|
||||
Another, more subtle example is where the result of the lookup is a standard library method from
|
||||
<code>Object.prototype</code>, which most objects have on their prototype chain. Examples of such
|
||||
methods include <code>valueOf</code>, <code>hasOwnProperty</code> and <code>__defineSetter__</code>.
|
||||
If the method call passes the wrong number or kind of arguments to these methods, they will
|
||||
throw an exception.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
It is best to avoid dynamic method lookup involving user-controlled names altogether, for instance
|
||||
by using a <code>Map</code> instead of a plain object.
|
||||
</p>
|
||||
<p>
|
||||
If the dynamic method lookup cannot be avoided, consider whitelisting permitted method names. At
|
||||
the very least, check that the method is an own property and not inherited from the prototype object.
|
||||
If the object on which the method is looked up contains properties that are not methods, you
|
||||
should additionally check that the result of the lookup is a function. Even if the object only
|
||||
contains methods it is still a good idea to perform this check in case other properties are
|
||||
added to the object later on.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
In the following example, an HTTP request parameter <code>action</code> property is used to dynamically
|
||||
look up a function in the <code>actions</code> map, which is then invoked with the <code>payload</code>
|
||||
parameter as its argument.
|
||||
</p>
|
||||
|
||||
<sample src="examples/UnvalidatedDynamicMethodCall.js" />
|
||||
|
||||
<p>
|
||||
The intention is to allow clients to invoke the <code>play</code> or <code>pause</code> method, but there
|
||||
is no check that <code>action</code> is actually the name of a method stored in <code>actions</code>.
|
||||
If, for example, <code>action</code> is <code>rewind</code>, <code>action</code> will be <code>undefined</code>
|
||||
and the call will result in a runtime error.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
The easiest way to prevent this is to turn <code>actions</code> into a <code>Map</code> and using
|
||||
<code>Map.prototype.has</code> to check whether the method name is valid before looking it up.
|
||||
</p>
|
||||
|
||||
<sample src="examples/UnvalidatedDynamicMethodCallGood.js" />
|
||||
|
||||
<p>
|
||||
If <code>actions</code> cannot be turned into a <code>Map</code>, a <code>hasOwnProperty</code>
|
||||
check should be added to validate the method name:
|
||||
</p>
|
||||
|
||||
<sample src="examples/UnvalidatedDynamicMethodCallGood2.js" />
|
||||
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>
|
||||
OWASP:
|
||||
<a href="https://www.owasp.org/index.php/Denial_of_Service">Denial of Service</a>.
|
||||
</li>
|
||||
<li>
|
||||
MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a>.
|
||||
</li>
|
||||
<li>
|
||||
MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/prototype">Object.prototype</a>.
|
||||
</li>
|
||||
</references>
|
||||
|
||||
</qhelp>
|
||||
@@ -0,0 +1,21 @@
|
||||
/**
|
||||
* @name Unvalidated dynamic method call
|
||||
* @description Calling a method with a user-controlled name may dispatch to
|
||||
* an unexpected target, which could cause an exception.
|
||||
* @kind path-problem
|
||||
* @problem.severity warning
|
||||
* @precision high
|
||||
* @id js/unvalidated-dynamic-method-call
|
||||
* @tags security
|
||||
* external/cwe/cwe-754
|
||||
*/
|
||||
|
||||
import javascript
|
||||
import semmle.javascript.security.dataflow.UnvalidatedDynamicMethodCall::UnvalidatedDynamicMethodCall
|
||||
import DataFlow::PathGraph
|
||||
|
||||
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
where cfg.hasFlowPath(source, sink)
|
||||
select sink.getNode(), source, sink,
|
||||
"Invocation of method with $@ name may dispatch to unexpected target and cause an exception.",
|
||||
source.getNode(), "user-controlled"
|
||||
@@ -0,0 +1,16 @@
|
||||
var express = require('express');
|
||||
var app = express();
|
||||
|
||||
var actions = {
|
||||
play(data) {
|
||||
// ...
|
||||
},
|
||||
pause(data) {
|
||||
// ...
|
||||
}
|
||||
}
|
||||
|
||||
app.get('/perform/:action/:payload', function(req, res) {
|
||||
let action = actions[req.params.action];
|
||||
res.end(action(req.params.payload));
|
||||
});
|
||||
@@ -0,0 +1,19 @@
|
||||
var express = require('express');
|
||||
var app = express();
|
||||
|
||||
var actions = new Map();
|
||||
actions.put("play", function (data) {
|
||||
// ...
|
||||
});
|
||||
actions.put("pause", function(data) {
|
||||
// ...
|
||||
});
|
||||
|
||||
app.get('/perform/:action/:payload', function(req, res) {
|
||||
if (actions.has(req.params.action)) {
|
||||
let action = actions.get(req.params.action);
|
||||
res.end(action(req.params.payload));
|
||||
} else {
|
||||
res.end("Unsupported action.");
|
||||
}
|
||||
});
|
||||
@@ -0,0 +1,22 @@
|
||||
var express = require('express');
|
||||
var app = express();
|
||||
|
||||
var actions = {
|
||||
play(data) {
|
||||
// ...
|
||||
},
|
||||
pause(data) {
|
||||
// ...
|
||||
}
|
||||
}
|
||||
|
||||
app.get('/perform/:action/:payload', function(req, res) {
|
||||
if (actions.hasOwnProperty(req.params.action)) {
|
||||
let action = actions[req.params.action];
|
||||
if (typeof action === 'function') {
|
||||
res.end(action(req.params.payload));
|
||||
return;
|
||||
}
|
||||
}
|
||||
res.end("Unsupported action.");
|
||||
});
|
||||
@@ -0,0 +1,153 @@
|
||||
/**
|
||||
* Provides a taint-tracking configuration for reasoning about unvalidated dynamic
|
||||
* method calls.
|
||||
*/
|
||||
|
||||
import javascript
|
||||
import semmle.javascript.frameworks.Express
|
||||
import PropertyInjectionShared
|
||||
private import semmle.javascript.dataflow.InferredTypes
|
||||
|
||||
module UnvalidatedDynamicMethodCall {
|
||||
private import DataFlow::FlowLabel
|
||||
|
||||
/**
|
||||
* A data flow source for unvalidated dynamic method calls.
|
||||
*/
|
||||
abstract class Source extends DataFlow::Node {
|
||||
/**
|
||||
* Gets the flow label relevant for this source.
|
||||
*/
|
||||
DataFlow::FlowLabel getFlowLabel() {
|
||||
result = data()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A data flow sink for unvalidated dynamic method calls.
|
||||
*/
|
||||
abstract class Sink extends DataFlow::Node {
|
||||
/**
|
||||
* Gets the flow label relevant for this sink
|
||||
*/
|
||||
abstract DataFlow::FlowLabel getFlowLabel();
|
||||
}
|
||||
|
||||
/**
|
||||
* A sanitizer for unvalidated dynamic method calls.
|
||||
*/
|
||||
abstract class Sanitizer extends DataFlow::Node {
|
||||
abstract predicate sanitizes(DataFlow::Node source, DataFlow::Node sink, DataFlow::FlowLabel lbl);
|
||||
}
|
||||
|
||||
/**
|
||||
* A flow label describing values read from a user-controlled property that
|
||||
* may not be functions.
|
||||
*/
|
||||
private class MaybeNonFunction extends DataFlow::FlowLabel {
|
||||
MaybeNonFunction() { this = "MaybeNonFunction" }
|
||||
}
|
||||
|
||||
/**
|
||||
* A flow label describing values read from a user-controlled property that
|
||||
* may originate from a prototype object.
|
||||
*/
|
||||
private class MaybeFromProto extends DataFlow::FlowLabel {
|
||||
MaybeFromProto() { this = "MaybeFromProto" }
|
||||
}
|
||||
|
||||
/**
|
||||
* A taint-tracking configuration for reasoning about unvalidated dynamic method calls.
|
||||
*/
|
||||
class Configuration extends TaintTracking::Configuration {
|
||||
Configuration() { this = "UnvalidatedDynamicMethodCall" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
|
||||
source.(Source).getFlowLabel() = label
|
||||
}
|
||||
|
||||
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
|
||||
sink.(Sink).getFlowLabel() = label
|
||||
}
|
||||
|
||||
override predicate isSanitizer(DataFlow::Node nd) {
|
||||
super.isSanitizer(nd) or
|
||||
nd instanceof PropertyInjection::Sanitizer
|
||||
}
|
||||
|
||||
override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) {
|
||||
exists (DataFlow::PropRead read |
|
||||
src = read.getPropertyNameExpr().flow() and
|
||||
dst = read and
|
||||
(srclabel = data() or srclabel = taint()) and
|
||||
(dstlabel instanceof MaybeNonFunction
|
||||
or
|
||||
// a property of `Object.create(null)` cannot come from a prototype
|
||||
not PropertyInjection::isPrototypeLessObject(read.getBase().getALocalSource()) and
|
||||
dstlabel instanceof MaybeFromProto) and
|
||||
// avoid overlapping results with unsafe dynamic method access query
|
||||
not PropertyInjection::hasUnsafeMethods(read.getBase().getALocalSource())
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A source of remote user input, considered as a source for unvalidated dynamic method calls.
|
||||
*/
|
||||
class RemoteFlowSourceAsSource extends Source {
|
||||
RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
|
||||
}
|
||||
|
||||
/**
|
||||
* The page URL considered as a flow source for unvalidated dynamic method calls.
|
||||
*/
|
||||
class DocumentUrlAsSource extends Source {
|
||||
DocumentUrlAsSource() { isDocumentURL(asExpr()) }
|
||||
}
|
||||
|
||||
/**
|
||||
* A function invocation of an unsafe function, as a sink for remote unvalidated dynamic method calls.
|
||||
*/
|
||||
class CalleeAsSink extends Sink {
|
||||
InvokeExpr invk;
|
||||
|
||||
CalleeAsSink() {
|
||||
this = invk.getCallee().flow() and
|
||||
// don't flag invocations inside a try-catch
|
||||
not invk.getASuccessor() instanceof CatchClause
|
||||
}
|
||||
|
||||
override DataFlow::FlowLabel getFlowLabel() {
|
||||
result instanceof MaybeNonFunction and
|
||||
// don't flag if the type inference can prove that it is a function;
|
||||
// this complements the `FunctionCheck` sanitizer below: the type inference can
|
||||
// detect more checks locally, but doesn't provide inter-procedural reasoning
|
||||
this.analyze().getAType() != TTFunction()
|
||||
or
|
||||
result instanceof MaybeFromProto
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A check of the form `typeof x === 'function'`, which sanitizes away the `MaybeNonFunction`
|
||||
* taint kind.
|
||||
*/
|
||||
class FunctionCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
|
||||
override EqualityTest astNode;
|
||||
TypeofExpr t;
|
||||
|
||||
FunctionCheck() {
|
||||
astNode.getAnOperand().getStringValue() = "function" and
|
||||
astNode.getAnOperand().getUnderlyingValue() = t
|
||||
}
|
||||
|
||||
override predicate sanitizes(boolean outcome, Expr e) {
|
||||
outcome = astNode.getPolarity() and
|
||||
e = t.getOperand().getUnderlyingValue()
|
||||
}
|
||||
|
||||
override DataFlow::FlowLabel getALabel() {
|
||||
result instanceof MaybeNonFunction
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user