diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index a69e13d341b..c50e858fb40 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -114,9 +114,7 @@ module IoUtil { * from its first argument to its first result. */ private class ReadAll extends TaintTracking::FunctionModel { - ReadAll() { - hasQualifiedName("io/ioutil", "ReadAll") - } + ReadAll() { hasQualifiedName("io/ioutil", "ReadAll") } override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { inp.isParameter(0) and outp.isResult(0) @@ -201,7 +199,7 @@ module OS { /** Provides models of commonly used functions in the `path` package. */ module Path { /** A path-manipulating function in the `path` package. */ - private class PathManipulatingFunction extends TaintTracking::FunctionModel { + class PathManipulatingFunction extends TaintTracking::FunctionModel { PathManipulatingFunction() { exists(string fn | hasQualifiedName("path", fn) | fn = "Base" or diff --git a/ql/src/semmle/go/security/OpenUrlRedirect.qll b/ql/src/semmle/go/security/OpenUrlRedirect.qll index 9d97669d4d7..e353f60ab2f 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirect.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirect.qll @@ -58,6 +58,8 @@ module OpenUrlRedirect { override predicate isSource(DataFlow::Node source) { source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", "URL") + or + source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", "Host") } override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } @@ -65,9 +67,32 @@ module OpenUrlRedirect { override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { TaintTracking::functionModelStep(any(SafeUrlMethod m), pred, succ) or + TaintTracking::functionModelStep(any(Path::PathManipulatingFunction pmf), pred, succ) + or + TaintTracking::functionModelStep(any(StringRightTrimmer rt), pred, succ) + or + TaintTracking::referenceStep(pred, succ) + or + TaintTracking::stringConcatStep(pred, succ) + or + TaintTracking::tupleStep(pred, succ) + or exists(DataFlow::FieldReadNode frn | succ = frn | - frn.getBase() = pred and frn.getFieldName() = "Host" + frn.getBase() = pred and + (frn.getFieldName() = "Host" or frn.getFieldName() = "Path") + ) + or + exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Path") | + w.writesField(v.getAUse(), f, pred) and succ = v.getAUse() ) } + + override predicate isBarrierOut(DataFlow::Node node) { + exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Path") | + w.writesField(node.getASuccessor(), f, _) + ) + } + + override int explorationLimit() { result = 30 } } } diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 198ced68298..d3ce863ba44 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -39,7 +39,9 @@ module OpenUrlRedirect { SafeUrlMethod() { this instanceof StringMethod or - exists(string m | this.hasQualifiedName("net/url", "URL", m) | m = "Hostname" or m = "Port") + exists(string m | this.hasQualifiedName("net/url", "URL", m) | + m = "Hostname" or m = "Port" or m = "RequestURI" + ) } override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) { @@ -47,6 +49,18 @@ module OpenUrlRedirect { } } + /** + * A function that trims the right hand side of a string, considered to preserve the safeness + * of taint flow from the full request URL. + */ + class StringRightTrimmer extends Strings::Trimmer { + StringRightTrimmer() { + this.hasQualifiedName("strings", "TrimSuffix") or + this.hasQualifiedName("strings", "TrimRight") or + this.hasQualifiedName("strings", "TrimRightFunc") + } + } + /** * A source of third-party user input, considered as a flow source for URL redirects. */ diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go index 7e24a892f83..50f74fc8689 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go @@ -139,5 +139,29 @@ func serveStdlib() { http.Redirect(w, r, target, 302) }) + http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + + // GOOD: Only safe parts of the URL are used + url := *r.URL + if url.Scheme == "http" { + url.Scheme = "https" + http.Redirect(w, r, url.String(), 302) + } else { + // ... + } + }) + + http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + + // GOOD: Only safe parts of the URL are used + if r.URL.Scheme == "http" { + http.Redirect(w, r, "https://"+r.URL.RequestURI(), 302) + } else { + // ... + } + }) + http.ListenAndServe(":80", nil) }