Go: promote html-template-escaping-bypass-xss

go/ql/integration-tests/query-suite/go-code-scanning.qls.expected

@@ -8,6 +8,7 @@ ql/go/ql/src/Security/CWE-022/TaintedPath.ql
 ql/go/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql
 ql/go/ql/src/Security/CWE-022/ZipSlip.ql
 ql/go/ql/src/Security/CWE-078/CommandInjection.ql
+ql/go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql
 ql/go/ql/src/Security/CWE-079/ReflectedXss.ql
 ql/go/ql/src/Security/CWE-089/SqlInjection.ql
 ql/go/ql/src/Security/CWE-089/StringBreak.ql

go/ql/integration-tests/query-suite/go-security-and-quality.qls.expected

@@ -30,6 +30,7 @@ ql/go/ql/src/Security/CWE-022/TaintedPath.ql
 ql/go/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql
 ql/go/ql/src/Security/CWE-022/ZipSlip.ql
 ql/go/ql/src/Security/CWE-078/CommandInjection.ql
+ql/go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql
 ql/go/ql/src/Security/CWE-079/ReflectedXss.ql
 ql/go/ql/src/Security/CWE-089/SqlInjection.ql
 ql/go/ql/src/Security/CWE-089/StringBreak.ql

go/ql/integration-tests/query-suite/go-security-extended.qls.expected

@@ -8,6 +8,7 @@ ql/go/ql/src/Security/CWE-022/TaintedPath.ql
 ql/go/ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql
 ql/go/ql/src/Security/CWE-022/ZipSlip.ql
 ql/go/ql/src/Security/CWE-078/CommandInjection.ql
+ql/go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql
 ql/go/ql/src/Security/CWE-079/ReflectedXss.ql
 ql/go/ql/src/Security/CWE-089/SqlInjection.ql
 ql/go/ql/src/Security/CWE-089/StringBreak.ql

go/ql/integration-tests/query-suite/not_included_in_qls.expected

@@ -20,7 +20,6 @@ ql/go/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.ql
 ql/go/ql/src/experimental/CWE-525/WebCacheDeception.ql
 ql/go/ql/src/experimental/CWE-74/DsnInjection.ql
 ql/go/ql/src/experimental/CWE-74/DsnInjectionLocal.ql
-ql/go/ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql
 ql/go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql
 ql/go/ql/src/experimental/CWE-840/ConditionalBypass.ql
 ql/go/ql/src/experimental/CWE-918/SSRF.ql

go/ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.qhelp → go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.qhelp

@@ -8,7 +8,7 @@
 			that allow values to be rendered as-is in the template, avoiding the escaping that all the other strings go
 			through.
 		</p>
-		<p>Using them on user-provided values will result in an opportunity for XSS.</p>
+		<p>Using them on user-provided values allows for a cross-site scripting vulnerability.</p>
 	</overview>
 	<recommendation>
 		<p>
@@ -19,10 +19,10 @@
 		<p>
 			In the first example you can see the special types and how they are used in a template:
 		</p>
-		<sample src="HTMLTemplateEscapingPassthroughBad.go" />
+		<sample src="HtmlTemplateEscapingBypassXssBad.go" />
 		<p>
 			To avoid XSS, all user input should be a normal string type.
 		</p>
-		<sample src="HTMLTemplateEscapingPassthroughGood.go" />
+		<sample src="HtmlTemplateEscapingBypassXssGood.go" />
 	</example>
 </qhelp>

go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXss.ql

@@ -0,0 +1,119 @@
+/**
+ * @name HTML template escaping bypass cross-site scripting
+ * @description Converting user input to a special type that avoids escaping
+ *              when fed into an HTML template allows for a cross-site
+ *              scripting vulnerability.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.1
+ * @precision high
+ * @id go/html-template-escaping-bypass-xss
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import go
+
+/**
+ * A type that will not be escaped when passed to a `html/template` template.
+ */
+class UnescapedType extends Type {
+  UnescapedType() {
+    this.hasQualifiedName("html/template",
+      ["CSS", "HTML", "HTMLAttr", "JS", "JSStr", "Srcset", "URL"])
+  }
+}
+
+/**
+ * Holds if the sink is a data value argument of a template execution call.
+ *
+ * Note that this is slightly more general than
+ * `SharedXss::HtmlTemplateSanitizer` because it uses `Function.getACall()`,
+ * which finds calls through interfaces which the receiver implements. This
+ * finds more results in practice.
+ */
+predicate isSinkToTemplateExec(DataFlow::Node sink) {
+  exists(Method fn, string methodName, DataFlow::CallNode call |
+    fn.hasQualifiedName("html/template", "Template", methodName) and
+    call = fn.getACall()
+  |
+    methodName = "Execute" and sink = call.getArgument(1)
+    or
+    methodName = "ExecuteTemplate" and sink = call.getArgument(2)
+  )
+}
+
+/**
+ * Data flow configuration that tracks flows from untrusted sources to template execution calls
+ * which go through a conversion to an unescaped type.
+ */
+module UntrustedToTemplateExecWithConversionConfig implements DataFlow::StateConfigSig {
+  private newtype TConversionState =
+    TUnconverted() or
+    TConverted(UnescapedType unescapedType)
+
+  /**
+   * The flow state for tracking whether a conversion to an unescaped type has
+   * occurred.
+   */
+  class FlowState extends TConversionState {
+    predicate isBeforeConversion() { this instanceof TUnconverted }
+
+    predicate isAfterConversion(UnescapedType unescapedType) { this = TConverted(unescapedType) }
+
+    /** Gets a textual representation of this element. */
+    string toString() {
+      this.isBeforeConversion() and result = "Unconverted"
+      or
+      exists(UnescapedType unescapedType | this.isAfterConversion(unescapedType) |
+        result = "Converted to " + unescapedType.getQualifiedName()
+      )
+    }
+  }
+
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    state.isBeforeConversion() and source instanceof ActiveThreatModelSource
+  }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) {
+    state.isAfterConversion(_) and isSinkToTemplateExec(sink)
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof SharedXss::Sanitizer and not node instanceof SharedXss::HtmlTemplateSanitizer
+    or
+    node.getType() instanceof NumericType
+  }
+
+  /**
+   * When a conversion to a passthrough type is encountered, transition the flow state.
+   */
+  predicate isAdditionalFlowStep(
+    DataFlow::Node pred, FlowState predState, DataFlow::Node succ, FlowState succState
+  ) {
+    exists(ConversionExpr conversion, UnescapedType unescapedType |
+      // If not yet converted, look for a conversion to a passthrough type
+      predState.isBeforeConversion() and
+      succState.isAfterConversion(unescapedType) and
+      succ.(DataFlow::TypeCastNode).getExpr() = conversion and
+      pred.asExpr() = conversion.getOperand() and
+      conversion.getType().getUnderlyingType*() = unescapedType
+    )
+  }
+}
+
+module UntrustedToTemplateExecWithConversionFlow =
+  TaintTracking::GlobalWithState<UntrustedToTemplateExecWithConversionConfig>;
+
+import UntrustedToTemplateExecWithConversionFlow::PathGraph
+
+from
+  UntrustedToTemplateExecWithConversionFlow::PathNode untrustedSource,
+  UntrustedToTemplateExecWithConversionFlow::PathNode templateExecCall, UnescapedType unescapedType
+where
+  UntrustedToTemplateExecWithConversionFlow::flowPath(untrustedSource, templateExecCall) and
+  templateExecCall.getState().isAfterConversion(unescapedType)
+select templateExecCall.getNode(), untrustedSource, templateExecCall,
+  "Data from an $@ will not be auto-escaped because it was converted to template." +
+    unescapedType.getName(), untrustedSource.getNode(), "untrusted source"

go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXssBad.go

@@ -0,0 +1,13 @@
+package main
+
+import (
+	"html/template"
+	"net/http"
+)
+
+func bad(w http.ResponseWriter, r *http.Request) {
+	r.ParseForm()
+	username := r.Form.Get("username")
+	tmpl, _ := template.New("test").Parse(`<b>Hi {{.}}</b>`)
+	tmpl.Execute(w, template.HTML(username))
+}

go/ql/src/Security/CWE-079/HtmlTemplateEscapingBypassXssGood.go

@@ -0,0 +1,13 @@
+package main
+
+import (
+	"html/template"
+	"net/http"
+)
+
+func good(w http.ResponseWriter, r *http.Request) {
+	r.ParseForm()
+	username := r.Form.Get("username")
+	tmpl, _ := template.New("test").Parse(`<b>Hi {{.}}</b>`)
+	tmpl.Execute(w, username)
+}

go/ql/src/Security/CWE-079/StoredXss.go

@@ -2,12 +2,12 @@ package main
 
 import (
 	"io"
-	"io/ioutil"
 	"net/http"
+	"os"
 )
 
 func ListFiles(w http.ResponseWriter, r *http.Request) {
-	files, _ := ioutil.ReadDir(".")
+	files, _ := os.ReadDir(".")
 
 	for _, file := range files {
 		io.WriteString(w, file.Name()+"\n")

go/ql/src/Security/CWE-079/StoredXssGood.go

@@ -3,12 +3,12 @@ package main
 import (
 	"html"
 	"io"
-	"io/ioutil"
 	"net/http"
+	"os"
 )
 
 func ListFiles1(w http.ResponseWriter, r *http.Request) {
-	files, _ := ioutil.ReadDir(".")
+	files, _ := os.ReadDir(".")
 
 	for _, file := range files {
 		io.WriteString(w, html.EscapeString(file.Name())+"\n")

go/ql/src/change-notes/2025-05-01-html-template-escaping-bypass-xss.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* A new query (`go/html-template-escaping-bypass-xss`) has been promoted to the main query suite. This query finds potential cross-site scripting (XSS) vulnerabilities when using the `html/template` package, caused by user input being cast to a type which bypasses the HTML autoescaping. It was originally contributed to the experimental query pack by @gagliardetto in <https://github.com/github/codeql-go/pull/493>.