diff --git a/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll b/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll index 074e848f74b..b051ad51c50 100644 --- a/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll +++ b/ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll @@ -34,6 +34,18 @@ 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 (*Part).Read([]byte) (int, error) + hasQualifiedName("mime/multipart", "Part", "FormName") and + (inp.isReceiver() and outp.isParameter(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..a581c6d6a2a 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] ) } } 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 + +}