From 541c82a7f3b7a42618a87f5e110d0f06b9184b2e Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 26 Mar 2020 07:20:14 -0700 Subject: [PATCH 1/6] HTTP: Add some more untrusted fields and methods Also, fix up broken tests. --- ql/src/semmle/go/frameworks/HTTP.qll | 25 ++++++++++++++++++- .../HTTP/UntrustedFlowSources.expected | 1 + 2 files changed, 25 insertions(+), 1 deletion(-) 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/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 | From 1f4d67b77bc95905286cfaefd016fecb0f850eea Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 26 Mar 2020 07:20:51 -0700 Subject: [PATCH 2/6] OpenUrlRedirect: Whitelist some more fields and methods --- .../OpenUrlRedirectCustomizations.qll | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 18071ad8a75..8582d73684f 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -39,29 +39,29 @@ 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(string fieldName | + this.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", fieldName) + | + fieldName = "Header" or fieldName = "Trailer" + ) and + not exists(string methName | + this + .(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("net/http", "Request", methName) + | + methName = "Cookie" or + methName = "Cookies" or + methName = "Referer" or + methName = "UserAgent" + ) } } From a4f1e2b52769b6323f73972e1c29d7db82f00b9a Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 26 Mar 2020 18:57:44 -0700 Subject: [PATCH 3/6] Add a model for Read methods on io.Reader --- ql/src/semmle/go/frameworks/Stdlib.qll | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index c50e858fb40..751a199400c 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/ioutil` 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 { From 05761bc2cd060524c8ae8cfcd141842f0b09f7c9 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 04:03:30 -0700 Subject: [PATCH 4/6] Address review comments --- .../OpenUrlRedirectCustomizations.qll | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 8582d73684f..5dee255d104 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -46,19 +46,26 @@ module OpenUrlRedirect { UntrustedFlowAsSource() { // exclude some fields and methods of URLs that are generally not attacker-controllable for // open redirect exploits - not exists(string fieldName | - this.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", fieldName) + not exists(Field f, string fieldName | + f.hasQualifiedName("net/http", "Request", fieldName) and + this = f.getARead() | - fieldName = "Header" or fieldName = "Trailer" + fieldName = "Body" or + fieldName = "GetBody" or + fieldName = "PostForm" or + fieldName = "MultipartForm" or + fieldName = "Header" or + fieldName = "Trailer" ) and - not exists(string methName | - this - .(DataFlow::MethodCallNode) - .getTarget() - .hasQualifiedName("net/http", "Request", methName) + 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" ) From 4747524feeeea2e7bcbab6b02765606cfa8436bc Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 04:15:30 -0700 Subject: [PATCH 5/6] Address review comments Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> --- ql/src/semmle/go/frameworks/Stdlib.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index 751a199400c..ec496aca063 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -95,7 +95,7 @@ module Fmt { } } -/** Provides models of commonly used functions in the `io/ioutil` package. */ +/** Provides models of commonly used functions in the `io` package. */ module Io { private class ReaderRead extends TaintTracking::FunctionModel, Method { ReaderRead() { From 080d14ea50483f16bba65b37cfe08f78588393cc Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 04:21:49 -0700 Subject: [PATCH 6/6] Add a test for the Read taint step --- .../frameworks/TaintSteps/TaintStep.expected | 36 ++++++++++--------- .../semmle/go/frameworks/TaintSteps/main.go | 7 ++++ 2 files changed, 27 insertions(+), 16 deletions(-) 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) +}