Merge pull request #329 from smowton/smowton/feature/xss-detect-more-json-encoding

Reflected XSS query: exclude more uses of encoding/json.Marshal

Author: max-schaefer

change-notes/2020-09-10-xss-false-positives.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The query "Reflected cross-site scripting" (`go/reflected-xss`) now recognizes more cases of JSON marshaled data, which cannot serve as a vector for an XSS attack. This may reduce false-positive results for this query.

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

@@ -76,9 +76,6 @@ module ReflectedXss {
         // 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()))
       )
     )
   }
@@ -117,4 +114,16 @@ module ReflectedXss {
       outcome = this.getPolarity()
     }
   }
+
+  /**
+   * A JSON marshaler, acting to sanitize a possible XSS vulnerability because the
+   * marshaled value is very unlikely to be returned as an HTML content-type.
+   */
+  class JsonMarshalSanitizer extends Sanitizer {
+    JsonMarshalSanitizer() {
+      exists(MarshalingFunction mf | mf.getFormat() = "JSON" |
+        this = mf.getOutput().getNode(mf.getACall())
+      )
+    }
+  }
 }

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

@@ -1,5 +1,5 @@
 edges
-| ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username |
+| ReflectedXss.go:12:15:12:20 | selection of Form : Values | ReflectedXss.go:15:44:15: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 |
@@ -15,8 +15,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:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values |
-| ReflectedXss.go:14:44:14:51 | username | semmle.label | username |
+| 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 |
 | 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 +46,7 @@ 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: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: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 |
 | 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: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 |

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

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
 )
@@ -18,3 +19,25 @@ func serve() {
 	})
 	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)
+
+}