hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-01-29 22:32:58 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #527 from smowton/smowton/fix/http-request-taint-tracking

Browse Source 
Improve net/http taint-tracking fidelity
This commit is contained in:
Chris Smowton
2021-04-20 12:40:19 +01:00
committed by GitHub
parent dbcf1e1cfa b2e92fa084
commit f40211bd20
7 changed files with 86 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
change-notes/2021-04-19-http-request-taint-flow.md Normal file
View File

@@ -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.

8
ql/src/semmle/go/frameworks/stdlib/MimeMultipart.qll
View File

@@ -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))

26
ql/src/semmle/go/frameworks/stdlib/NetHttp.qll
View File

@@ -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"]
)

8
ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected
View File

@@ -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 | |

8
ql/test/query-tests/Security/CWE-022/TaintedPath.expected
View File

@@ -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 |

31
ql/test/query-tests/Security/CWE-079/ReflectedXss.expected
View File

@@ -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 | |

27
ql/test/query-tests/Security/CWE-079/ReflectedXss.go
View File

@@ -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
}