hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-22 23:35:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

JS: Fix barrier guards for ServerSideUrlRedirect

Browse Source 
The barrier guards for ServerSideUrlRedirect were lost when it was ported to ConfigSig, and the aforementioned spurious alert was a result of that.

The query had two guards: a proper barrier guard and a heuristic one for functions named 'isLocalURL'. We should move away from the heuristic name-based sanitiser guards, so I'm only reinstating the proper barrier guard.

Therefore updating the test to test the real barrier guard.
This commit is contained in:
Asger F
2025-02-21 21:05:47 +01:00
parent 38be524b6a
commit 29659647ea
2 changed files with 10 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
javascript/ql/lib/semmle/javascript/security/dataflow/ServerSideUrlRedirectQuery.qll
View File

@@ -20,7 +20,11 @@ module ServerSideUrlRedirectConfig implements DataFlow::ConfigSig {
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
predicate isBarrier(DataFlow::Node node) {
node instanceof Sanitizer
or
node = HostnameSanitizerGuard::getABarrierNode()
}
predicate isBarrierOut(DataFlow::Node node) { hostnameSanitizingPrefixEdge(node, _) }
@@ -69,10 +73,12 @@ deprecated class Configuration extends TaintTracking::Configuration {
}
/**
* DEPRECATED. This is no longer used as a sanitizer guard.
*
* A call to a function called `isLocalUrl` or similar, which is
* considered to sanitize a variable for purposes of URL redirection.
*/
class LocalUrlSanitizingGuard extends DataFlow::CallNode {
deprecated class LocalUrlSanitizingGuard extends DataFlow::CallNode {
LocalUrlSanitizingGuard() { this.getCalleeName().regexpMatch("(?i)(is_?)?local_?url") }
/** DEPRECATED. Use `blocksExpr` instead. */

4
javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js
View File

@@ -23,9 +23,9 @@ app.get('/some/other/path2', function(req, res) {
app.get('/some/path', function(req, res) {
var target = req.param("target");
if (isLocalURL(target))
if (target.startsWith("http://example.com/"))
// OK - request parameter is sanitized before incorporating it into the redirect
res.redirect(target); // $ SPURIOUS: Alert
res.redirect(target);
else
res.redirect(target); // $ Alert - sanitization doesn't apply here
res.redirect(target); // $ Alert - sanitization doesn't apply here