Merge pull request #181 from sauyon/reflectedxss-fps

ReflectedXSS refinement

change-notes/2020-06-17-reflected-xss-refinements.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* The query "Reflected cross-site scripting" has been improved to more correctly determine whether
+  an HTML mime type will be sniffed, which should lead to more accurate results.

ql/src/semmle/go/security/ReflectedXssCustomizations.qll

@@ -64,15 +64,18 @@ module ReflectedXss {
       or
       exists(DataFlow::CallNode call | call.getTarget().hasQualifiedName("fmt", "Fprintf") |
         body = call.getAnArgument() and
-        // checks that the format value does not start with:
+        // checks that the format value does not start with (ignoring whitespace as defined by
+        // https://mimesniff.spec.whatwg.org/#whitespace-byte):
         //  - '<', which could lead to an HTML content type being detected, or
         //  - '%', which could be a format string.
-        call.getArgument(1).getStringValue().regexpMatch("^[^<%].*")
+        call.getArgument(1).getStringValue().regexpMatch("(?s)[\\t\\n\\x0c\\r ]*+[^<%].*")
       )
       or
       exists(DataFlow::Node pred | body = pred.getASuccessor*() |
-        // data starting with a character other than `<` cannot cause an HTML content type to be detected.
-        pred.getStringValue().regexpMatch("^[^<].*")
+        // data starting with a character other than `<` (ignoring whitespace as defined by
+        // https://mimesniff.spec.whatwg.org/#whitespace-byte) cannot cause an HTML content type to
+        // be detected.
+        pred.getStringValue().regexpMatch("(?s)[\\t\\n\\x0c\\r ]*+[^<].*")
         or
         // json data cannot begin with `<`
         exists(EncodingJson::MarshalFunction mf | pred = mf.getOutput().getNode(mf.getACall()))

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

@@ -2,6 +2,10 @@ 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 |
+| contenttype.go:63:11:63:29 | call to FormValue : string | contenttype.go:64:52:64:55 | data |
+| contenttype.go:73:11:73:29 | call to FormValue : string | contenttype.go:79:11:79:14 | data |
+| contenttype.go:88:11:88:29 | call to FormValue : string | contenttype.go:91:4:91:7 | data |
+| contenttype.go:113:11:113:29 | call to FormValue : string | contenttype.go:114:50:114:53 | data |
 | 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 |
@@ -17,6 +21,14 @@ 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 |
+| contenttype.go:63:11:63:29 | call to FormValue : string | semmle.label | call to FormValue : string |
+| contenttype.go:64:52:64:55 | data | semmle.label | data |
+| contenttype.go:73:11:73:29 | call to FormValue : string | semmle.label | call to FormValue : string |
+| contenttype.go:79:11:79:14 | data | semmle.label | data |
+| contenttype.go:88:11:88:29 | call to FormValue : string | semmle.label | call to FormValue : string |
+| contenttype.go:91:4:91:7 | data | semmle.label | data |
+| contenttype.go:113:11:113:29 | call to FormValue : string | semmle.label | call to FormValue : string |
+| contenttype.go:114:50:114:53 | data | semmle.label | data |
 | 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 |
@@ -37,6 +49,10 @@ nodes
 | 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 |
+| contenttype.go:64:52:64:55 | data | contenttype.go:63:11:63:29 | call to FormValue : string | contenttype.go:64:52:64:55 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:63:11:63:29 | call to FormValue | user-provided value |
+| contenttype.go:79:11:79:14 | data | contenttype.go:73:11:73:29 | call to FormValue : string | contenttype.go:79:11:79:14 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:73:11:73:29 | call to FormValue | user-provided value |
+| contenttype.go:91:4:91:7 | data | contenttype.go:88:11:88:29 | call to FormValue : string | contenttype.go:91:4:91:7 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:88:11:88:29 | call to FormValue | user-provided value |
+| contenttype.go:114:50:114:53 | data | contenttype.go:113:11:113:29 | call to FormValue : string | contenttype.go:114:50:114:53 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:113:11:113:29 | call to FormValue | user-provided value |
 | 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: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 |
 | 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 |

ql/test/query-tests/Security/CWE-079/contenttype.go

@@ -54,3 +54,63 @@ func serve5() {
 	})
 	http.ListenAndServe(":80", nil)
 }
+
+func serve10() {
+	http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+		data := r.Form.Get("data")
+
+		data := r.FormValue("data")
+		fmt.Fprintf(w, "\t<html><body>%s</body></html>", data) // Not OK
+	})
+}
+
+func serve11() {
+	http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+		data := r.Form.Get("data")
+
+		data := r.FormValue("data")
+		fmt.Fprintf(w, `
+<html>
+  <body>
+    %s
+  </body>
+</html>`, data) // Not OK
+	})
+}
+
+func serve12() {
+	http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+		data := r.Form.Get("data")
+
+		data := r.FormValue("data")
+		fmt.Fprintf(w, `
+    %s
+`, data) // Not OK
+	})
+}
+
+func serve13() {
+	http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+		data := r.Form.Get("data")
+
+		data := r.FormValue("data")
+		fmt.Fprintf(w, `
+Echoed:
+%s
+`, data) // OK
+	})
+}
+
+func serve14() {
+	http.HandleFunc("/echo", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+		data := r.Form.Get("data")
+
+		data := r.FormValue("data")
+		fmt.Fprintf(w, "<html><body>%s</body></html>", data) // Not OK
+	})
+}