OpenRedirect: treat assignments to Url.Path as a barrier

This commit is contained in:
Sauyon Lee
2019-11-15 01:25:36 -08:00
parent 8cc60ba543
commit 4c9bf2265e
2 changed files with 36 additions and 1 deletions

View File

@@ -50,6 +50,41 @@ module OpenUrlRedirect {
}
}
/**
* Holds if `a` and `b` read the same variable, field, or method.
*/
predicate readsSameEntity(Read a, Read b) {
exists(DataFlow::Node aBase, DataFlow::Node bBase | readsSameEntity(aBase, bBase) |
exists(Field f | a.readsField(aBase, f) | b.readsField(bBase, f))
or
exists(Method m | a.readsMethod(aBase, m) | b.readsMethod(bBase, m))
)
}
/**
* An access to a variable that is preceded by an assignment to its `Path` field.
*
* This is overapproximate; this will currently remove flow through all `Url.Path` assignments
* which contain a substring that could sanitize data.
*/
class PathAssignmentBarrier extends Barrier, Read {
PathAssignmentBarrier() {
exists(Write w, Field f, Read writeBase, ValueEntity v |
f.getName() = "Path" and
hasHostnameSanitizingSubstring(w.getRhs()) and
readsSameEntity(this, writeBase)
|
w.writesField(writeBase, f, _) and
w.getBasicBlock().(ReachableBasicBlock).dominates(this.asInstruction().getBasicBlock()) and
(
not w.getBasicBlock() = this.asInstruction().getBasicBlock()
or
w.getASuccessor+() = this.asInstruction()
)
)
}
}
/**
* A call to a function called `isLocalUrl` or similar, which is
* considered a barrier for purposes of URL redirection.

View File

@@ -55,7 +55,7 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
* In the latter two cases, the additional check is necessary to avoid a `/` that could be interpreted as
* the `//` separating the (optional) scheme from the hostname.
*/
private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
nd.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*")
or
hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))