Merge branch 'main' into identity-consistency-check

ruby/ql/lib/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.6.1
+
+No user-facing changes.
+
 ## 0.6.0
 
 ### Deprecated APIs

ruby/ql/lib/change-notes/released/0.6.1.md

@@ -0,0 +1,3 @@
+## 0.6.1
+
+No user-facing changes.

ruby/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.6.0
+lastReleaseVersion: 0.6.1

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImplCommon.qll

@@ -815,24 +815,20 @@ private module Cached {
     )
   }
 
-  private predicate store(
-    Node node1, Content c, Node node2, DataFlowType contentType, DataFlowType containerType
-  ) {
-    exists(ContentSet cs |
-      c = cs.getAStoreContent() and storeSet(node1, cs, node2, contentType, containerType)
-    )
-  }
-
   /**
    * Holds if data can flow from `node1` to `node2` via a direct assignment to
-   * `f`.
+   * `c`.
    *
    * This includes reverse steps through reads when the result of the read has
    * been stored into, in order to handle cases like `x.f1.f2 = y`.
    */
   cached
-  predicate store(Node node1, TypedContent tc, Node node2, DataFlowType contentType) {
-    store(node1, tc.getContent(), node2, contentType, tc.getContainerType())
+  predicate store(
+    Node node1, Content c, Node node2, DataFlowType contentType, DataFlowType containerType
+  ) {
+    exists(ContentSet cs |
+      c = cs.getAStoreContent() and storeSet(node1, cs, node2, contentType, containerType)
+    )
   }
 
   /**
@@ -932,36 +928,15 @@ private module Cached {
     TReturnCtxNoFlowThrough() or
     TReturnCtxMaybeFlowThrough(ReturnPosition pos)
 
-  cached
-  newtype TTypedContentApprox =
-    MkTypedContentApprox(ContentApprox c, DataFlowType t) {
-      exists(Content cont |
-        c = getContentApprox(cont) and
-        store(_, cont, _, _, t)
-      )
-    }
-
-  cached
-  newtype TTypedContent = MkTypedContent(Content c, DataFlowType t) { store(_, c, _, _, t) }
-
-  cached
-  TypedContent getATypedContent(TypedContentApprox c) {
-    exists(ContentApprox cls, DataFlowType t, Content cont |
-      c = MkTypedContentApprox(cls, pragma[only_bind_into](t)) and
-      result = MkTypedContent(cont, pragma[only_bind_into](t)) and
-      cls = getContentApprox(cont)
-    )
-  }
-
   cached
   newtype TAccessPathFront =
-    TFrontNil(DataFlowType t) or
-    TFrontHead(TypedContent tc)
+    TFrontNil() or
+    TFrontHead(Content c)
 
   cached
   newtype TApproxAccessPathFront =
-    TApproxFrontNil(DataFlowType t) or
-    TApproxFrontHead(TypedContentApprox tc)
+    TApproxFrontNil() or
+    TApproxFrontHead(ContentApprox c)
 
   cached
   newtype TAccessPathFrontOption =
@@ -986,8 +961,16 @@ predicate recordDataFlowCallSite(DataFlowCall call, DataFlowCallable callable) {
 /**
  * A `Node` at which a cast can occur such that the type should be checked.
  */
-class CastingNode extends Node {
+class CastingNode instanceof Node {
   CastingNode() { castingNode(this) }
+
+  string toString() { result = super.toString() }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
 }
 
 private predicate readStepWithTypes(
@@ -1135,9 +1118,17 @@ LocalCallContext getLocalCallContext(CallContext ctx, DataFlowCallable callable)
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph.
  */
-class ParamNode extends Node {
+class ParamNode instanceof Node {
   ParamNode() { parameterNode(this, _, _) }
 
+  string toString() { result = super.toString() }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+
   /**
    * Holds if this node is the parameter of callable `c` at the specified
    * position.
@@ -1146,9 +1137,17 @@ class ParamNode extends Node {
 }
 
 /** A data-flow node that represents a call argument. */
-class ArgNode extends Node {
+class ArgNode instanceof Node {
   ArgNode() { argumentNode(this, _, _) }
 
+  string toString() { result = super.toString() }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+
   /** Holds if this argument occurs at the given position in the given call. */
   final predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
     argumentNode(this, call, pos)
@@ -1159,9 +1158,17 @@ class ArgNode extends Node {
  * A node from which flow can return to the caller. This is either a regular
  * `ReturnNode` or a `PostUpdateNode` corresponding to the value of a parameter.
  */
-class ReturnNodeExt extends Node {
+class ReturnNodeExt instanceof Node {
   ReturnNodeExt() { returnNodeExt(this, _) }
 
+  string toString() { result = super.toString() }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+
   /** Gets the kind of this returned value. */
   ReturnKindExt getKind() { returnNodeExt(this, result) }
 }
@@ -1170,8 +1177,16 @@ class ReturnNodeExt extends Node {
  * A node to which data can flow from a call. Either an ordinary out node
  * or a post-update node associated with a call argument.
  */
-class OutNodeExt extends Node {
+class OutNodeExt instanceof Node {
   OutNodeExt() { outNodeExt(this) }
+
+  string toString() { result = super.toString() }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
 }
 
 /**
@@ -1387,67 +1402,37 @@ class ReturnCtx extends TReturnCtx {
   }
 }
 
-/** An approximated `Content` tagged with the type of a containing object. */
-class TypedContentApprox extends MkTypedContentApprox {
-  private ContentApprox c;
-  private DataFlowType t;
-
-  TypedContentApprox() { this = MkTypedContentApprox(c, t) }
-
-  /** Gets a typed content approximated by this value. */
-  TypedContent getATypedContent() { result = getATypedContent(this) }
-
-  /** Gets the content. */
-  ContentApprox getContent() { result = c }
-
-  /** Gets the container type. */
-  DataFlowType getContainerType() { result = t }
-
-  /** Gets a textual representation of this approximated content. */
-  string toString() { result = c.toString() }
-}
-
 /**
  * The front of an approximated access path. This is either a head or a nil.
  */
 abstract class ApproxAccessPathFront extends TApproxAccessPathFront {
   abstract string toString();
 
-  abstract DataFlowType getType();
-
   abstract boolean toBoolNonEmpty();
 
-  TypedContentApprox getHead() { this = TApproxFrontHead(result) }
+  ContentApprox getHead() { this = TApproxFrontHead(result) }
 
   pragma[nomagic]
-  TypedContent getAHead() {
-    exists(TypedContentApprox cont |
+  Content getAHead() {
+    exists(ContentApprox cont |
       this = TApproxFrontHead(cont) and
-      result = cont.getATypedContent()
+      cont = getContentApprox(result)
     )
   }
 }
 
 class ApproxAccessPathFrontNil extends ApproxAccessPathFront, TApproxFrontNil {
-  private DataFlowType t;
-
-  ApproxAccessPathFrontNil() { this = TApproxFrontNil(t) }
-
-  override string toString() { result = ppReprType(t) }
-
-  override DataFlowType getType() { result = t }
+  override string toString() { result = "nil" }
 
   override boolean toBoolNonEmpty() { result = false }
 }
 
 class ApproxAccessPathFrontHead extends ApproxAccessPathFront, TApproxFrontHead {
-  private TypedContentApprox tc;
+  private ContentApprox c;
 
-  ApproxAccessPathFrontHead() { this = TApproxFrontHead(tc) }
+  ApproxAccessPathFrontHead() { this = TApproxFrontHead(c) }
 
-  override string toString() { result = tc.toString() }
-
-  override DataFlowType getType() { result = tc.getContainerType() }
+  override string toString() { result = c.toString() }
 
   override boolean toBoolNonEmpty() { result = true }
 }
@@ -1461,65 +1446,31 @@ class ApproxAccessPathFrontOption extends TApproxAccessPathFrontOption {
   }
 }
 
-/** A `Content` tagged with the type of a containing object. */
-class TypedContent extends MkTypedContent {
-  private Content c;
-  private DataFlowType t;
-
-  TypedContent() { this = MkTypedContent(c, t) }
-
-  /** Gets the content. */
-  Content getContent() { result = c }
-
-  /** Gets the container type. */
-  DataFlowType getContainerType() { result = t }
-
-  /** Gets a textual representation of this content. */
-  string toString() { result = c.toString() }
-
-  /**
-   * Holds if access paths with this `TypedContent` at their head always should
-   * be tracked at high precision. This disables adaptive access path precision
-   * for such access paths.
-   */
-  predicate forceHighPrecision() { forceHighPrecision(c) }
-}
-
 /**
  * The front of an access path. This is either a head or a nil.
  */
 abstract class AccessPathFront extends TAccessPathFront {
   abstract string toString();
 
-  abstract DataFlowType getType();
-
   abstract ApproxAccessPathFront toApprox();
 
-  TypedContent getHead() { this = TFrontHead(result) }
+  Content getHead() { this = TFrontHead(result) }
 }
 
 class AccessPathFrontNil extends AccessPathFront, TFrontNil {
-  private DataFlowType t;
+  override string toString() { result = "nil" }
 
-  AccessPathFrontNil() { this = TFrontNil(t) }
-
-  override string toString() { result = ppReprType(t) }
-
-  override DataFlowType getType() { result = t }
-
-  override ApproxAccessPathFront toApprox() { result = TApproxFrontNil(t) }
+  override ApproxAccessPathFront toApprox() { result = TApproxFrontNil() }
 }
 
 class AccessPathFrontHead extends AccessPathFront, TFrontHead {
-  private TypedContent tc;
+  private Content c;
 
-  AccessPathFrontHead() { this = TFrontHead(tc) }
+  AccessPathFrontHead() { this = TFrontHead(c) }
 
-  override string toString() { result = tc.toString() }
+  override string toString() { result = c.toString() }
 
-  override DataFlowType getType() { result = tc.getContainerType() }
-
-  override ApproxAccessPathFront toApprox() { result.getAHead() = tc }
+  override ApproxAccessPathFront toApprox() { result.getAHead() = c }
 }
 
 /** An optional access path front. */

ruby/ql/lib/codeql/ruby/filters/GeneratedCode.qll

@@ -19,7 +19,7 @@ abstract class GeneratedCodeComment extends Ruby::Comment { }
  */
 class GenericGeneratedCodeComment extends GeneratedCodeComment {
   GenericGeneratedCodeComment() {
-    exists(string line, string entity, string was, string automatically | line = getValue() |
+    exists(string line, string entity, string was, string automatically | line = this.getValue() |
       entity = "file|class|art[ei]fact|module|script" and
       was = "was|is|has been" and
       automatically = "automatically |mechanically |auto[- ]?" and
@@ -32,7 +32,7 @@ class GenericGeneratedCodeComment extends GeneratedCodeComment {
 /** A comment warning against modifications. */
 class DontModifyMarkerComment extends GeneratedCodeComment {
   DontModifyMarkerComment() {
-    exists(string line | line = getValue() |
+    exists(string line | line = this.getValue() |
       line.regexpMatch("(?i).*\\bGenerated by\\b.*\\bDo not edit\\b.*") or
       line.regexpMatch("(?i).*\\bAny modifications to this file will be lost\\b.*")
     )

ruby/ql/lib/codeql/ruby/frameworks/Rails.qll

@@ -400,3 +400,19 @@ private class AccessLocalsKeySummary extends SummarizedCallable {
     preservesValue = true
   }
 }
+
+/** A call to `render inline: foo`, considered as a ERB template rendering. */
+private class RailsTemplateRendering extends TemplateRendering::Range, DataFlow::CallNode {
+  private DataFlow::Node template;
+
+  RailsTemplateRendering() {
+    (
+      this.asExpr().getExpr() instanceof Rails::RenderCall
+      or
+      this.asExpr().getExpr() instanceof Rails::RenderToCall
+    ) and
+    template = this.getKeywordArgument("inline")
+  }
+
+  override DataFlow::Node getTemplate() { result = template }
+}

ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll

@@ -211,7 +211,7 @@ predicate invocationMatchesExtraCallSiteFilter(InvokeNode invoke, AccessPathToke
 /** An API graph node representing a method call. */
 class InvokeNode extends API::MethodAccessNode {
   /** Gets the number of arguments to the call. */
-  int getNumArgument() { result = getCallNode().getNumberOfArguments() }
+  int getNumArgument() { result = this.getCallNode().getNumberOfArguments() }
 }
 
 /** Gets the `InvokeNode` corresponding to a specific invocation of `node`. */

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

@@ -51,7 +51,7 @@ private CryptographicAlgorithm getBestAlgorithmForName(string name) {
  */
 abstract class CryptographicAlgorithm extends TCryptographicAlgorithm {
   /** Gets a textual representation of this element. */
-  string toString() { result = getName() }
+  string toString() { result = this.getName() }
 
   /**
    * Gets the normalized name of this algorithm (upper-case, no spaces, dashes or underscores).

ruby/ql/lib/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/ruby-all
-version: 0.6.1-dev
+version: 0.6.2-dev
 groups: ruby
 extractor: ruby
 dbscheme: ruby.dbscheme

ruby/ql/src/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.6.1
+
+No user-facing changes.
+
 ## 0.6.0
 
 ### New Queries

ruby/ql/src/change-notes/released/0.6.1.md

@@ -0,0 +1,3 @@
+## 0.6.1
+
+No user-facing changes.

ruby/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.6.0
+lastReleaseVersion: 0.6.1

ruby/ql/src/experimental/template-injection/examples/SSTIBad.rb

@@ -9,6 +9,7 @@ class BadERBController < ActionController::Base
       <h2>Hello %s </h2></body></html>
       " % name
     template = ERB.new(html_text).result(binding) 
+    render inline: html_text
   end
 end
 

ruby/ql/src/experimental/template-injection/examples/SSTIGood.rb

@@ -9,6 +9,7 @@ class GoodController < ActionController::Base
       <h2>Hello <%= name %> </h2></body></html>
       "
     template = ERB.new(html_text).result(binding) 
+    render inline: html_text
   end
 end
 

ruby/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/ruby-queries
-version: 0.6.1-dev
+version: 0.6.2-dev
 groups: 
   - ruby
   - queries

ruby/ql/src/queries/meta/internal/TaintMetrics.qll

@@ -8,6 +8,7 @@ private import codeql.ruby.security.PathInjectionCustomizations
 private import codeql.ruby.security.ServerSideRequestForgeryCustomizations
 private import codeql.ruby.security.UnsafeDeserializationCustomizations
 private import codeql.ruby.security.UrlRedirectCustomizations
+private import codeql.ruby.security.SqlInjectionCustomizations
 
 class RelevantFile extends File {
   RelevantFile() { not getRelativePath().regexpMatch(".*/test(case)?s?/.*") }
@@ -34,6 +35,8 @@ DataFlow::Node relevantTaintSink(string kind) {
     kind = "UnsafeDeserialization" and result instanceof UnsafeDeserialization::Sink
     or
     kind = "UrlRedirect" and result instanceof UrlRedirect::Sink
+    or
+    kind = "SqlInjection" and result instanceof SqlInjection::Sink
   ) and
   // the sink is not a string literal
   not exists(Ast::StringLiteral str |

ruby/ql/test/query-tests/experimental/TemplateInjection/ErbInjection.rb

@@ -14,6 +14,10 @@ class FooController < ActionController::Base
     # where name is unsanitized
     template = ERB.new(bad_text).result(binding) 
 
+    # BAD: user input is evaluated
+    # where name is unsanitized
+    render inline: bad_text
+
     # Template with the source 
     good_text = "
       <!DOCTYPE html><html><body>
@@ -22,6 +26,9 @@ class FooController < ActionController::Base
 
     # GOOD: user input is not evaluated
     template2 = ERB.new(good_text).result(binding) 
+
+    # GOOD: user input is not evaluated
+    render inline: good_text
   end
 end
 

ruby/ql/test/query-tests/experimental/TemplateInjection/TemplateInjection.expected

@@ -4,6 +4,7 @@ edges
 | ErbInjection.rb:5:12:5:17 | call to params | ErbInjection.rb:5:12:5:24 | ...[...] |
 | ErbInjection.rb:5:12:5:24 | ...[...] | ErbInjection.rb:5:5:5:8 | name |
 | ErbInjection.rb:8:5:8:12 | bad_text | ErbInjection.rb:15:24:15:31 | bad_text |
+| ErbInjection.rb:8:5:8:12 | bad_text | ErbInjection.rb:19:20:19:27 | bad_text |
 | ErbInjection.rb:8:16:11:14 | ... % ... | ErbInjection.rb:8:5:8:12 | bad_text |
 | ErbInjection.rb:11:11:11:14 | name | ErbInjection.rb:8:16:11:14 | ... % ... |
 | SlimInjection.rb:5:5:5:8 | name | SlimInjection.rb:8:5:8:12 | bad_text |
@@ -23,6 +24,7 @@ nodes
 | ErbInjection.rb:8:16:11:14 | ... % ... | semmle.label | ... % ... |
 | ErbInjection.rb:11:11:11:14 | name | semmle.label | name |
 | ErbInjection.rb:15:24:15:31 | bad_text | semmle.label | bad_text |
+| ErbInjection.rb:19:20:19:27 | bad_text | semmle.label | bad_text |
 | SlimInjection.rb:5:5:5:8 | name | semmle.label | name |
 | SlimInjection.rb:5:12:5:17 | call to params | semmle.label | call to params |
 | SlimInjection.rb:5:12:5:24 | ...[...] | semmle.label | ...[...] |
@@ -35,5 +37,6 @@ nodes
 subpaths
 #select
 | ErbInjection.rb:15:24:15:31 | bad_text | ErbInjection.rb:5:12:5:17 | call to params | ErbInjection.rb:15:24:15:31 | bad_text | This template depends on a $@. | ErbInjection.rb:5:12:5:17 | call to params | user-provided value |
+| ErbInjection.rb:19:20:19:27 | bad_text | ErbInjection.rb:5:12:5:17 | call to params | ErbInjection.rb:19:20:19:27 | bad_text | This template depends on a $@. | ErbInjection.rb:5:12:5:17 | call to params | user-provided value |
 | SlimInjection.rb:14:25:14:32 | bad_text | SlimInjection.rb:5:12:5:17 | call to params | SlimInjection.rb:14:25:14:32 | bad_text | This template depends on a $@. | SlimInjection.rb:5:12:5:17 | call to params | user-provided value |
 | SlimInjection.rb:23:25:23:33 | bad2_text | SlimInjection.rb:5:12:5:17 | call to params | SlimInjection.rb:23:25:23:33 | bad2_text | This template depends on a $@. | SlimInjection.rb:5:12:5:17 | call to params | user-provided value |