add unsafe-html-construction query

ruby/ql/lib/codeql/ruby/security/UnsafeHtmlConstructionCustomizations.qll

@@ -0,0 +1,96 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * HTML constructed from library input vulnerabilities, as
+ * well as extension points for adding your own.
+ */
+
+private import ruby
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.frameworks.core.Gem::Gem as Gem
+
+/**
+ * Module containing sources, sinks, and sanitizers for HTML constructed from library input.
+ */
+module UnsafeHtmlConstruction {
+  /** A source for HTML constructed from library input vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** An input parameter to a gem seen as a source. */
+  private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
+    LibraryInputAsSource() { this = Gem::getALibraryInput() }
+  }
+
+  /** A sink for HTML constructed from library input vulnerabilities. */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the node for the XSS sink the HTML flows to.
+     */
+    abstract DataFlow::Node getXssSink();
+
+    /**
+     * Gets the type of sink.
+     */
+    abstract string getSinkType();
+  }
+
+  private import codeql.ruby.security.XSS::ReflectedXss as ReflectedXss
+
+  /** Gets a node that eventually ends up in the XSS `sink`. */
+  DataFlow::Node getANodeThatEndsInXssSink(ReflectedXss::Sink sink) {
+    result = getANodeThatEndsInXssSink(TypeTracker::TypeBackTracker::end(), sink)
+  }
+
+  private import codeql.ruby.typetracking.TypeTracker as TypeTracker
+
+  /** Gets a node that is eventually ends up in the XSS `sink`, type-tracked with `t`. */
+  private DataFlow::LocalSourceNode getANodeThatEndsInXssSink(
+    TypeTracker::TypeBackTracker t, ReflectedXss::Sink sink
+  ) {
+    t.start() and
+    result = sink.getALocalSource()
+    or
+    exists(TypeTracker::TypeBackTracker t2 |
+      result = getANodeThatEndsInXssSink(t2, sink).backtrack(t2, t)
+    )
+  }
+
+  /**
+   * A string constructed from a string-literal (e.g. `"foo #{sink}"`),
+   * where the resulting string ends up being used in an XSS sink.
+   */
+  class StringFormatAsSink extends Sink {
+    ReflectedXss::Sink s;
+
+    StringFormatAsSink() {
+      exists(Ast::StringlikeLiteral lit |
+        any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeThatEndsInXssSink(s) and
+        this.asExpr().getExpr() = lit.getComponent(_)
+      )
+    }
+
+    override DataFlow::Node getXssSink() { result = s }
+
+    override string getSinkType() { result = "string interpolation" }
+  }
+
+  import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
+
+  /**
+   * A string constructed from a printf-style call,
+   * where the resulting string ends up being used in an XSS sink.
+   */
+  class TaintedFormatStringAsSink extends Sink {
+    ReflectedXss::Sink s;
+
+    TaintedFormatStringAsSink() {
+      exists(TaintedFormat::PrintfStyleCall call |
+        call = getANodeThatEndsInXssSink(s) and
+        this = [call.getFormatArgument(_), call.getFormatString()]
+      )
+    }
+
+    override DataFlow::Node getXssSink() { result = s }
+
+    override string getSinkType() { result = "string format" }
+  }
+}

ruby/ql/lib/codeql/ruby/security/UnsafeHtmlConstructionQuery.qll

@@ -0,0 +1,49 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about HTML
+ * constructed from library input vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is needed,
+ * otherwise `UnsafeHtmlConstructionCustomizations` should be imported instead.
+ */
+
+import codeql.ruby.DataFlow
+import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction
+private import codeql.ruby.TaintTracking
+private import codeql.ruby.dataflow.BarrierGuards
+
+/**
+ * A taint-tracking configuration for detecting unsafe HTML construction.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "UnsafeShellCommandConstruction" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof StringConstCompareBarrier or
+    node instanceof StringConstArrayInclusionCallBarrier
+  }
+
+  // override to require the path doesn't have unmatched return steps
+  override DataFlow::FlowFeature getAFeature() {
+    result instanceof DataFlow::FeatureHasSourceCallContext
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    // if an array element gets tainted, then we treat the entire array as tainted
+    exists(DataFlow::CallNode call |
+      call.getMethodName() = ["<<", "push", "append"] and
+      call.getReceiver() = succ and
+      pred = call.getArgument(0) and
+      call.getNumberOfArguments() = 1
+    )
+    or
+    exists(DataFlow::CallNode call |
+      call.getMethodName() = "[]" and
+      succ = call and
+      pred = call.getArgument(_)
+    )
+  }
+}

ruby/ql/src/change-notes/2022-11-28-unsafe-html-construction.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/html-constructed-from-input`, to detect libraries that unsafely construct HTML from their inputs.

ruby/ql/src/queries/security/cwe-079/UnsafeHtmlConstruction.qhelp

@@ -0,0 +1,73 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+	<p>
+		When a library function dynamically constructs HTML in a potentially unsafe
+		way, then it's important to document to clients of the library that the function 
+		should only be used with trusted inputs. 
+
+		If the function is not documented as being potentially unsafe, then a client
+		may inadvertently use inputs containing unsafe HTML fragments, and thereby leave
+		the client vulnerable to cross-site scripting attacks.
+	</p>
+</overview>
+<recommendation>
+
+	<p>
+		Document all library functions that can lead to cross-site scripting 
+		attacks, and guard against unsafe inputs where dynamic HTML
+		construction is not intended.
+	</p>
+</recommendation>
+<example>
+
+	<p>
+		The following example has a library function that renders a boldface name
+		by creating a string containing a <code>&lt;b&gt;</code> with the name
+        embedded in it.
+	</p>
+
+	<sample src="examples/unsafe-html-construction.rb" />
+
+	<p>
+		This library function, however, does not escape unsafe HTML, and a client
+		that calls the function with user-supplied input may be vulnerable to
+		cross-site scripting attacks.
+	</p>
+
+	<p>
+		The library could either document that this function should not be used
+		with unsafe inputs, or escape the input before embedding it in the
+        HTML fragment.
+	</p>
+
+	<sample src="examples/unsafe-html-construction_safe.rb" />
+
+</example>
+<references>
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet">DOM based
+		XSS Prevention Cheat Sheet</a>.
+	</li>
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet">XSS
+		(Cross Site Scripting) Prevention Cheat Sheet</a>.
+	</li>
+	<li>
+		OWASP
+		<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
+	</li>
+	<li>
+		OWASP
+		<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
+		Scripting</a>.
+	</li>
+	<li>
+		Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+	</li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-079/UnsafeHtmlConstruction.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Unsafe HTML constructed from library input
+ * @description Using externally controlled strings to construct HTML might allow a malicious
+ *              user to perform a cross-site scripting attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.1
+ * @precision high
+ * @id rb/html-constructed-from-input
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import codeql.ruby.security.UnsafeHtmlConstructionQuery
+import DataFlow::PathGraph
+
+from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where cfg.hasFlowPath(source, sink) and sink.getNode() = sinkNode
+select sinkNode, source, sink,
+  "This " + sinkNode.getSinkType() + " which depends on $@ might later allow $@.", source.getNode(),
+  "library input", sinkNode.getXssSink(), "cross-site scripting"

ruby/ql/src/queries/security/cwe-079/examples/unsafe-html-construction.rb

@@ -0,0 +1,6 @@
+class UsersController < ActionController::Base
+  # BAD - create a user description, where the name is not escaped
+  def create_user_description (name)
+    "<h2>#{name}</h2>".html_safe
+  end
+end

ruby/ql/src/queries/security/cwe-079/examples/unsafe-html-construction_safe.rb

@@ -0,0 +1,6 @@
+class UsersController < ActionController::Base
+  # Good - create a user description, where the name is escaped
+  def create_user_description (name)
+    "<h2>#{ERB::Util.html_escape(name)}</h2>".html_safe
+  end
+end

ruby/ql/test/query-tests/security/cwe-079/UnsafeHtmlConstruction.expected

@@ -0,0 +1,16 @@
+edges
+| lib/unsafeHtml.rb:2:31:2:34 | name :  | lib/unsafeHtml.rb:3:10:3:16 | #{...} |
+| lib/unsafeHtml.rb:9:27:9:30 | name :  | lib/unsafeHtml.rb:11:13:11:19 | #{...} |
+| lib/unsafeHtml.rb:16:19:16:22 | name :  | lib/unsafeHtml.rb:17:28:17:31 | name |
+nodes
+| lib/unsafeHtml.rb:2:31:2:34 | name :  | semmle.label | name :  |
+| lib/unsafeHtml.rb:3:10:3:16 | #{...} | semmle.label | #{...} |
+| lib/unsafeHtml.rb:9:27:9:30 | name :  | semmle.label | name :  |
+| lib/unsafeHtml.rb:11:13:11:19 | #{...} | semmle.label | #{...} |
+| lib/unsafeHtml.rb:16:19:16:22 | name :  | semmle.label | name :  |
+| lib/unsafeHtml.rb:17:28:17:31 | name | semmle.label | name |
+subpaths
+#select
+| lib/unsafeHtml.rb:3:10:3:16 | #{...} | lib/unsafeHtml.rb:2:31:2:34 | name :  | lib/unsafeHtml.rb:3:10:3:16 | #{...} | This string interpolation which depends on $@ might later allow $@. | lib/unsafeHtml.rb:2:31:2:34 | name | library input | lib/unsafeHtml.rb:3:5:3:22 | "<h2>#{...}</h2>" | cross-site scripting |
+| lib/unsafeHtml.rb:11:13:11:19 | #{...} | lib/unsafeHtml.rb:9:27:9:30 | name :  | lib/unsafeHtml.rb:11:13:11:19 | #{...} | This string interpolation which depends on $@ might later allow $@. | lib/unsafeHtml.rb:9:27:9:30 | name | library input | lib/unsafeHtml.rb:13:5:13:5 | h | cross-site scripting |
+| lib/unsafeHtml.rb:17:28:17:31 | name | lib/unsafeHtml.rb:16:19:16:22 | name :  | lib/unsafeHtml.rb:17:28:17:31 | name | This string format which depends on $@ might later allow $@. | lib/unsafeHtml.rb:16:19:16:22 | name | library input | lib/unsafeHtml.rb:17:5:17:32 | call to sprintf | cross-site scripting |

ruby/ql/test/query-tests/security/cwe-079/UnsafeHtmlConstruction.qlref

@@ -0,0 +1 @@
+queries/security/cwe-079/UnsafeHtmlConstruction.ql

ruby/ql/test/query-tests/security/cwe-079/lib/unsafeHtml.rb

@@ -0,0 +1,22 @@
+class Foobar
+  def create_user_description(name)
+    "<h2>#{name}</h2>".html_safe # NOT OK - the parameter is not escaped
+
+    # escape
+    "<h2>#{ERB::Util.html_escape(name)}</h2>".html_safe # OK - the parameter is escaped
+  end
+
+  def string_like_literal name
+    h = <<-HTML
+        <h2>#{name}</h2>
+    HTML
+    h.html_safe # NOT OK - the parameter is not escaped
+  end
+
+  def sprintf_use name
+    sprintf("<h2>%s</h2>", name).html_safe # NOT OK - the parameter is not escaped
+
+    # escape
+    sprintf("<h2>%s</h2>", ERB::Util.html_escape(name)).html_safe # OK - the parameter is escaped
+  end
+end

ruby/ql/test/query-tests/security/cwe-079/unsafe-html.gemspec

@@ -0,0 +1,5 @@
+Gem::Specification.new do |s|
+    s.name = 'unsafe-html'
+    s.require_path = "lib"
+  end
+