Merge remote-tracking branch 'origin/main' into nickrolfe/incomplete_sanitization

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

@@ -5047,6 +5047,7 @@ private module FlowExploration {
     )
   }
 
+  pragma[nomagic]
   private predicate revPartialPathStep(
     PartialPathNodeRev mid, NodeEx node, FlowState state, TRevSummaryCtx1 sc1, TRevSummaryCtx2 sc2,
     TRevSummaryCtx3 sc3, RevPartialAccessPath ap, Configuration config

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

@@ -5047,6 +5047,7 @@ private module FlowExploration {
     )
   }
 
+  pragma[nomagic]
   private predicate revPartialPathStep(
     PartialPathNodeRev mid, NodeEx node, FlowState state, TRevSummaryCtx1 sc1, TRevSummaryCtx2 sc2,
     TRevSummaryCtx3 sc3, RevPartialAccessPath ap, Configuration config

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

@@ -5047,6 +5047,7 @@ private module FlowExploration {
     )
   }
 
+  pragma[nomagic]
   private predicate revPartialPathStep(
     PartialPathNodeRev mid, NodeEx node, FlowState state, TRevSummaryCtx1 sc1, TRevSummaryCtx2 sc2,
     TRevSummaryCtx3 sc3, RevPartialAccessPath ap, Configuration config

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

@@ -820,24 +820,13 @@ string ppReprType(DataFlowType t) { result = t.toString() }
 pragma[inline]
 predicate compatibleTypes(DataFlowType t1, DataFlowType t2) { any() }
 
-/**
- * A node associated with an object after an operation that might have
- * changed its state.
- *
- * This can be either the argument to a callable after the callable returns
- * (which might have mutated the argument), or the qualifier of a field after
- * an update to the field.
- *
- * Nodes corresponding to AST elements, for example `ExprNode`, usually refer
- * to the value before the update.
- */
-abstract class PostUpdateNode extends Node {
+abstract class PostUpdateNodeImpl extends Node {
   /** Gets the node before the state update. */
   abstract Node getPreUpdateNode();
 }
 
 private module PostUpdateNodes {
-  class ExprPostUpdateNode extends PostUpdateNode, NodeImpl, TExprPostUpdateNode {
+  class ExprPostUpdateNode extends PostUpdateNodeImpl, NodeImpl, TExprPostUpdateNode {
     private CfgNodes::ExprCfgNode e;
 
     ExprPostUpdateNode() { this = TExprPostUpdateNode(e) }
@@ -851,7 +840,7 @@ private module PostUpdateNodes {
     override string toStringImpl() { result = "[post] " + e.toString() }
   }
 
-  private class SummaryPostUpdateNode extends SummaryNode, PostUpdateNode {
+  private class SummaryPostUpdateNode extends SummaryNode, PostUpdateNodeImpl {
     private Node pre;
 
     SummaryPostUpdateNode() { FlowSummaryImpl::Private::summaryPostUpdateNode(this, pre) }

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

@@ -132,6 +132,22 @@ class LocalSourceNode extends Node {
   LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
 }
 
+/**
+ * A node associated with an object after an operation that might have
+ * changed its state.
+ *
+ * This can be either the argument to a callable after the callable returns
+ * (which might have mutated the argument), or the qualifier of a field after
+ * an update to the field.
+ *
+ * Nodes corresponding to AST elements, for example `ExprNode`, usually refer
+ * to the value before the update.
+ */
+class PostUpdateNode extends Node instanceof PostUpdateNodeImpl {
+  /** Gets the node before the state update. */
+  Node getPreUpdateNode() { result = super.getPreUpdateNode() }
+}
+
 cached
 private predicate hasLocalSource(Node sink, Node source) {
   // Declaring `source` to be a `SourceNode` currently causes a redundant check in the

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

@@ -0,0 +1,91 @@
+/**
+ * Provides predicates for reasoning about insecure dependency configurations.
+ */
+
+private import ruby
+
+/**
+ * A method call in a Gemfile.
+ */
+private class GemfileMethodCall extends MethodCall {
+  GemfileMethodCall() { this.getLocation().getFile().getBaseName() = "Gemfile" }
+}
+
+/**
+ * Method calls that configure gem dependencies and can specify (possibly insecure) URLs.
+ */
+abstract private class RelevantGemCall extends GemfileMethodCall {
+  abstract Expr getAUrlPart();
+}
+
+/**
+ * A call to `source`.
+ */
+private class SourceCall extends RelevantGemCall {
+  SourceCall() { this.getMethodName() = "source" }
+
+  override Expr getAUrlPart() { result = this.getAnArgument() }
+}
+
+/**
+ * A call to `git_source`.
+ */
+private class GitSourceCall extends RelevantGemCall {
+  GitSourceCall() { this.getMethodName() = "git_source" }
+
+  override Expr getAUrlPart() { result = this.getBlock().getLastStmt() }
+}
+
+/**
+ * A call to `gem`.
+ */
+private class GemCall extends RelevantGemCall {
+  GemCall() { this.getMethodName() = "gem" }
+
+  override Expr getAUrlPart() { result = this.getKeywordArgument(["source", "git"]) }
+}
+
+/**
+ * Holds if `s` is a URL with an insecure protocol. `proto` is the protocol.
+ */
+bindingset[s]
+private predicate hasInsecureProtocol(string s, string proto) {
+  proto = s.regexpCapture("^(http|ftp):.+", 1).toUpperCase()
+}
+
+/**
+ * Holds if `e` is a string containing a URL that uses the insecure protocol `proto`.
+ */
+private predicate containsInsecureUrl(Expr e, string proto) {
+  // Handle cases where the string as a whole has no constant value (due to interpolations)
+  // but has a known prefix. E.g. "http://#{foo}"
+  exists(StringComponent c | c = e.(StringlikeLiteral).getComponent(0) |
+    hasInsecureProtocol(c.getConstantValue().getString(), proto)
+  )
+  or
+  hasInsecureProtocol(e.getConstantValue().getString(), proto)
+}
+
+/**
+ * Returns the suggested protocol to use in place of the insecure protocol `proto`.
+ */
+bindingset[proto]
+private string suggestedProtocol(string proto) {
+  proto = "HTTP" and result = "HTTPS"
+  or
+  proto = "FTP" and result = "FTPS or SFTP"
+}
+
+/**
+ * Holds if `url` is a string containing a URL that uses an insecure protocol.
+ * `msg` is the alert message that will be displayed to the user.
+ */
+predicate insecureDependencyUrl(Expr url, string msg) {
+  exists(RelevantGemCall call, string proto |
+    url = call.getAUrlPart() and
+    containsInsecureUrl(url, proto) and
+    msg =
+      "Dependency source URL uses the unencrypted protocol " + proto + ". Use " +
+        suggestedProtocol(proto) + " instead."
+  )
+}

ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll

@@ -147,7 +147,7 @@ predicate basicStoreStep(Node nodeFrom, Node nodeTo, string content) {
   // TODO: support SetterMethodCall inside TuplePattern
   exists(ExprNodes::MethodCallCfgNode call |
     content = getSetterCallAttributeName(call.getExpr()) and
-    nodeTo.(DataFlowPrivate::PostUpdateNode).getPreUpdateNode().asExpr() = call.getReceiver() and
+    nodeTo.(DataFlowPublic::PostUpdateNode).getPreUpdateNode().asExpr() = call.getReceiver() and
     call.getExpr() instanceof AST::SetterMethodCall and
     call.getArgument(call.getNumberOfArguments() - 1) =
       nodeFrom.(DataFlowPublic::ExprNode).getExprNode()

ruby/ql/src/change-notes/2022-03-30-insecure-dependency.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/insecure-dependency`. The query finds cases where Ruby gems may be downloaded over an insecure communication channel.

ruby/ql/src/queries/security/cwe-300/InsecureDependencyResolution.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Using an insecure protocol like HTTP or FTP to download dependencies makes the build process vulnerable to a 
+man-in-the-middle (MITM) attack.
+</p>
+<p>
+This can allow attackers to inject malicious code into the downloaded dependencies, and thereby 
+infect the build artifacts and execute arbitrary code on the machine building the artifacts.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Always use a secure protocol, such as HTTPS or SFTP, when downloading artifacts from a URL.</p>
+
+</recommendation>
+
+<example>
+<p>
+The below example shows a <code>Gemfile</code> that specifies a gem source using the insecure HTTP protocol.
+</p>
+<sample src="examples/bad_gemfile.rb" />
+<p>
+The fix is to change the protocol to HTTPS.
+</p>
+<sample src="examples/good_gemfile.rb" />
+</example>
+
+<references>
+<li>
+  Jonathan Leitschuh: 
+  <a href="https://infosecwriteups.com/want-to-take-over-the-java-ecosystem-all-you-need-is-a-mitm-1fc329d898fb">
+    Want to take over the Java ecosystem? All you need is a MITM!
+  </a>
+</li>
+<li>
+  Max Veytsman: 
+  <a href="https://max.computer/blog/how-to-take-over-the-computer-of-any-java-or-clojure-or-scala-developer/">
+    How to take over the computer of any Java (or Clojure or Scala) Developer.
+  </a>
+</li>
+<li>
+  Wikipedia: <a href="https://en.wikipedia.org/wiki/Supply_chain_attack">Supply chain attack.</a>
+</li>
+<li>
+  Wikipedia: <a href="https://en.wikipedia.org/wiki/Man-in-the-middle_attack">Man-in-the-middle attack.</a>
+</li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-300/InsecureDependencyResolution.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Dependency download using unencrypted communication channel
+ * @description Using unencrypted protocols to fetch dependencies can leave an application
+ *              open to man-in-the-middle attacks.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 8.1
+ * @precision high
+ * @id rb/insecure-dependency
+ * @tags security
+ *       external/cwe/cwe-300
+ *       external/cwe/cwe-319
+ *       external/cwe/cwe-494
+ *       external/cwe/cwe-829
+ */
+
+import ruby
+import codeql.ruby.security.InsecureDependencyQuery
+
+from Expr url, string msg
+where insecureDependencyUrl(url, msg)
+select url, msg

ruby/ql/src/queries/security/cwe-300/examples/bad_gemfile.rb

@@ -0,0 +1,3 @@
+source "http://rubygems.org"
+
+gem "my-gem-a", "1.2.3"

ruby/ql/src/queries/security/cwe-300/examples/good_gemfile.rb

@@ -0,0 +1,3 @@
+source "https://rubygems.org"
+
+gem "my-gem-a", "1.2.3"

ruby/ql/test/query-tests/security/cwe-300/Gemfile

@@ -0,0 +1,56 @@
+source "https://rubygems.org" # GOOD
+source "http://rubygems.org" # $result=BAD
+source "ftp://rubygems.org" # $result=BAD
+source "ftps://rubygems.org" # GOOD
+source "unknown://rubygems.org" # GOOD
+
+git_source(:a) { "https://github.com"   } # GOOD
+git_source(:b) { "http://github.com"    } # $result=BAD
+git_source(:c) { "ftp://github.com"     } # $result=BAD
+git_source(:d) { "ftps://github.com"    } # GOOD
+git_source(:e) { "unknown://github.com" } # GOOD
+
+git_source(:f) { |name| "https://github.com/#{name}" } # GOOD
+git_source(:g) { |name| "http://github.com/#{name}" } # $result=BAD
+git_source(:h) { |name| "ftp://github.com/#{name}" } # $result=BAD
+git_source(:i) { |name| "ftps://github.com/#{name}" } # GOOD
+git_source(:j) { |name| "unknown://github.com/#{name}" } # GOOD
+
+git_source(:k) do |name|
+    foo
+    "https://github.com/#{name}" } # GOOD
+end
+git_source(:l) do |name|
+    foo
+    "http://github.com/#{name}" } # $result=BAD
+end
+git_source(:m) do |name|
+    foo
+    "ftp://github.com/#{name}" } # $result=BAD
+end
+git_source(:n) do |name|
+    foo
+    "ftps://github.com/#{name}" } # GOOD
+end
+git_source(:o) do |name|
+    foo
+    "unknown://github.com/#{name}" } # GOOD
+end
+
+gem "jwt", "1.2.3", git: "https://github.com/jwt/ruby-jwt" # GOOD
+gem "jwt", "1.2.3", git: "http://github.com/jwt/ruby-jwt" # $result=BAD
+gem "jwt", "1.2.3", git: "ftp://github.com/jwt/ruby-jwt" # $result=BAD
+gem "jwt", "1.2.3", git: "ftps://github.com/jwt/ruby-jwt" # GOOD
+gem "jwt", "1.2.3", git: "unknown://github.com/jwt/ruby-jwt" # GOOD
+
+gem "jwt", "1.2.3", :git => "https://github.com/jwt/ruby-jwt" # GOOD
+gem "jwt", "1.2.3", :git => "http://github.com/jwt/ruby-jwt" # $result=BAD
+gem "jwt", "1.2.3", :git => "ftp://github.com/jwt/ruby-jwt" # $result=BAD
+gem "jwt", "1.2.3", :git => "ftps://github.com/jwt/ruby-jwt" # GOOD
+gem "jwt", "1.2.3", :git => "unknown://github.com/jwt/ruby-jwt" # GOOD
+
+gem "jwt", "1.2.3", source: "https://rubygems.org" # GOOD
+gem "jwt", "1.2.3", source: "http://rubygems.org" # $result=BAD
+gem "jwt", "1.2.3", source: "ftp://rubygems.org" # $result=BAD
+gem "jwt", "1.2.3", source: "ftps://rubygems.org" # GOOD
+gem "jwt", "1.2.3", source: "unknown://rubygems.org" # GOOD

ruby/ql/test/query-tests/security/cwe-300/InsecureDependency.expected

@@ -0,0 +1,16 @@
+failures
+#select
+| Gemfile:2:8:2:28 | "http://rubygems.org" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:3:8:3:27 | "ftp://rubygems.org" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |
+| Gemfile:8:18:8:36 | "http://github.com" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:9:18:9:35 | "ftp://github.com" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |
+| Gemfile:14:25:14:51 | "http://github.com/#{...}" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:15:25:15:50 | "ftp://github.com/#{...}" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |
+| Gemfile:25:5:25:31 | "http://github.com/#{...}" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:29:5:29:30 | "ftp://github.com/#{...}" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |
+| Gemfile:41:26:41:57 | "http://github.com/jwt/ruby-jwt" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:42:26:42:56 | "ftp://github.com/jwt/ruby-jwt" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |
+| Gemfile:47:29:47:60 | "http://github.com/jwt/ruby-jwt" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:48:29:48:59 | "ftp://github.com/jwt/ruby-jwt" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |
+| Gemfile:53:29:53:49 | "http://rubygems.org" | Dependency source URL uses the unencrypted protocol HTTP. Use HTTPS instead. |
+| Gemfile:54:29:54:48 | "ftp://rubygems.org" | Dependency source URL uses the unencrypted protocol FTP. Use FTPS or SFTP instead. |

ruby/ql/test/query-tests/security/cwe-300/InsecureDependency.ql

@@ -0,0 +1,23 @@
+import ruby
+import TestUtilities.InlineExpectationsTest
+import codeql.ruby.security.InsecureDependencyQuery
+
+class InsecureDependencyTest extends InlineExpectationsTest {
+  InsecureDependencyTest() { this = "InsecureDependencyTest" }
+
+  override string getARelevantTag() { result = "BAD" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "result" and
+    value = "BAD" and
+    exists(Expr e |
+      insecureDependencyUrl(e, _) and
+      location = e.getLocation() and
+      element = e.toString()
+    )
+  }
+}
+
+from Expr url, string msg
+where insecureDependencyUrl(url, msg)
+select url, msg

ruby/ql/test/query-tests/security/cwe-300/foo.rb

@@ -0,0 +1,5 @@
+# Calls to `gem` etc. outside of the Gemfile should be ignored, since they may not be configuring dependencies.
+
+gem "foo", git: "http://foo.com"
+git_source :a { |x| "http://foo.com" }
+source "http://foo.com"