From 58e41e9302337151a50ae2c165ce994d07a5803d Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 11 May 2020 15:49:37 -0700 Subject: [PATCH] ReflectedXss: More broadly exclude values with a constant prefix --- ql/src/semmle/go/frameworks/Stdlib.qll | 2 +- .../security/ReflectedXssCustomizations.qll | 8 +++++ .../Security/CWE-079/ReflectedXss.expected | 12 ++++--- ql/test/query-tests/Security/CWE-079/tst.go | 35 +++++++++++++++++++ 4 files changed, 52 insertions(+), 5 deletions(-) diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index 36c9516a449..6d1228d62de 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -596,7 +596,7 @@ module Log { /** Provides models of some functions in the `encoding/json` package. */ module EncodingJson { - private class MarshalFunction extends TaintTracking::FunctionModel, MarshalingFunction::Range { + class MarshalFunction extends TaintTracking::FunctionModel, MarshalingFunction::Range { MarshalFunction() { this.hasQualifiedName("encoding/json", "Marshal") or this.hasQualifiedName("encoding/json", "MarshalIndent") diff --git a/ql/src/semmle/go/security/ReflectedXssCustomizations.qll b/ql/src/semmle/go/security/ReflectedXssCustomizations.qll index 631c18780a7..826feb4a326 100644 --- a/ql/src/semmle/go/security/ReflectedXssCustomizations.qll +++ b/ql/src/semmle/go/security/ReflectedXssCustomizations.qll @@ -69,6 +69,14 @@ module ReflectedXss { // - '%', which could be a format string. call.getArgument(1).getStringValue().regexpMatch("^[^<%].*") ) + or + exists(DataFlow::Node pred | body = pred.getASuccessor*() | + // data starting with `<` cannot cause an HTML content type to be detected. + pred.getStringValue().regexpMatch("^[^<].*") + or + // json data cannot begin with `<` + pred = any(EncodingJson::MarshalFunction mf).getOutput().getExitNode(_) + ) ) } diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index a4dcddef714..c32d23ceb7b 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -2,7 +2,8 @@ edges | 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 | -| tst.go:11:15:11:20 | selection of Form : Values | tst.go:15:12:15:39 | type conversion | +| tst.go:13:15:13:20 | selection of Form : Values | tst.go:17:12:17:39 | type conversion | +| tst.go:47:14:47:19 | selection of Form : Values | tst.go:52:12:52:26 | type conversion | nodes | 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 | @@ -10,10 +11,13 @@ nodes | 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 | | contenttype.go:53:34:53:37 | data | semmle.label | data | -| tst.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values | -| tst.go:15:12:15:39 | type conversion | semmle.label | type conversion | +| tst.go:13:15:13:20 | selection of Form : Values | semmle.label | selection of Form : Values | +| tst.go:17:12:17:39 | type conversion | semmle.label | type conversion | +| tst.go:47:14:47:19 | selection of Form : Values | semmle.label | selection of Form : Values | +| tst.go:52:12:52:26 | type conversion | semmle.label | type conversion | #select | 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 | | 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: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 | -| tst.go:15:12:15:39 | type conversion | tst.go:11:15:11:20 | selection of Form : Values | tst.go:15:12:15:39 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:11:15:11:20 | selection of Form | user-provided value | +| tst.go:17:12:17:39 | type conversion | tst.go:13:15:13:20 | selection of Form : Values | tst.go:17:12:17:39 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:13:15:13:20 | selection of Form | user-provided value | +| tst.go:52:12:52:26 | type conversion | tst.go:47:14:47:19 | selection of Form : Values | tst.go:52:12:52:26 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:47:14:47:19 | selection of Form | user-provided value | diff --git a/ql/test/query-tests/Security/CWE-079/tst.go b/ql/test/query-tests/Security/CWE-079/tst.go index 20e2edd4935..f4a154c94cb 100644 --- a/ql/test/query-tests/Security/CWE-079/tst.go +++ b/ql/test/query-tests/Security/CWE-079/tst.go @@ -1,6 +1,8 @@ package main import ( + "encoding/json" + "fmt" "net/http" "strings" ) @@ -19,3 +21,36 @@ func serve6() { }) http.ListenAndServe(":80", nil) } + +type User struct { + name string +} + +func serve7() { + http.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + username := r.Form.Get("username") + if !isValidUsername(username) { + // OK: json data cannot cause an HTML content type to be detected + a, _ := json.Marshal(User{username}) + w.Write(a) + } else { + // TODO: do something exciting + } + }) + http.ListenAndServe(":80", nil) +} + +func serve8() { + http.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + service := r.Form.Get("service") + if service != "service1" && service != "service2" { + fmt.Fprintln(w, "Service not found") + } else { + // OK: json data cannot cause an HTML content type to be detected + w.Write([]byte(service)) + } + }) + http.ListenAndServe(":80", nil) +}