mirror of
https://github.com/github/codeql.git
synced 2026-05-02 12:15:17 +02:00
JS: Add CaseSensitiveMiddlewarePath query
This commit is contained in:
@@ -148,6 +148,18 @@ module Routing {
|
||||
this instanceof MkRouter
|
||||
}
|
||||
|
||||
/**
|
||||
* Like `mayResumeDispatch` but without the assumption that functions with an unknown
|
||||
* implementation invoke their continuation.
|
||||
*/
|
||||
predicate definitelyResumesDispatch() {
|
||||
this.getLastChild().definitelyResumesDispatch()
|
||||
or
|
||||
exists(this.(RouteHandler).getAContinuationInvocation())
|
||||
or
|
||||
this instanceof MkRouter
|
||||
}
|
||||
|
||||
/** Gets the parent of this node, provided that this node may invoke its continuation. */
|
||||
private Node getContinuationParent() {
|
||||
result = this.getParent() and
|
||||
|
||||
@@ -33,6 +33,11 @@ module Express {
|
||||
or
|
||||
// `app = [new] express.Router()`
|
||||
result = DataFlow::moduleMember("express", "Router").getAnInvocation()
|
||||
or
|
||||
exists(DataFlow::SourceNode app |
|
||||
app.hasUnderlyingType("probot/lib/application", "Application") and
|
||||
result = app.getAMethodCall("route")
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -1043,4 +1048,22 @@ module Express {
|
||||
|
||||
override DataFlow::SourceNode getOutput() { result = this.getCallback(2).getParameter(1) }
|
||||
}
|
||||
|
||||
private class ResumeDispatchRefinement extends Routing::RouteHandler {
|
||||
ResumeDispatchRefinement() { getFunction() instanceof RouteHandler }
|
||||
|
||||
override predicate mayResumeDispatch() { getAParameter().getName() = "next" }
|
||||
|
||||
override predicate definitelyResumesDispatch() { getAParameter().getName() = "next" }
|
||||
}
|
||||
|
||||
private class ExpressStaticResumeDispatchRefinement extends Routing::Node {
|
||||
ExpressStaticResumeDispatchRefinement() {
|
||||
this = Routing::getNode(DataFlow::moduleMember("express", "static").getACall())
|
||||
}
|
||||
|
||||
override predicate mayResumeDispatch() { none() }
|
||||
|
||||
override predicate definitelyResumesDispatch() { none() }
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,112 @@
|
||||
/**
|
||||
* @name Case-sensitive middleware path
|
||||
* @description Middleware with case-sensitive paths do not protect endpoints with case-insensitive paths
|
||||
* @kind problem
|
||||
* @problem.severity warning
|
||||
* @security-severity 7.3
|
||||
* @precision high
|
||||
* @id js/case-sensitive-middleware-path
|
||||
* @tags security
|
||||
* external/cwe/cwe-178
|
||||
*/
|
||||
|
||||
import javascript
|
||||
|
||||
/**
|
||||
* Converts `s` to upper case, or to lower-case if it was already upper case.
|
||||
*/
|
||||
bindingset[s]
|
||||
string invertCase(string s) {
|
||||
if s.regexpMatch(".*[a-z].*") then result = s.toUpperCase() else result = s.toLowerCase()
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `term` distinguishes between upper and lower case letters, assuming the `i` flag is not present.
|
||||
*/
|
||||
pragma[inline]
|
||||
predicate isCaseSensitiveRegExp(RegExpTerm term) {
|
||||
exists(RegExpConstant const |
|
||||
const = term.getAChild*() and
|
||||
const.getValue().regexpMatch(".*[a-zA-Z].*") and
|
||||
not const.getParent().(RegExpCharacterClass).getAChild().(RegExpConstant).getValue() =
|
||||
invertCase(const.getValue()) and
|
||||
not const.getParent*() instanceof RegExpNegativeLookahead and
|
||||
not const.getParent*() instanceof RegExpNegativeLookbehind
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a string matched by `term`, or part of such a string.
|
||||
*/
|
||||
string getExampleString(RegExpTerm term) {
|
||||
result = term.getAMatchedString()
|
||||
or
|
||||
// getAMatchedString does not recurse into sequences. Perform one step manually.
|
||||
exists(RegExpSequence seq | seq = term |
|
||||
result =
|
||||
strictconcat(RegExpTerm child, int i, string text |
|
||||
child = seq.getChild(i) and
|
||||
(
|
||||
text = child.getAMatchedString()
|
||||
or
|
||||
not exists(child.getAMatchedString()) and
|
||||
text = ""
|
||||
)
|
||||
|
|
||||
text order by i
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
string getCaseSensitiveBypassExample(RegExpTerm term) {
|
||||
result = invertCase(getExampleString(term)) and
|
||||
result != ""
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `setup` has a path-argument `arg` referring to the given case-sensitive `regexp`.
|
||||
*/
|
||||
predicate isCaseSensitiveMiddleware(
|
||||
Routing::RouteSetup setup, DataFlow::RegExpCreationNode regexp, DataFlow::Node arg
|
||||
) {
|
||||
exists(DataFlow::MethodCallNode call |
|
||||
setup = Routing::getRouteSetupNode(call) and
|
||||
(
|
||||
setup.definitelyResumesDispatch()
|
||||
or
|
||||
// If applied to all HTTP methods, be a bit more lenient in detecting middleware
|
||||
setup.mayResumeDispatch() and
|
||||
not exists(setup.getOwnHttpMethod())
|
||||
) and
|
||||
arg = call.getArgument(0) and
|
||||
regexp.getAReference().flowsTo(arg) and
|
||||
isCaseSensitiveRegExp(regexp.getRoot()) and
|
||||
exists(string flags |
|
||||
flags = regexp.getFlags() and
|
||||
not flags.matches("%i%")
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
predicate isGuardedCaseInsensitiveEndpoint(
|
||||
Routing::RouteSetup endpoint, Routing::RouteSetup middleware
|
||||
) {
|
||||
isCaseSensitiveMiddleware(middleware, _, _) and
|
||||
exists(DataFlow::MethodCallNode call |
|
||||
endpoint = Routing::getRouteSetupNode(call) and
|
||||
endpoint.isGuardedByNode(middleware) and
|
||||
call.getArgument(0).mayHaveStringValue(_)
|
||||
)
|
||||
}
|
||||
|
||||
from
|
||||
DataFlow::RegExpCreationNode regexp, Routing::RouteSetup middleware, Routing::RouteSetup endpoint,
|
||||
DataFlow::Node arg, string example
|
||||
where
|
||||
isCaseSensitiveMiddleware(middleware, regexp, arg) and
|
||||
example = getCaseSensitiveBypassExample(regexp.getRoot()) and
|
||||
isGuardedCaseInsensitiveEndpoint(endpoint, middleware) and
|
||||
exists(endpoint.getRelativePath().toLowerCase().indexOf(example.toLowerCase()))
|
||||
select arg,
|
||||
"This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '"
|
||||
+ example + "' will bypass the middleware.", regexp, "pattern", endpoint, "here"
|
||||
@@ -0,0 +1,3 @@
|
||||
| tst.js:8:9:8:19 | /\\/foo\\/.*/ | This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '/FOO/' will bypass the middleware. | tst.js:8:9:8:19 | /\\/foo\\/.*/ | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | here |
|
||||
| tst.js:14:5:14:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '/FOO' will bypass the middleware. | tst.js:14:5:14:28 | new Reg ... (.*)?') | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | here |
|
||||
| tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '/FOO/' will bypass the middleware. | tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | here |
|
||||
@@ -0,0 +1 @@
|
||||
Security/CWE-178/CaseSensitiveMiddlewarePath.ql
|
||||
61
javascript/ql/test/query-tests/Security/CWE-178/tst.js
Normal file
61
javascript/ql/test/query-tests/Security/CWE-178/tst.js
Normal file
@@ -0,0 +1,61 @@
|
||||
const express = require('express');
|
||||
const app = express();
|
||||
const unknown = require('~something/blah');
|
||||
|
||||
app.all(/\/.*/, unknown()); // OK - does not contain letters
|
||||
app.all(/\/.*/i, unknown()); // OK
|
||||
|
||||
app.all(/\/foo\/.*/, unknown()); // NOT OK
|
||||
app.all(/\/foo\/.*/i, unknown()); // OK - case insensitive
|
||||
|
||||
app.use(/\/x\/#\d{6}/, express.static('images/')); // OK - not a middleware
|
||||
|
||||
app.get(
|
||||
new RegExp('^/foo(.*)?'), // NOT OK - case sensitive
|
||||
unknown(),
|
||||
function(req, res, next) {
|
||||
if (req.params.blah) {
|
||||
next();
|
||||
}
|
||||
}
|
||||
);
|
||||
|
||||
app.get(
|
||||
new RegExp('^/foo(.*)?', 'i'), // OK - case insensitive
|
||||
unknown(),
|
||||
function(req, res, next) {
|
||||
if (req.params.blah) {
|
||||
next();
|
||||
}
|
||||
}
|
||||
);
|
||||
|
||||
app.get(
|
||||
new RegExp('^/foo(.*)?'), // OK - not a middleware
|
||||
unknown(),
|
||||
function(req,res) {
|
||||
res.send('Hello World!');
|
||||
}
|
||||
);
|
||||
|
||||
app.use(/\/foo\/([0-9]+)/, (req, res, next) => { // NOT OK - case sensitive
|
||||
unknown(req);
|
||||
next();
|
||||
});
|
||||
|
||||
app.use(/\/foo\/([0-9]+)/i, (req, res, next) => { // OK - case insensitive
|
||||
unknown(req);
|
||||
next();
|
||||
});
|
||||
|
||||
|
||||
app.use(/\/foo\/([0-9]+)/, (req, res) => { // OK - not middleware
|
||||
unknown(req, res);
|
||||
});
|
||||
|
||||
app.use(/\/foo\/([0-9]+)/i, (req, res) => { // OK - not middleware (also case insensitive)
|
||||
unknown(req, res);
|
||||
});
|
||||
|
||||
app.get('/foo/:param', (req, res) => { // OK - not a middleware
|
||||
});
|
||||
Reference in New Issue
Block a user