Merge pull request #16026 from erik-krogh/htmlSafeSan

RB: Add barrier guard for `.html_safe?` to the XSS queries

Author: erik-krogh

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

@@ -7,6 +7,7 @@
 private import ruby
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.frameworks.core.Gem::Gem as Gem
+private import codeql.ruby.security.XSS::ReflectedXss as ReflectedXss
 private import codeql.ruby.typetracking.TypeTracking
 
 /**
@@ -34,7 +35,11 @@ module UnsafeHtmlConstruction {
     abstract string getSinkType();
   }
 
-  private import codeql.ruby.security.XSS::ReflectedXss as ReflectedXss
+  /** A sanitizer for HTML constructed from library input vulnerabilities. */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /** A sanitizer from the reflected-xss query, which is also a sanitizer for unsafe HTML construction. */
+  private class ReflectedXssSanitizers extends Sanitizer instanceof ReflectedXss::Sanitizer { }
 
   /** Gets a node that eventually ends up in the XSS `sink`. */
   private DataFlow::Node getANodeThatEndsInXssSink(ReflectedXss::Sink sink) {

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

@@ -23,10 +23,7 @@ deprecated class Configuration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier
-  }
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
   // override to require the path doesn't have unmatched return steps
   override DataFlow::FlowFeature getAFeature() {
@@ -39,10 +36,7 @@ private module UnsafeHtmlConstructionConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  predicate isBarrier(DataFlow::Node node) {
-    node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier
-  }
+  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
 
   // override to require the path doesn't have unmatched return steps
   DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }

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

@@ -248,6 +248,19 @@ private module Shared {
     or
     isFlowFromHelperMethod(node1, node2)
   }
+
+  private predicate htmlSafeGuard(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
+    exists(DataFlow::CallNode html_safe_call | html_safe_call.getMethodName() = "html_safe?" |
+      guard = html_safe_call.asExpr() and
+      testedNode = html_safe_call.getReceiver().asExpr() and
+      branch = true
+    )
+  }
+
+  /** A guard that calls `.html_safe?` to check whether the string is already HTML-safe. */
+  private class HtmlSafeGuard extends Sanitizer {
+    HtmlSafeGuard() { this = DataFlow::BarrierGuard<htmlSafeGuard/3>::getABarrierNode() }
+  }
 }
 
 /**

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

@@ -2,15 +2,19 @@ edges
 | lib/unsafeHtml.rb:2:31:2:34 | name | lib/unsafeHtml.rb:3:10:3:16 | #{...} | provenance |  |
 | lib/unsafeHtml.rb:9:27:9:30 | name | lib/unsafeHtml.rb:11:13:11:19 | #{...} | provenance |  |
 | lib/unsafeHtml.rb:16:19:16:22 | name | lib/unsafeHtml.rb:17:28:17:31 | name | provenance |  |
+| lib/unsafeHtml.rb:23:32:23:35 | name | lib/unsafeHtml.rb:24:10:24:16 | #{...} | provenance |  |
 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 |
+| lib/unsafeHtml.rb:23:32:23:35 | name | semmle.label | name |
+| lib/unsafeHtml.rb:24:10:24:16 | #{...} | semmle.label | #{...} |
 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 |
+| lib/unsafeHtml.rb:24:10:24:16 | #{...} | lib/unsafeHtml.rb:23:32:23:35 | name | lib/unsafeHtml.rb:24:10:24:16 | #{...} | This string interpolation which depends on $@ might later allow $@. | lib/unsafeHtml.rb:23:32:23:35 | name | library input | lib/unsafeHtml.rb:24:5:24:22 | "<h2>#{...}</h2>" | cross-site scripting |

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

@@ -19,4 +19,12 @@ def sprintf_use name
     # escape
     sprintf("<h2>%s</h2>", ERB::Util.html_escape(name)).html_safe # OK - the parameter is escaped
   end
+
+  def create_user_description2(name)
+    "<h2>#{name}</h2>".html_safe # NOT OK - the value is not necessarily HTML safe
+
+    if name.html_safe?
+      "<h2>#{name}</h2>".html_safe # OK - value is marked as being HTML safe
+    end
+  end
 end