From 0cef5fb5d0c7ddf857ec8fe5d31638f54b284a54 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 20 Apr 2021 19:42:16 +0100 Subject: [PATCH 1/9] Add test case for map extraction --- ql/test/query-tests/Security/CWE-079/ReflectedXss.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go index 7111ad462f8..485a031d862 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go @@ -66,5 +66,12 @@ func ErrTest(w http.ResponseWriter, r http.Request) { 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 - +} + +func QueryMapTest(w http.ResponseWriter, r http.Request) { + keys, ok := r.URL.Query()["data_id"] + if ok && len(keys[0]) > 0 { + key := keys[0] + w.Write([]byte(key)) + } } From ba2da6d9a999fda5c5d9eba7b03af315b55105a1 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 20 Apr 2021 13:43:21 -0700 Subject: [PATCH 2/9] Add test exercising channel data flow --- .../semmle/go/dataflow/FlowSteps/LocalFlowStep.expected | 3 +++ ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected index 97197b9975c..745c997096e 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected @@ -105,6 +105,9 @@ | main.go:48:3:48:11 | ... += ... | main.go:48:3:48:6 | definition of vals | | main.go:49:3:49:6 | definition of keys | main.go:47:20:47:20 | keys = phi(def@46:24, def@49:3) | | main.go:49:3:49:11 | ... += ... | main.go:49:3:49:6 | definition of keys | +| main.go:55:6:55:7 | definition of ch | main.go:56:2:56:3 | ch | +| main.go:55:6:55:7 | definition of ch | main.go:57:4:57:5 | ch | +| main.go:55:6:55:7 | zero value for ch | main.go:55:6:55:7 | definition of ch | | strings.go:8:12:8:12 | argument corresponding to s | strings.go:8:12:8:12 | definition of s | | strings.go:8:12:8:12 | definition of s | strings.go:9:24:9:24 | s | | strings.go:8:12:8:12 | definition of s | strings.go:10:27:10:27 | s | diff --git a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go index d47942ee9e8..94e14f29110 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go +++ b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go @@ -50,3 +50,9 @@ func test10(xs []int) (keys int, vals int) { } return } + +func testch() { + var ch chan bool + ch <- true + <-ch +} From d1daca541e882c84313b290c5d01519ad273fd55 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 20 Apr 2021 13:45:53 -0700 Subject: [PATCH 3/9] Add types for more tuple extractions Specifically, extractions where the RHS is a map element read or a channel receive will now have types. --- ql/src/semmle/go/controlflow/IR.qll | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index f5313794506..ec25b62ad69 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -751,6 +751,18 @@ module IR { result = tae.getType().(TupleType).getComponentType(pragma[only_bind_into](i)) ) or + exists(Instruction mapBase | this.getBase().readsElement(mapBase, _) | + i = 0 and result = mapBase.getResultType().getUnderlyingType().(MapType).getValueType() + or + i = 1 and result = any(BoolExprType b) + ) + or + exists(RecvExpr re | this.getBase() = evalExprInstruction(re) | + i = 0 and result = re.getOperand().getType().getUnderlyingType().(ChanType).getElementType() + or + i = 1 and result = any(BoolExprType b) + ) + or exists(Type rangeType | rangeType = s.(RangeStmt).getDomain().getType().getUnderlyingType() | exists(Type baseType | baseType = rangeType.(ArrayType).getElementType() or From 50bb6187b8b21596421f94d545f129d4c85b1364 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 20 Apr 2021 14:20:22 -0700 Subject: [PATCH 4/9] Revert ReflectedXss.go to example --- .../Security/CWE-079/ReflectedXss.expected | 58 +++++++++--------- .../Security/CWE-079/ReflectedXss.go | 59 +------------------ .../Security/CWE-079/reflectedxsstest.go | 56 ++++++++++++++++++ 3 files changed, 88 insertions(+), 85 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-079/reflectedxsstest.go diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index 1d06f2560e5..1d70fec1ac1 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -1,19 +1,20 @@ edges -| 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 | +| ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username | | 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 | | contenttype.go:73:10:73:28 | call to FormValue : string | contenttype.go:79:11:79:14 | data | | contenttype.go:88:10:88:28 | call to FormValue : string | contenttype.go:91:4:91:7 | data | | contenttype.go:113:10:113:28 | call to FormValue : string | contenttype.go:114:50:114:53 | data | +| reflectedxsstest.go:27:2:27:38 | ... := ...[0] : pointer type | reflectedxsstest.go:28:10:28:57 | type conversion | +| reflectedxsstest.go:31:2:31:44 | ... := ...[0] : File | reflectedxsstest.go:33:10:33:57 | type conversion | +| reflectedxsstest.go:31:2:31:44 | ... := ...[1] : pointer type | reflectedxsstest.go:34:10:34:62 | type conversion | +| reflectedxsstest.go:31:2:31:44 | ... := ...[1] : pointer type | reflectedxsstest.go:34:46:34:51 | implicit dereference : FileHeader | +| reflectedxsstest.go:34:46:34:51 | implicit dereference : FileHeader | reflectedxsstest.go:34:10:34:62 | type conversion | +| reflectedxsstest.go:34:46:34:51 | implicit dereference : FileHeader | reflectedxsstest.go:34:46:34:51 | implicit dereference : FileHeader | +| reflectedxsstest.go:38:2:38:35 | ... := ...[0] : pointer type | reflectedxsstest.go:44:10:44:55 | type conversion | +| reflectedxsstest.go:38:2:38:35 | ... := ...[0] : pointer type | reflectedxsstest.go:45:10:45:18 | byteSlice | +| reflectedxsstest.go:51:14:51:18 | selection of URL : pointer type | reflectedxsstest.go:54:11:54:21 | type conversion | | tst.go:14:15:14:20 | selection of Form : Values | tst.go:18:12:18:39 | type conversion | | tst.go:48:14:48:19 | selection of Form : Values | tst.go:53:12:53:26 | type conversion | | websocketXss.go:30:7:30:10 | definition of xnet : slice type | websocketXss.go:32:24:32:27 | xnet | @@ -23,18 +24,8 @@ 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: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 | +| ReflectedXss.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values | +| ReflectedXss.go:14:44:14:51 | username | semmle.label | username | | 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 | @@ -47,6 +38,18 @@ nodes | contenttype.go:91:4:91:7 | data | semmle.label | data | | contenttype.go:113:10:113:28 | call to FormValue : string | semmle.label | call to FormValue : string | | contenttype.go:114:50:114:53 | data | semmle.label | data | +| reflectedxsstest.go:27:2:27:38 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | +| reflectedxsstest.go:28:10:28:57 | type conversion | semmle.label | type conversion | +| reflectedxsstest.go:31:2:31:44 | ... := ...[0] : File | semmle.label | ... := ...[0] : File | +| reflectedxsstest.go:31:2:31:44 | ... := ...[1] : pointer type | semmle.label | ... := ...[1] : pointer type | +| reflectedxsstest.go:33:10:33:57 | type conversion | semmle.label | type conversion | +| reflectedxsstest.go:34:10:34:62 | type conversion | semmle.label | type conversion | +| reflectedxsstest.go:34:46:34:51 | implicit dereference : FileHeader | semmle.label | implicit dereference : FileHeader | +| reflectedxsstest.go:38:2:38:35 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | +| reflectedxsstest.go:44:10:44:55 | type conversion | semmle.label | type conversion | +| reflectedxsstest.go:45:10:45:18 | byteSlice | semmle.label | byteSlice | +| reflectedxsstest.go:51:14:51:18 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| reflectedxsstest.go:54:11:54:21 | type conversion | semmle.label | type conversion | | tst.go:14:15:14:20 | selection of Form : Values | semmle.label | selection of Form : Values | | tst.go:18:12:18:39 | type conversion | semmle.label | type conversion | | tst.go:48:14:48:19 | selection of Form : Values | semmle.label | selection of Form : Values | @@ -64,18 +67,19 @@ 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: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 | | +| ReflectedXss.go:14:44:14:51 | username | ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:11:15:11:20 | selection of Form | 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 | | | contenttype.go:79:11:79:14 | data | contenttype.go:73:10:73:28 | call to FormValue : string | contenttype.go:79:11:79:14 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:73:10:73:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | | | contenttype.go:91:4:91:7 | data | contenttype.go:88:10:88:28 | call to FormValue : string | contenttype.go:91:4:91:7 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:88:10:88:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | | | contenttype.go:114:50:114:53 | data | contenttype.go:113:10:113:28 | call to FormValue : string | contenttype.go:114:50:114:53 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:113:10:113:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | | +| reflectedxsstest.go:28:10:28:57 | type conversion | reflectedxsstest.go:27:2:27:38 | ... := ...[0] : pointer type | reflectedxsstest.go:28:10:28:57 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:27:2:27:38 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | | +| reflectedxsstest.go:33:10:33:57 | type conversion | reflectedxsstest.go:31:2:31:44 | ... := ...[0] : File | reflectedxsstest.go:33:10:33:57 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:31:2:31:44 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | | +| reflectedxsstest.go:34:10:34:62 | type conversion | reflectedxsstest.go:31:2:31:44 | ... := ...[1] : pointer type | reflectedxsstest.go:34:10:34:62 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:31:2:31:44 | ... := ...[1] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | | +| reflectedxsstest.go:44:10:44:55 | type conversion | reflectedxsstest.go:38:2:38:35 | ... := ...[0] : pointer type | reflectedxsstest.go:44:10:44:55 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:38:2:38:35 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | | +| reflectedxsstest.go:45:10:45:18 | byteSlice | reflectedxsstest.go:38:2:38:35 | ... := ...[0] : pointer type | reflectedxsstest.go:45:10:45:18 | byteSlice | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:38:2:38:35 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | | +| reflectedxsstest.go:54:11:54:21 | type conversion | reflectedxsstest.go:51:14:51:18 | selection of URL : pointer type | reflectedxsstest.go:54:11:54:21 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:51:14:51:18 | selection of URL | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | | | tst.go:18:12:18:39 | type conversion | tst.go:14:15:14:20 | selection of Form : Values | tst.go:18:12:18:39 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:14:15:14:20 | selection of Form | user-provided value | tst.go:0:0:0:0 | tst.go | | | tst.go:53:12:53:26 | type conversion | tst.go:48:14:48:19 | selection of Form : Values | tst.go:53:12:53:26 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:48:14:48:19 | selection of Form | user-provided value | tst.go:0:0:0:0 | tst.go | | | websocketXss.go:32:24:32:27 | xnet | websocketXss.go:30:7:30:10 | definition of xnet : slice type | websocketXss.go:32:24:32:27 | xnet | Cross-site scripting vulnerability due to $@. | websocketXss.go:30:7:30:10 | definition of xnet | user-provided value | websocketXss.go:0:0:0:0 | websocketXss.go | | diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go index 485a031d862..43e5e022598 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go @@ -1,9 +1,7 @@ package main import ( - "encoding/json" "fmt" - "io/ioutil" "net/http" ) @@ -15,63 +13,8 @@ func serve() { // BAD: a request parameter is incorporated without validation into the response fmt.Fprintf(w, "%q is an unknown user", username) } else { - // TODO: do something exciting + // TODO: Handle successful login } }) http.ListenAndServe(":80", nil) } - -func encode(s string) ([]byte, error) { - - return json.Marshal(s) - -} - -func ServeJsonIndirect(w http.ResponseWriter, r http.Request) { - - tainted := r.Header.Get("Origin") - noLongerTainted, _ := encode(tainted) - w.Write(noLongerTainted) - -} - -func ServeJsonDirect(w http.ResponseWriter, r http.Request) { - - tainted := r.Header.Get("Origin") - noLongerTainted, _ := json.Marshal(tainted) - 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 -} - -func QueryMapTest(w http.ResponseWriter, r http.Request) { - keys, ok := r.URL.Query()["data_id"] - if ok && len(keys[0]) > 0 { - key := keys[0] - w.Write([]byte(key)) - } -} diff --git a/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go b/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go new file mode 100644 index 00000000000..9340bc8b189 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go @@ -0,0 +1,56 @@ +package main + +import ( + "encoding/json" + "fmt" + "io/ioutil" + "net/http" +) + +func encode(s string) ([]byte, error) { + return json.Marshal(s) +} + +func ServeJsonIndirect(w http.ResponseWriter, r http.Request) { + tainted := r.Header.Get("Origin") + noLongerTainted, _ := encode(tainted) + w.Write(noLongerTainted) +} + +func ServeJsonDirect(w http.ResponseWriter, r http.Request) { + tainted := r.Header.Get("Origin") + noLongerTainted, _ := json.Marshal(tainted) + 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 +} + +func QueryMapTest(w http.ResponseWriter, r http.Request) { + keys, ok := r.URL.Query()["data_id"] + if ok && len(keys[0]) > 0 { + key := keys[0] + w.Write([]byte(key)) // BAD + } +} From 7efbcec50d8f2f212dc4052f533961cae3e26a9a Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 20 Apr 2021 14:41:02 -0700 Subject: [PATCH 5/9] Add change note --- change-notes/2021-04-20-tuple-types.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2021-04-20-tuple-types.md diff --git a/change-notes/2021-04-20-tuple-types.md b/change-notes/2021-04-20-tuple-types.md new file mode 100644 index 00000000000..422f95255de --- /dev/null +++ b/change-notes/2021-04-20-tuple-types.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Fixed a bug where data flow was not correctly computed through two-value index expressions. This may cause more results from the security queries. From 4fb714f4452569691c2fad0ee1e84d47904b3392 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 21 Apr 2021 12:33:00 +0100 Subject: [PATCH 6/9] Simplify implementation of ExtractTupleElementInstruction.getResultType --- ql/src/semmle/go/controlflow/IR.qll | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index ec25b62ad69..c85d69ee45c 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -747,20 +747,8 @@ module IR { result = c.getTarget().getResultType(i) ) or - exists(TypeAssertExpr tae | this.getBase() = evalExprInstruction(tae) | - result = tae.getType().(TupleType).getComponentType(pragma[only_bind_into](i)) - ) - or - exists(Instruction mapBase | this.getBase().readsElement(mapBase, _) | - i = 0 and result = mapBase.getResultType().getUnderlyingType().(MapType).getValueType() - or - i = 1 and result = any(BoolExprType b) - ) - or - exists(RecvExpr re | this.getBase() = evalExprInstruction(re) | - i = 0 and result = re.getOperand().getType().getUnderlyingType().(ChanType).getElementType() - or - i = 1 and result = any(BoolExprType b) + exists(Expr e | this.getBase() = evalExprInstruction(e) | + result = e.getType().(TupleType).getComponentType(pragma[only_bind_into](i)) ) or exists(Type rangeType | rangeType = s.(RangeStmt).getDomain().getType().getUnderlyingType() | From a152eec9f22f4fa2ef17017ca53619dfa2ff3195 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 21 Apr 2021 12:33:51 +0100 Subject: [PATCH 7/9] Add test for ExtractTupleElementInstruction.getResultType() --- .../library-tests/semmle/go/IR/test.expected | 6 +++++ ql/test/library-tests/semmle/go/IR/test.go | 23 +++++++++++++++++++ ql/test/library-tests/semmle/go/IR/test.ql | 4 ++++ 3 files changed, 33 insertions(+) create mode 100644 ql/test/library-tests/semmle/go/IR/test.expected create mode 100644 ql/test/library-tests/semmle/go/IR/test.go create mode 100644 ql/test/library-tests/semmle/go/IR/test.ql diff --git a/ql/test/library-tests/semmle/go/IR/test.expected b/ql/test/library-tests/semmle/go/IR/test.expected new file mode 100644 index 00000000000..eef7b1f4e9a --- /dev/null +++ b/ql/test/library-tests/semmle/go/IR/test.expected @@ -0,0 +1,6 @@ +| test.go:9:2:9:16 | ... := ...[0] | bool | +| test.go:9:2:9:16 | ... := ...[1] | bool | +| test.go:15:2:15:20 | ... := ...[0] | string | +| test.go:15:2:15:20 | ... := ...[1] | bool | +| test.go:21:2:21:22 | ... := ...[0] | string | +| test.go:21:2:21:22 | ... := ...[1] | bool | diff --git a/ql/test/library-tests/semmle/go/IR/test.go b/ql/test/library-tests/semmle/go/IR/test.go new file mode 100644 index 00000000000..de0d567dc3a --- /dev/null +++ b/ql/test/library-tests/semmle/go/IR/test.go @@ -0,0 +1,23 @@ +package test + +import ( + "fmt" +) + +func testChannel() { + var ch chan bool + got, ok := <-ch + fmt.Printf("%v %v", got, ok) +} + +func testMap() { + var m map[string]string + got, ok := m["key"] + fmt.Printf("%v %v", got, ok) +} + +func testTypeAssert() { + var i interface{} + got, ok := i.(string) + fmt.Printf("%v %v", got, ok) +} diff --git a/ql/test/library-tests/semmle/go/IR/test.ql b/ql/test/library-tests/semmle/go/IR/test.ql new file mode 100644 index 00000000000..1644bd5b2ef --- /dev/null +++ b/ql/test/library-tests/semmle/go/IR/test.ql @@ -0,0 +1,4 @@ +import go + +from IR::ExtractTupleElementInstruction extract +select extract, extract.getResultType() From e50ad90856e8119c92af071cbf60da971beaa278 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 21 Apr 2021 12:36:43 +0100 Subject: [PATCH 8/9] Elaborate comment and change-note a little --- change-notes/2021-04-20-tuple-types.md | 2 +- ql/test/query-tests/Security/CWE-079/reflectedxsstest.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/change-notes/2021-04-20-tuple-types.md b/change-notes/2021-04-20-tuple-types.md index 422f95255de..88e9edd1288 100644 --- a/change-notes/2021-04-20-tuple-types.md +++ b/change-notes/2021-04-20-tuple-types.md @@ -1,2 +1,2 @@ lgtm,codescanning -* Fixed a bug where data flow was not correctly computed through two-value index expressions. This may cause more results from the security queries. +* Fixed a bug where data flow was not correctly computed through two-value index expressions (e.g. `got, ok := myMap[someIndex]`). This may lead to extra results from any dataflow query when an index expression would form part of an important dataflow path. diff --git a/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go b/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go index 9340bc8b189..a6f311e8458 100644 --- a/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go +++ b/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go @@ -51,6 +51,6 @@ func QueryMapTest(w http.ResponseWriter, r http.Request) { keys, ok := r.URL.Query()["data_id"] if ok && len(keys[0]) > 0 { key := keys[0] - w.Write([]byte(key)) // BAD + w.Write([]byte(key)) // BAD: query string is user-controlled } } From 9ab1a8d144217f4d7bb90de370bd19f68f4694af Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 21 Apr 2021 15:28:28 +0100 Subject: [PATCH 9/9] Reword change note Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> --- change-notes/2021-04-20-tuple-types.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/2021-04-20-tuple-types.md b/change-notes/2021-04-20-tuple-types.md index 88e9edd1288..a4db5df1967 100644 --- a/change-notes/2021-04-20-tuple-types.md +++ b/change-notes/2021-04-20-tuple-types.md @@ -1,2 +1,2 @@ lgtm,codescanning -* Fixed a bug where data flow was not correctly computed through two-value index expressions (e.g. `got, ok := myMap[someIndex]`). This may lead to extra results from any dataflow query when an index expression would form part of an important dataflow path. +* Fixed a bug where data flow was not correctly computed through two-value index expressions (for example, `got, ok := myMap[someIndex]`). This may lead to extra results from any dataflow query when an index expression would form part of an important dataflow path.