From bf0f0aff5ed303ed5cbc998a3b3be2b3e80cb3a7 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 25 Jan 2021 17:06:51 +0000 Subject: [PATCH] Move reused barrier guards into separate files This way only the barrier guards that are used will be imported. This is important because of the comment above BarrierGuard, which warns about the potential danger of having classes that extend BarrierGuard in scope which are not used. --- .../semmle/go/dataflow/BarrierGuardUtil.qll | 68 ------------------- .../RedirectCheckBarrierGuard.qll | 21 ++++++ .../dataflow/barrierguardutil/RegexpCheck.qll | 25 +++++++ .../go/dataflow/barrierguardutil/UrlCheck.qll | 32 +++++++++ .../OpenUrlRedirectCustomizations.qll | 4 +- .../security/RequestForgeryCustomizations.qll | 4 +- .../go/security/TaintedPathCustomizations.qll | 2 +- 7 files changed, 85 insertions(+), 71 deletions(-) delete mode 100644 ql/src/semmle/go/dataflow/BarrierGuardUtil.qll create mode 100644 ql/src/semmle/go/dataflow/barrierguardutil/RedirectCheckBarrierGuard.qll create mode 100644 ql/src/semmle/go/dataflow/barrierguardutil/RegexpCheck.qll create mode 100644 ql/src/semmle/go/dataflow/barrierguardutil/UrlCheck.qll diff --git a/ql/src/semmle/go/dataflow/BarrierGuardUtil.qll b/ql/src/semmle/go/dataflow/BarrierGuardUtil.qll deleted file mode 100644 index 89f7cb29555..00000000000 --- a/ql/src/semmle/go/dataflow/BarrierGuardUtil.qll +++ /dev/null @@ -1,68 +0,0 @@ -/** - * Provides implementations of some commonly used barrier guards for sanitizing untrusted URLs. - */ - -import go - -/** - * A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is - * considered a barrier guard for sanitizing untrusted URLs. - */ -class RedirectCheckBarrierGuard extends DataFlow::BarrierGuard, DataFlow::CallNode { - RedirectCheckBarrierGuard() { - this.getCalleeName().regexpMatch("(?i)(is_?)?(local_?url|valid_?redir(ect)?)(ur[li])?") - } - - override predicate checks(Expr e, boolean outcome) { - // `isLocalUrl(e)` is a barrier for `e` if it evaluates to `true` - getAnArgument().asExpr() = e and - outcome = true - } -} - -/** - * An equality check comparing a data-flow node against a constant string, considered as - * a barrier guard for sanitizing untrusted URLs. - * - * Additionally, a check comparing `url.Hostname()` against a constant string is also - * considered a barrier guard for `url`. - */ -class UrlCheck extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode { - DataFlow::Node url; - - UrlCheck() { - exists(this.getAnOperand().getStringValue()) and - ( - url = this.getAnOperand() - or - exists(DataFlow::MethodCallNode mc | mc = this.getAnOperand() | - mc.getTarget().getName() = "Hostname" and - url = mc.getReceiver() - ) - ) - } - - override predicate checks(Expr e, boolean outcome) { - e = url.asExpr() and outcome = this.getPolarity() - } -} - -/** - * A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs. - * - * This is overapproximate: we do not attempt to reason about the correctness of the regexp. - */ -class RegexpCheck extends DataFlow::BarrierGuard { - RegexpMatchFunction matchfn; - DataFlow::CallNode call; - - RegexpCheck() { - matchfn.getACall() = call and - this = matchfn.getResult().getNode(call).getASuccessor*() - } - - override predicate checks(Expr e, boolean branch) { - e = matchfn.getValue().getNode(call).asExpr() and - (branch = false or branch = true) - } -} diff --git a/ql/src/semmle/go/dataflow/barrierguardutil/RedirectCheckBarrierGuard.qll b/ql/src/semmle/go/dataflow/barrierguardutil/RedirectCheckBarrierGuard.qll new file mode 100644 index 00000000000..4876c890612 --- /dev/null +++ b/ql/src/semmle/go/dataflow/barrierguardutil/RedirectCheckBarrierGuard.qll @@ -0,0 +1,21 @@ +/** + * Provides an implementation of a commonly used barrier guard for sanitizing untrusted URLs. + */ + +import go + +/** + * A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is + * considered a barrier guard for sanitizing untrusted URLs. + */ +class RedirectCheckBarrierGuard extends DataFlow::BarrierGuard, DataFlow::CallNode { + RedirectCheckBarrierGuard() { + this.getCalleeName().regexpMatch("(?i)(is_?)?(local_?url|valid_?redir(ect)?)(ur[li])?") + } + + override predicate checks(Expr e, boolean outcome) { + // `isLocalUrl(e)` is a barrier for `e` if it evaluates to `true` + getAnArgument().asExpr() = e and + outcome = true + } +} diff --git a/ql/src/semmle/go/dataflow/barrierguardutil/RegexpCheck.qll b/ql/src/semmle/go/dataflow/barrierguardutil/RegexpCheck.qll new file mode 100644 index 00000000000..f78428c7abe --- /dev/null +++ b/ql/src/semmle/go/dataflow/barrierguardutil/RegexpCheck.qll @@ -0,0 +1,25 @@ +/** + * Provides an implementation of a commonly used barrier guard for sanitizing untrusted URLs. + */ + +import go + +/** + * A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs. + * + * This is overapproximate: we do not attempt to reason about the correctness of the regexp. + */ +class RegexpCheck extends DataFlow::BarrierGuard { + RegexpMatchFunction matchfn; + DataFlow::CallNode call; + + RegexpCheck() { + matchfn.getACall() = call and + this = matchfn.getResult().getNode(call).getASuccessor*() + } + + override predicate checks(Expr e, boolean branch) { + e = matchfn.getValue().getNode(call).asExpr() and + (branch = false or branch = true) + } +} diff --git a/ql/src/semmle/go/dataflow/barrierguardutil/UrlCheck.qll b/ql/src/semmle/go/dataflow/barrierguardutil/UrlCheck.qll new file mode 100644 index 00000000000..8aefc67ee38 --- /dev/null +++ b/ql/src/semmle/go/dataflow/barrierguardutil/UrlCheck.qll @@ -0,0 +1,32 @@ +/** + * Provides an implementation of a commonly used barrier guard for sanitizing untrusted URLs. + */ + +import go + +/** + * An equality check comparing a data-flow node against a constant string, considered as + * a barrier guard for sanitizing untrusted URLs. + * + * Additionally, a check comparing `url.Hostname()` against a constant string is also + * considered a barrier guard for `url`. + */ +class UrlCheck extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode { + DataFlow::Node url; + + UrlCheck() { + exists(this.getAnOperand().getStringValue()) and + ( + url = this.getAnOperand() + or + exists(DataFlow::MethodCallNode mc | mc = this.getAnOperand() | + mc.getTarget().getName() = "Hostname" and + url = mc.getReceiver() + ) + ) + } + + override predicate checks(Expr e, boolean outcome) { + e = url.asExpr() and outcome = this.getPolarity() + } +} diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 586f15b4c42..c8b23ac0e66 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -7,7 +7,9 @@ import go import UrlConcatenation import SafeUrlFlowCustomizations -import semmle.go.dataflow.BarrierGuardUtil +import semmle.go.dataflow.barrierguardutil.RedirectCheckBarrierGuard +import semmle.go.dataflow.barrierguardutil.RegexpCheck +import semmle.go.dataflow.barrierguardutil.UrlCheck /** * Provides extension points for customizing the taint-tracking configuration for reasoning about diff --git a/ql/src/semmle/go/security/RequestForgeryCustomizations.qll b/ql/src/semmle/go/security/RequestForgeryCustomizations.qll index 19a0852d2a7..5d294aa74f3 100644 --- a/ql/src/semmle/go/security/RequestForgeryCustomizations.qll +++ b/ql/src/semmle/go/security/RequestForgeryCustomizations.qll @@ -5,7 +5,9 @@ import go import UrlConcatenation import SafeUrlFlowCustomizations -import semmle.go.dataflow.BarrierGuardUtil +import semmle.go.dataflow.barrierguardutil.RedirectCheckBarrierGuard +import semmle.go.dataflow.barrierguardutil.RegexpCheck +import semmle.go.dataflow.barrierguardutil.UrlCheck /** Provides classes and predicates for the request forgery query. */ module RequestForgery { diff --git a/ql/src/semmle/go/security/TaintedPathCustomizations.qll b/ql/src/semmle/go/security/TaintedPathCustomizations.qll index 8430b6b0153..e51fd12a97d 100644 --- a/ql/src/semmle/go/security/TaintedPathCustomizations.qll +++ b/ql/src/semmle/go/security/TaintedPathCustomizations.qll @@ -4,7 +4,7 @@ */ import go -import semmle.go.dataflow.BarrierGuardUtil +import semmle.go.dataflow.barrierguardutil.RegexpCheck /** * Provides extension points for customizing the taint tracking configuration for reasoning about