diff --git a/ql/src/semmle/go/frameworks/HTTP.qll b/ql/src/semmle/go/frameworks/HTTP.qll index d6554b25f49..e657b20a1ad 100644 --- a/ql/src/semmle/go/frameworks/HTTP.qll +++ b/ql/src/semmle/go/frameworks/HTTP.qll @@ -10,7 +10,30 @@ private module StdlibHttp { DataFlow::FieldReadNode { UserControlledRequestField() { exists(string fieldName | this.getField().hasQualifiedName("net/http", "Request", fieldName) | - fieldName = "Body" or fieldName = "Form" or fieldName = "Header" or fieldName = "URL" + fieldName = "Body" or + fieldName = "GetBody" or + fieldName = "Form" or + fieldName = "PostForm" or + fieldName = "MultipartForm" or + fieldName = "Header" or + fieldName = "Trailer" or + fieldName = "URL" + ) + } + } + + private class UserControlledRequestMethod extends UntrustedFlowSource::Range, + DataFlow::MethodCallNode { + UserControlledRequestMethod() { + exists(string methName | this.getTarget().hasQualifiedName("net/http", "Request", methName) | + methName = "Cookie" or + methName = "Cookies" or + methName = "FormFile" or + methName = "FormValue" or + methName = "MultipartReader" or + methName = "PostFormValue" or + methName = "Referer" or + methName = "UserAgent" ) } } diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index c50e858fb40..ec496aca063 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -95,6 +95,19 @@ module Fmt { } } +/** Provides models of commonly used functions in the `io` package. */ +module Io { + private class ReaderRead extends TaintTracking::FunctionModel, Method { + ReaderRead() { + exists(Method im | im.hasQualifiedName("io", "Reader", "Read") | this.implements(im)) + } + + override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { + inp.isReceiver() and outp.isParameter(0) + } + } +} + /** Provides models of commonly used functions in the `io/ioutil` package. */ module IoUtil { private class IoUtilFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode { diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 18071ad8a75..5dee255d104 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -39,29 +39,36 @@ module OpenUrlRedirect { UnsafeUrlMethod() { this.getName() = "Query" } } - /** - * 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. */ class UntrustedFlowAsSource extends Source, UntrustedFlowSource { UntrustedFlowAsSource() { - // exclude request headers, as they are generally not attacker-controllable for open redirect - // exploits - not this - .(DataFlow::FieldReadNode) - .getField() - .hasQualifiedName("net/http", "Request", "Header") + // exclude some fields and methods of URLs that are generally not attacker-controllable for + // open redirect exploits + not exists(Field f, string fieldName | + f.hasQualifiedName("net/http", "Request", fieldName) and + this = f.getARead() + | + fieldName = "Body" or + fieldName = "GetBody" or + fieldName = "PostForm" or + fieldName = "MultipartForm" or + fieldName = "Header" or + fieldName = "Trailer" + ) and + not exists(Method m, string methName | + m.hasQualifiedName("net/http", "Request", methName) and + this = m.getACall() + | + methName = "Cookie" or + methName = "Cookies" or + methName = "FormValue" or + methName = "MultipartReader" or + methName = "PostFormValues" or + methName = "Referer" or + methName = "UserAgent" + ) } } diff --git a/ql/test/library-tests/semmle/go/frameworks/HTTP/UntrustedFlowSources.expected b/ql/test/library-tests/semmle/go/frameworks/HTTP/UntrustedFlowSources.expected index 81869d09829..f2f61551504 100644 --- a/ql/test/library-tests/semmle/go/frameworks/HTTP/UntrustedFlowSources.expected +++ b/ql/test/library-tests/semmle/go/frameworks/HTTP/UntrustedFlowSources.expected @@ -13,3 +13,4 @@ | server.go:9:6:9:13 | selection of Header | | server.go:10:6:10:13 | selection of Header | | server.go:13:6:13:11 | selection of Body | +| server.go:14:15:14:23 | selection of GetBody | diff --git a/ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected b/ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected index 242e8ec4d37..d912d745d7a 100644 --- a/ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected +++ b/ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected @@ -5,19 +5,23 @@ | crypto.go:11:18:11:57 | call to Open | crypto.go:11:2:11:57 | ... := ...[0] | | crypto.go:11:18:11:57 | call to Open | crypto.go:11:2:11:57 | ... := ...[1] | | crypto.go:11:42:11:51 | ciphertext | crypto.go:11:2:11:57 | ... := ...[0] | -| main.go:10:12:10:26 | call to Marshal | main.go:10:2:10:26 | ... := ...[0] | -| main.go:10:12:10:26 | call to Marshal | main.go:10:2:10:26 | ... := ...[1] | -| main.go:10:25:10:25 | v | main.go:10:2:10:26 | ... := ...[0] | -| main.go:12:14:12:52 | call to MarshalIndent | main.go:12:2:12:52 | ... := ...[0] | -| main.go:12:14:12:52 | call to MarshalIndent | main.go:12:2:12:52 | ... := ...[1] | -| main.go:12:33:12:33 | v | main.go:12:2:12:52 | ... := ...[0] | -| main.go:13:25:13:25 | b | main.go:13:9:13:41 | composite literal | -| main.go:13:28:13:30 | err | main.go:13:9:13:41 | composite literal | -| main.go:13:33:13:34 | b2 | main.go:13:9:13:41 | composite literal | -| main.go:13:37:13:40 | err2 | main.go:13:9:13:41 | composite literal | -| main.go:18:18:18:42 | call to DecodeString | main.go:18:2:18:42 | ... := ...[0] | -| main.go:18:18:18:42 | call to DecodeString | main.go:18:2:18:42 | ... := ...[1] | -| main.go:18:35:18:41 | encoded | main.go:18:2:18:42 | ... := ...[0] | -| main.go:22:25:22:31 | decoded | main.go:22:9:22:48 | composite literal | -| main.go:22:34:22:36 | err | main.go:22:9:22:48 | composite literal | -| main.go:22:39:22:47 | reEncoded | main.go:22:9:22:48 | composite literal | +| main.go:11:12:11:26 | call to Marshal | main.go:11:2:11:26 | ... := ...[0] | +| main.go:11:12:11:26 | call to Marshal | main.go:11:2:11:26 | ... := ...[1] | +| main.go:11:25:11:25 | v | main.go:11:2:11:26 | ... := ...[0] | +| main.go:13:14:13:52 | call to MarshalIndent | main.go:13:2:13:52 | ... := ...[0] | +| main.go:13:14:13:52 | call to MarshalIndent | main.go:13:2:13:52 | ... := ...[1] | +| main.go:13:33:13:33 | v | main.go:13:2:13:52 | ... := ...[0] | +| main.go:14:25:14:25 | b | main.go:14:9:14:41 | composite literal | +| main.go:14:28:14:30 | err | main.go:14:9:14:41 | composite literal | +| main.go:14:33:14:34 | b2 | main.go:14:9:14:41 | composite literal | +| main.go:14:37:14:40 | err2 | main.go:14:9:14:41 | composite literal | +| main.go:19:18:19:42 | call to DecodeString | main.go:19:2:19:42 | ... := ...[0] | +| main.go:19:18:19:42 | call to DecodeString | main.go:19:2:19:42 | ... := ...[1] | +| main.go:19:35:19:41 | encoded | main.go:19:2:19:42 | ... := ...[0] | +| main.go:23:25:23:31 | decoded | main.go:23:9:23:48 | composite literal | +| main.go:23:34:23:36 | err | main.go:23:9:23:48 | composite literal | +| main.go:23:39:23:47 | reEncoded | main.go:23:9:23:48 | composite literal | +| main.go:28:2:28:4 | implicit dereference | main.go:26:15:26:17 | definition of req | +| main.go:28:2:28:4 | implicit dereference | main.go:28:2:28:9 | selection of Body | +| main.go:28:2:28:4 | req | main.go:28:2:28:4 | implicit dereference | +| main.go:28:2:28:9 | selection of Body | main.go:27:2:27:2 | definition of b | diff --git a/ql/test/library-tests/semmle/go/frameworks/TaintSteps/main.go b/ql/test/library-tests/semmle/go/frameworks/TaintSteps/main.go index cd8ffe20f16..79e7040c296 100644 --- a/ql/test/library-tests/semmle/go/frameworks/TaintSteps/main.go +++ b/ql/test/library-tests/semmle/go/frameworks/TaintSteps/main.go @@ -3,6 +3,7 @@ package main import ( "encoding/hex" "encoding/json" + "net/http" ) func jsonTest(v interface{}) []interface{} { @@ -21,3 +22,9 @@ func hexTest(encoded string) []interface{} { reEncoded := hex.EncodeToString(decoded) return [](interface{}){decoded, err, reEncoded} } + +func readTest(req *http.Request) string { + b := make([]byte, 8) + req.Body.Read(b) + return string(b) +}