From 87865afa4290e7d1436ce360273cc685549cbbae Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 3 Feb 2020 13:59:36 -0800 Subject: [PATCH] ReflectedXss: Remove FPs from constant prefix Fprintfs --- ql/src/Security/CWE-079/ReflectedXss.go | 2 +- ql/src/Security/CWE-079/ReflectedXssGood.go | 2 +- .../security/ReflectedXssCustomizations.qll | 16 ++++++++++--- .../Security/CWE-079/ReflectedXss.expected | 18 +++++++++------ .../Security/CWE-079/ReflectedXss.go | 2 +- .../Security/CWE-079/ReflectedXssGood.go | 2 +- .../Security/CWE-079/contenttype.go | 23 +++++++++++++++++++ 7 files changed, 51 insertions(+), 14 deletions(-) diff --git a/ql/src/Security/CWE-079/ReflectedXss.go b/ql/src/Security/CWE-079/ReflectedXss.go index 3f976dc7f66..002732488e1 100644 --- a/ql/src/Security/CWE-079/ReflectedXss.go +++ b/ql/src/Security/CWE-079/ReflectedXss.go @@ -11,7 +11,7 @@ func serve() { username := r.Form.Get("username") if !isValidUsername(username) { // BAD: a request parameter is incorporated without validation into the response - fmt.Fprintf(w, "Unknown user: %q", username) + fmt.Fprintf(w, "%q is an unknown user", username) } else { // TODO: do something exciting } diff --git a/ql/src/Security/CWE-079/ReflectedXssGood.go b/ql/src/Security/CWE-079/ReflectedXssGood.go index 19408bcf492..a16c6538e34 100644 --- a/ql/src/Security/CWE-079/ReflectedXssGood.go +++ b/ql/src/Security/CWE-079/ReflectedXssGood.go @@ -12,7 +12,7 @@ func serve1() { username := r.Form.Get("username") if !isValidUsername(username) { // BAD: a request parameter is incorporated without validation into the response - fmt.Fprintf(w, "Unknown user: %q", html.EscapeString(username)) + fmt.Fprintf(w, "%q is an unknown user", html.EscapeString(username)) } else { // TODO: do something exciting } diff --git a/ql/src/semmle/go/security/ReflectedXssCustomizations.qll b/ql/src/semmle/go/security/ReflectedXssCustomizations.qll index c88b709fd81..0011f9acd8c 100644 --- a/ql/src/semmle/go/security/ReflectedXssCustomizations.qll +++ b/ql/src/semmle/go/security/ReflectedXssCustomizations.qll @@ -37,18 +37,28 @@ module ReflectedXss { * is to prevent us from flagging plain-text or JSON responses as vulnerable. */ class HttpResponseBodySink extends Sink, HTTP::ResponseBody { - HttpResponseBodySink() { not nonHtmlContentType(this.getResponseWriter()) } + HttpResponseBodySink() { not nonHtmlContentType(this) } } /** * Holds if `h` may send a response with a content type other than HTML. */ - private predicate nonHtmlContentType(HTTP::ResponseWriter rw) { + private predicate nonHtmlContentType(HTTP::ResponseBody body) { exists(HTTP::HeaderWrite hw | - hw = rw.getAHeaderWrite() and hw.definesHeader("content-type", _) + hw = body.getResponseWriter().getAHeaderWrite() and hw.definesHeader("content-type", _) | not exists(string tp | hw.definesHeader("content-type", tp) | tp.regexpMatch("(?i).*html.*")) ) + or + not exists(HTTP::HeaderWrite hw, string tp | + hw = body.getResponseWriter().getAHeaderWrite() and hw.definesHeader("content-type", tp) + | + tp.regexpMatch("(?i).*html.*") + ) and + exists(DataFlow::CallNode call | call.getTarget().hasQualifiedName("fmt", "Fprintf") | + body = call.getAnArgument() and + call.getArgument(1).getStringValue().regexpMatch("^[^<%].*") + ) } /** diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index c3eedee3b79..396eaf6eb61 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -1,11 +1,15 @@ edges -| ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:39:14:46 | username | -| contenttype.go:10:11:10:16 | selection of Form : Values | contenttype.go:16:11:16:22 | type conversion | +| 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 | nodes | ReflectedXss.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values | -| ReflectedXss.go:14:39:14:46 | username | semmle.label | username | -| contenttype.go:10:11:10:16 | selection of Form : Values | semmle.label | selection of Form : Values | -| contenttype.go:16:11:16:22 | type conversion | semmle.label | type conversion | +| 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 | +| contenttype.go:53:34:53:37 | data | semmle.label | data | #select -| ReflectedXss.go:14:39:14:46 | username | ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:39:14:46 | username | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:11:15:11:20 | selection of Form | user-provided value | -| contenttype.go:16:11:16:22 | type conversion | contenttype.go:10:11:10:16 | selection of Form : Values | contenttype.go:16:11:16:22 | type conversion | Cross-site scripting vulnerability due to $@. | contenttype.go:10:11:10:16 | selection of Form | user-provided value | +| 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 | diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go index 3f976dc7f66..002732488e1 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXss.go +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXss.go @@ -11,7 +11,7 @@ func serve() { username := r.Form.Get("username") if !isValidUsername(username) { // BAD: a request parameter is incorporated without validation into the response - fmt.Fprintf(w, "Unknown user: %q", username) + fmt.Fprintf(w, "%q is an unknown user", username) } else { // TODO: do something exciting } diff --git a/ql/test/query-tests/Security/CWE-079/ReflectedXssGood.go b/ql/test/query-tests/Security/CWE-079/ReflectedXssGood.go index 19408bcf492..a16c6538e34 100644 --- a/ql/test/query-tests/Security/CWE-079/ReflectedXssGood.go +++ b/ql/test/query-tests/Security/CWE-079/ReflectedXssGood.go @@ -12,7 +12,7 @@ func serve1() { username := r.Form.Get("username") if !isValidUsername(username) { // BAD: a request parameter is incorporated without validation into the response - fmt.Fprintf(w, "Unknown user: %q", html.EscapeString(username)) + fmt.Fprintf(w, "%q is an unknown user", html.EscapeString(username)) } else { // TODO: do something exciting } diff --git a/ql/test/query-tests/Security/CWE-079/contenttype.go b/ql/test/query-tests/Security/CWE-079/contenttype.go index caf9b98d69d..519ca54335f 100644 --- a/ql/test/query-tests/Security/CWE-079/contenttype.go +++ b/ql/test/query-tests/Security/CWE-079/contenttype.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "net/http" ) @@ -31,3 +32,25 @@ func serve3() { }) http.ListenAndServe(":80", nil) } + +func serve4() { + http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + data := r.Form.Get("data") + + fmt.Fprintf(w, "Constant: %s", data) // OK; the prefix causes the content type header to be text/plain + }) + http.ListenAndServe(":80", nil) +} + +func serve5() { + http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) { + r.ParseForm() + data := r.Form.Get("data") + + w.Header().Set("Content-Type", "text/html") + + fmt.Fprintf(w, "Constant: %s", data) // Not OK; the content-type header is explicitly set to html + }) + http.ListenAndServe(":80", nil) +}