diff --git a/change-notes/2021-04-19-http-request-taint-flow.md b/change-notes/2021-04-19-http-request-taint-flow.md new file mode 100644 index 00000000000..08eb761e0db --- /dev/null +++ b/change-notes/2021-04-19-http-request-taint-flow.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* `net/http.Request` and `mime/multipart.Part`'s models have been improved. `Request`'s error returns are no longer considered tainted, and `Part`'s methods propagate taint (for example, the `Part.FileName()` of a tainted `Part` is itself tainted). This should lead to more accurate results from any query where `Request` or `Part` methods occurred in a taint-flow path. diff --git a/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll b/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll index 074e848f74b..9de15b824be 100644 --- a/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll +++ b/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll @@ -34,6 +34,14 @@ module MimeMultipart { hasQualifiedName("mime/multipart", "FileHeader", "Open") and (inp.isReceiver() and outp.isResult(0)) or + // signature: func (*Part).FileName() string + hasQualifiedName("mime/multipart", "Part", "FileName") and + (inp.isReceiver() and outp.isResult(0)) + or + // signature: func (*Part).FormName() string + hasQualifiedName("mime/multipart", "Part", "FormName") and + (inp.isReceiver() and outp.isResult(0)) + or // signature: func (*Reader).NextPart() (*Part, error) hasQualifiedName("mime/multipart", "Reader", "NextPart") and (inp.isReceiver() and outp.isResult(0)) diff --git a/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll b/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll index 2a12e046bfb..cb38284d116 100644 --- a/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll +++ b/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll @@ -23,18 +23,20 @@ module NetHttp { } } - private class UserControlledRequestMethod extends UntrustedFlowSource::Range, - DataFlow::MethodCallNode { + private class UserControlledRequestMethod extends UntrustedFlowSource::Range { 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" + exists(DataFlow::MethodCallNode callNode, string methName, int resultIdx | + callNode.getTarget().hasQualifiedName("net/http", "Request", methName) and + this = callNode.getResult(resultIdx) + | + methName = + [ + "Cookie", "Cookies", "FormValue", "MultipartReader", "PostFormValue", "Referer", + "UserAgent" + ] and + resultIdx = 0 + or + methName = "FormFile" and resultIdx = [0, 1] ) } } @@ -255,7 +257,7 @@ module NetHttp { or exists(Method m, string methName | m.hasQualifiedName("net/http", "Request", methName) and - this = m.getACall() + this = m.getACall().getResult(0) | methName = ["Cookie", "Cookies", "MultipartReader", "PostFormValue", "Referer", "UserAgent"] ) diff --git a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected b/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected index ccc5d52013c..1365b68890a 100644 --- a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected +++ b/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected @@ -1,6 +1,6 @@ edges -| test.go:10:15:10:42 | call to Cookie : tuple type | test.go:14:15:14:55 | type conversion | -| test.go:10:15:10:42 | call to Cookie : tuple type | test.go:14:42:14:47 | implicit dereference : Cookie | +| test.go:10:2:10:42 | ... := ...[0] : pointer type | test.go:14:15:14:55 | type conversion | +| test.go:10:2:10:42 | ... := ...[0] : pointer type | test.go:14:42:14:47 | implicit dereference : Cookie | | test.go:14:42:14:47 | implicit dereference : Cookie | test.go:14:15:14:55 | type conversion | | test.go:14:42:14:47 | implicit dereference : Cookie | test.go:14:42:14:47 | implicit dereference : Cookie | | test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:15:17:31 | type conversion | @@ -29,7 +29,7 @@ edges | test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:36:22:36:38 | call to Token : Token | | test.go:36:22:36:38 | call to Token : Token | test.go:36:15:36:44 | type conversion | nodes -| test.go:10:15:10:42 | call to Cookie : tuple type | semmle.label | call to Cookie : tuple type | +| test.go:10:2:10:42 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | | test.go:14:15:14:55 | type conversion | semmle.label | type conversion | | test.go:14:42:14:47 | implicit dereference : Cookie | semmle.label | implicit dereference : Cookie | | test.go:16:24:16:35 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser | @@ -53,7 +53,7 @@ nodes | test.go:36:15:36:44 | type conversion | semmle.label | type conversion | | test.go:36:22:36:38 | call to Token : Token | semmle.label | call to Token : Token | #select -| test.go:14:15:14:55 | type conversion | test.go:10:15:10:42 | call to Cookie : tuple type | test.go:14:15:14:55 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:10:15:10:42 | call to Cookie | user-provided value | test.go:0:0:0:0 | test.go | | +| test.go:14:15:14:55 | type conversion | test.go:10:2:10:42 | ... := ...[0] : pointer type | test.go:14:15:14:55 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:10:2:10:42 | ... := ...[0] | user-provided value | test.go:0:0:0:0 | test.go | | | test.go:17:15:17:31 | type conversion | test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:15:17:31 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:16:24:16:35 | selection of Body | user-provided value | test.go:0:0:0:0 | test.go | | | test.go:20:15:20:32 | type conversion | test.go:19:36:19:47 | selection of Body : ReadCloser | test.go:20:15:20:32 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:19:36:19:47 | selection of Body | user-provided value | test.go:0:0:0:0 | test.go | | | test.go:23:15:23:35 | type conversion | test.go:22:33:22:44 | selection of Body : ReadCloser | test.go:23:15:23:35 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:22:33:22:44 | selection of Body | user-provided value | test.go:0:0:0:0 | test.go | | diff --git a/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/ql/test/query-tests/Security/CWE-022/TaintedPath.expected index 39e4380b136..8793ee4fa15 100644 --- a/ql/test/query-tests/Security/CWE-022/TaintedPath.expected +++ b/ql/test/query-tests/Security/CWE-022/TaintedPath.expected @@ -1,18 +1,18 @@ edges | TaintedPath.go:10:10:10:14 | selection of URL : pointer type | TaintedPath.go:13:29:13:32 | path | | TaintedPath.go:10:10:10:14 | selection of URL : pointer type | TaintedPath.go:17:28:17:61 | call to Join | -| tst.go:14:22:14:39 | call to FormFile : tuple type | tst.go:17:41:17:47 | implicit dereference : FileHeader | -| tst.go:14:22:14:39 | call to FormFile : tuple type | tst.go:17:41:17:56 | selection of Filename | +| tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:47 | implicit dereference : FileHeader | +| tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename | | tst.go:17:41:17:47 | implicit dereference : FileHeader | tst.go:17:41:17:47 | implicit dereference : FileHeader | | tst.go:17:41:17:47 | implicit dereference : FileHeader | tst.go:17:41:17:56 | selection of Filename | nodes | TaintedPath.go:10:10:10:14 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | | TaintedPath.go:13:29:13:32 | path | semmle.label | path | | TaintedPath.go:17:28:17:61 | call to Join | semmle.label | call to Join | -| tst.go:14:22:14:39 | call to FormFile : tuple type | semmle.label | call to FormFile : tuple type | +| tst.go:14:2:14:39 | ... := ...[1] : pointer type | semmle.label | ... := ...[1] : pointer type | | tst.go:17:41:17:47 | implicit dereference : FileHeader | semmle.label | implicit dereference : FileHeader | | tst.go:17:41:17:56 | selection of Filename | semmle.label | selection of Filename | #select | TaintedPath.go:13:29:13:32 | path | TaintedPath.go:10:10:10:14 | selection of URL : pointer type | TaintedPath.go:13:29:13:32 | path | This path depends on $@. | TaintedPath.go:10:10:10:14 | selection of URL | a user-provided value | | TaintedPath.go:17:28:17:61 | call to Join | TaintedPath.go:10:10:10:14 | selection of URL : pointer type | TaintedPath.go:17:28:17:61 | call to Join | This path depends on $@. | TaintedPath.go:10:10:10:14 | selection of URL | a user-provided value | -| tst.go:17:41:17:56 | selection of Filename | tst.go:14:22:14:39 | call to FormFile : tuple type | tst.go:17:41:17:56 | selection of Filename | This path depends on $@. | tst.go:14:22:14:39 | call to FormFile | a user-provided value | +| tst.go:17:41:17:56 | selection of Filename | tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename | This path depends on $@. | tst.go:14:2:14:39 | ... := ...[1] | a user-provided value | diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index bfcc05d5abf..1d06f2560e5 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -1,5 +1,13 @@ edges -| ReflectedXss.go:12:15:12:20 | selection of Form : Values | ReflectedXss.go:15:44:15:51 | username | +| ReflectedXss.go:13:15:13:20 | selection of Form : Values | ReflectedXss.go:16:44:16:51 | username | +| ReflectedXss.go:48:2:48:38 | ... := ...[0] : pointer type | ReflectedXss.go:49:10:49:57 | type conversion | +| ReflectedXss.go:52:2:52:44 | ... := ...[0] : File | ReflectedXss.go:54:10:54:57 | type conversion | +| ReflectedXss.go:52:2:52:44 | ... := ...[1] : pointer type | ReflectedXss.go:55:10:55:62 | type conversion | +| ReflectedXss.go:52:2:52:44 | ... := ...[1] : pointer type | ReflectedXss.go:55:46:55:51 | implicit dereference : FileHeader | +| ReflectedXss.go:55:46:55:51 | implicit dereference : FileHeader | ReflectedXss.go:55:10:55:62 | type conversion | +| ReflectedXss.go:55:46:55:51 | implicit dereference : FileHeader | ReflectedXss.go:55:46:55:51 | implicit dereference : FileHeader | +| ReflectedXss.go:59:2:59:35 | ... := ...[0] : pointer type | ReflectedXss.go:65:10:65:55 | type conversion | +| ReflectedXss.go:59:2:59:35 | ... := ...[0] : pointer type | ReflectedXss.go:66:10:66:18 | byteSlice | | contenttype.go:11:11:11:16 | selection of Form : Values | contenttype.go:17:11:17:22 | type conversion | | contenttype.go:49:11:49:16 | selection of Form : Values | contenttype.go:53:34:53:37 | data | | contenttype.go:63:10:63:28 | call to FormValue : string | contenttype.go:64:52:64:55 | data | @@ -15,8 +23,18 @@ edges | websocketXss.go:50:3:50:10 | definition of gorilla2 : slice type | websocketXss.go:52:24:52:31 | gorilla2 | | websocketXss.go:54:3:54:38 | ... := ...[1] : slice type | websocketXss.go:55:24:55:31 | gorilla3 | nodes -| ReflectedXss.go:12:15:12:20 | selection of Form : Values | semmle.label | selection of Form : Values | -| ReflectedXss.go:15:44:15:51 | username | semmle.label | username | +| ReflectedXss.go:13:15:13:20 | selection of Form : Values | semmle.label | selection of Form : Values | +| ReflectedXss.go:16:44:16:51 | username | semmle.label | username | +| ReflectedXss.go:48:2:48:38 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | +| ReflectedXss.go:49:10:49:57 | type conversion | semmle.label | type conversion | +| ReflectedXss.go:52:2:52:44 | ... := ...[0] : File | semmle.label | ... := ...[0] : File | +| ReflectedXss.go:52:2:52:44 | ... := ...[1] : pointer type | semmle.label | ... := ...[1] : pointer type | +| ReflectedXss.go:54:10:54:57 | type conversion | semmle.label | type conversion | +| ReflectedXss.go:55:10:55:62 | type conversion | semmle.label | type conversion | +| ReflectedXss.go:55:46:55:51 | implicit dereference : FileHeader | semmle.label | implicit dereference : FileHeader | +| ReflectedXss.go:59:2:59:35 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | +| ReflectedXss.go:65:10:65:55 | type conversion | semmle.label | type conversion | +| ReflectedXss.go:66:10:66:18 | byteSlice | semmle.label | byteSlice | | contenttype.go:11:11:11:16 | selection of Form : Values | semmle.label | selection of Form : Values | | contenttype.go:17:11:17:22 | type conversion | semmle.label | type conversion | | contenttype.go:49:11:49:16 | selection of Form : Values | semmle.label | selection of Form : Values | @@ -46,7 +64,12 @@ nodes | websocketXss.go:54:3:54:38 | ... := ...[1] : slice type | semmle.label | ... := ...[1] : slice type | | websocketXss.go:55:24:55:31 | gorilla3 | semmle.label | gorilla3 | #select -| ReflectedXss.go:15:44:15:51 | username | ReflectedXss.go:12:15:12:20 | selection of Form : Values | ReflectedXss.go:15:44:15:51 | username | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:12:15:12:20 | selection of Form | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | +| ReflectedXss.go:16:44:16:51 | username | ReflectedXss.go:13:15:13:20 | selection of Form : Values | ReflectedXss.go:16:44:16:51 | username | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:13:15:13:20 | selection of Form | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | +| ReflectedXss.go:49:10:49:57 | type conversion | ReflectedXss.go:48:2:48:38 | ... := ...[0] : pointer type | ReflectedXss.go:49:10:49:57 | type conversion | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:48:2:48:38 | ... := ...[0] | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | +| ReflectedXss.go:54:10:54:57 | type conversion | ReflectedXss.go:52:2:52:44 | ... := ...[0] : File | ReflectedXss.go:54:10:54:57 | type conversion | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:52:2:52:44 | ... := ...[0] | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | +| ReflectedXss.go:55:10:55:62 | type conversion | ReflectedXss.go:52:2:52:44 | ... := ...[1] : pointer type | ReflectedXss.go:55:10:55:62 | type conversion | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:52:2:52:44 | ... := ...[1] | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | +| ReflectedXss.go:65:10:65:55 | type conversion | ReflectedXss.go:59:2:59:35 | ... := ...[0] : pointer type | ReflectedXss.go:65:10:65:55 | type conversion | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:59:2:59:35 | ... := ...[0] | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | +| ReflectedXss.go:66:10:66:18 | byteSlice | ReflectedXss.go:59:2:59:35 | ... := ...[0] : pointer type | ReflectedXss.go:66:10:66:18 | byteSlice | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:59:2:59:35 | ... := ...[0] | user-provided value | ReflectedXss.go:0:0:0:0 | ReflectedXss.go | | | contenttype.go:17:11:17:22 | type conversion | contenttype.go:11:11:11:16 | selection of Form : Values | contenttype.go:17:11:17:22 | type conversion | Cross-site scripting vulnerability due to $@. | contenttype.go:11:11:11:16 | selection of Form | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | | | contenttype.go:53:34:53:37 | data | contenttype.go:49:11:49:16 | selection of Form : Values | contenttype.go:53:34:53:37 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:49:11:49:16 | selection of Form | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | | | contenttype.go:64:52:64:55 | data | contenttype.go:63:10:63:28 | call to FormValue : string | contenttype.go:64:52:64:55 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:63:10:63:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | | diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go index 8f1e697846c..7111ad462f8 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "fmt" + "io/ioutil" "net/http" ) @@ -41,3 +42,29 @@ func ServeJsonDirect(w http.ResponseWriter, r http.Request) { w.Write(noLongerTainted) } + +func ErrTest(w http.ResponseWriter, r http.Request) { + + cookie, err := r.Cookie("somecookie") + w.Write([]byte(fmt.Sprintf("Cookie result: %v", cookie))) // BAD: Cookie's value is user-controlled + w.Write([]byte(fmt.Sprintf("Cookie check error: %v", err))) // GOOD: Cookie's err return is harmless + + file, header, err := r.FormFile("someFile") + content, err2 := ioutil.ReadAll(file) + w.Write([]byte(fmt.Sprintf("File content: %v", content))) // BAD: file content is user-controlled + w.Write([]byte(fmt.Sprintf("File name: %v", header.Filename))) // BAD: file header is user-controlled + w.Write([]byte(fmt.Sprintf("FormFile error: %v", err))) // GOOD: FormFile's err return is harmless + w.Write([]byte(fmt.Sprintf("FormFile error: %v", err2))) // GOOD: ReadAll's err return is harmless + + reader, err := r.MultipartReader() + part, err2 := reader.NextPart() + partName := part.FileName() + byteSlice := make([]byte, 100) + part.Read(byteSlice) + + w.Write([]byte(fmt.Sprintf("Part name: %v", partName))) // BAD: part name is user-controlled + w.Write(byteSlice) // BAD: part contents are user-controlled + w.Write([]byte(fmt.Sprintf("MultipartReader error: %v", err))) // GOOD: MultipartReader's err return is harmless + w.Write([]byte(fmt.Sprintf("MultipartReader error: %v", err2))) // GOOD: NextPart's err return is harmless + +}