Merge remote-tracking branch 'origin/main' into rb/sensitive-get-query

ruby/ql/src/queries/meta/TaintedNodes.ql

@@ -0,0 +1,28 @@
+/**
+ * @name Tainted nodes
+ * @description Nodes reachable from a remote flow source via default taint-tracking steps.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id rb/meta/tainted-nodes
+ * @tags meta
+ * @precision very-low
+ */
+
+import internal.TaintMetrics
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+
+class BasicTaintConfiguration extends TaintTracking::Configuration {
+  BasicTaintConfiguration() { this = "BasicTaintConfiguration" }
+
+  override predicate isSource(DataFlow::Node node) { node = relevantTaintSource(_) }
+
+  override predicate isSink(DataFlow::Node node) {
+    // To reduce noise from synthetic nodes, only count nodes that have an associated expression.
+    exists(node.asExpr().getExpr())
+  }
+}
+
+from DataFlow::Node node
+where any(BasicTaintConfiguration cfg).hasFlow(_, node)
+select node, "Tainted node"

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

@@ -36,3 +36,10 @@ DataFlow::Node relevantTaintSink(string kind) {
     kind = "UrlRedirect" and result instanceof UrlRedirect::Sink
   )
 }
+
+/**
+ * Gets the root folder of the snapshot.
+ *
+ * This is selected as the location for project-wide metrics.
+ */
+Folder projectRoot() { result.getRelativePath() = "" }

ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
+character, it will execute the remaining string as a shell command. If a
+malicious user can control the file name, they can execute arbitrary code.
+The same vulnerability applies to <code>IO.read</code>.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Use <code>File.open</code> instead of <code>Kernel.open</code>, as the former
+does not have this vulnerability. Similarly, use <code>File.read</code> instead
+of <code>IO.read</code>.</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example shows code that calls <code>Kernel.open</code> on a
+user-supplied file path.
+</p>
+
+<sample src="examples/kernel_open.rb" />
+
+<p>Instead, <code>File.open</code> should be used, as in the following example.</p>
+
+<sample src="examples/file_open.rb" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+
+<li>
+Example CVE: <a href="https://www.ruby-lang.org/en/news/2021/05/02/os-command-injection-in-rdoc/">Command Injection in RDoc</a>.
+</li>
+
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-078/KernelOpen.qhelp

@@ -1,46 +1,4 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
 <qhelp>
-<overview>
-<p>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
-character, it will execute the remaining string as a shell command. If a
-malicious user can control the file name, they can execute arbitrary code.
-The same vulnerability applies to <code>IO.read</code>.
-</p>
-
-</overview>
-<recommendation>
-
-<p>Use <code>File.open</code> instead of <code>Kernel.open</code>, as the former
-does not have this vulnerability. Similarly, use <code>File.read</code> instead
-of <code>IO.read</code>.</p>
-
-</recommendation>
-<example>
-
-<p>
-The following example shows code that calls <code>Kernel.open</code> on a
-user-supplied file path.
-</p>
-
-<sample src="examples/kernel_open.rb" />
-
-<p>Instead, <code>File.open</code> should be used, as in the following example.</p>
-
-<sample src="examples/file_open.rb" />
-
-</example>
-<references>
-
-<li>
-OWASP:
-<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
-</li>
-
-<li>
-Example CVE: <a href="https://www.ruby-lang.org/en/news/2021/05/02/os-command-injection-in-rdoc/">Command Injection in RDoc</a>.
-</li>
-
-</references>
-</qhelp>
+  <include src="KernelOpen.inc.qhelp" />
+</qhelp>

ruby/ql/src/queries/security/cwe-078/KernelOpen.ql

@@ -1,5 +1,5 @@
 /**
- * @name Use of `Kernel.open` or `IO.read`
+ * @name Use of `Kernel.open` or `IO.read` with user-controlled input
  * @description Using `Kernel.open` or `IO.read` may allow a malicious
  *              user to execute arbitrary system commands.
  * @kind path-problem
@@ -14,39 +14,12 @@
  *       external/cwe/cwe-073
  */
 
-import codeql.ruby.AST
-import codeql.ruby.ApiGraphs
-import codeql.ruby.frameworks.core.Kernel::Kernel
-import codeql.ruby.TaintTracking
-import codeql.ruby.dataflow.BarrierGuards
-import codeql.ruby.dataflow.RemoteFlowSources
 import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import codeql.ruby.dataflow.RemoteFlowSources
+import codeql.ruby.dataflow.BarrierGuards
 import DataFlow::PathGraph
-
-/**
- * A method call that has a suggested replacement.
- */
-abstract class Replacement extends DataFlow::CallNode {
-  abstract string getFrom();
-
-  abstract string getTo();
-}
-
-class KernelOpenCall extends KernelMethodCall, Replacement {
-  KernelOpenCall() { this.getMethodName() = "open" }
-
-  override string getFrom() { result = "Kernel.open" }
-
-  override string getTo() { result = "File.open" }
-}
-
-class IOReadCall extends DataFlow::CallNode, Replacement {
-  IOReadCall() { this = API::getTopLevelMember("IO").getAMethodCall("read") }
-
-  override string getFrom() { result = "IO.read" }
-
-  override string getTo() { result = "File.read" }
-}
+import codeql.ruby.security.KernelOpenQuery
 
 class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "KernelOpen" }
@@ -54,9 +27,7 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(KernelOpenCall c | c.getArgument(0) = sink)
-    or
-    exists(IOReadCall c | c.getArgument(0) = sink)
+    sink = any(AmbiguousPathCall r).getPathArgument()
   }
 
   override predicate isSanitizer(DataFlow::Node node) {
@@ -73,5 +44,6 @@ where
   sourceNode = source.getNode() and
   call.getArgument(0) = sink.getNode()
 select sink.getNode(), source, sink,
-  "This call to " + call.(Replacement).getFrom() + " depends on a $@. Replace it with " +
-    call.(Replacement).getTo() + ".", source.getNode(), "user-provided value"
+  "This call to " + call.(AmbiguousPathCall).getName() +
+    " depends on a $@. Consider replacing it with " + call.(AmbiguousPathCall).getReplacement() +
+    ".", source.getNode(), "user-provided value"

ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.qhelp

@@ -0,0 +1,4 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+  <include src="KernelOpen.inc.qhelp" />
+</qhelp>

ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.ql

@@ -0,0 +1,29 @@
+/**
+ * @name Use of `Kernel.open` or `IO.read` with a non-constant value
+ * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ *              user to execute arbitrary system commands.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 6.5
+ * @precision high
+ * @id rb/non-constant-kernel-open
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ *       external/cwe/cwe-073
+ */
+
+import codeql.ruby.security.KernelOpenQuery
+import codeql.ruby.ast.Literal
+
+from AmbiguousPathCall call
+where
+  // there is not a constant string argument
+  not exists(call.getPathArgument().asExpr().getExpr().getConstantValue()) and
+  // if it's a format string, then the first argument is not a constant string
+  not call.getPathArgument().getALocalSource().asExpr().getExpr().(StringLiteral).getComponent(0)
+    instanceof StringTextComponent
+select call,
+  "Call to " + call.getName() + " with a non-constant value. Consider replacing it with " +
+    call.getReplacement() + "."

ruby/ql/src/queries/security/cwe-295/RequestWithoutValidation.ql

@@ -27,8 +27,6 @@ where
   // NOTE: We compare the locations instead of DataFlow::Nodes directly, since for
   // snippet `Excon.defaults[:ssl_verify_peer] = false`, `disablingNode = argumentNode`
   // does NOT hold.
-  if disablingNode.getLocation() = origin.getLocation()
-  then ending = "."
-  else ending = " by the value from $@."
-select request, "This request may run without certificate validation because it is $@" + ending,
-  disablingNode, "disabled here", origin, "here"
+  if disablingNode.getLocation() = origin.getLocation() then ending = "." else ending = " by $@."
+select request, "This request may run without certificate validation because $@" + ending,
+  disablingNode, "the request is disabled", origin, "this value"

ruby/ql/src/queries/security/cwe-312/CleartextLogging.ql

@@ -20,5 +20,5 @@ import DataFlow::PathGraph
 
 from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "Sensitive data returned by $@ is logged here.",
+select sink.getNode(), source, sink, "This logs sensitive data returned by $@ as clear text.",
   source.getNode(), source.getNode().(Source).describe()

ruby/ql/src/queries/security/cwe-312/CleartextStorage.ql

@@ -21,5 +21,5 @@ import DataFlow::PathGraph
 
 from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "Sensitive data returned by $@ is stored here.",
+select sink.getNode(), source, sink, "This stores sensitive data returned by $@ as clear text.",
   source.getNode(), source.getNode().(Source).describe()

ruby/ql/src/queries/security/cwe-502/UnsafeDeserialization.qhelp

@@ -12,7 +12,7 @@ to execute arbitrary code.
 <recommendation>
 <p>
 Avoid deserialization of untrusted data if possible. If the architecture permits
-it, use serialization formats that cannot represent arbitarary objects. For
+it, use serialization formats that cannot represent arbitrary objects. For
 libraries that support it, such as the Ruby standard library's <code>JSON</code>
 module, ensure that the parser is configured to disable
 deserialization of arbitrary objects.

ruby/ql/src/queries/security/cwe-732/WeakFilePermissions.ql

@@ -63,5 +63,6 @@ from
   DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf,
   FileSystemPermissionModification mod
 where conf.hasFlowPath(source, sink) and mod.getAPermissionNode() = sink.getNode()
-select source.getNode(), source, sink, "Overly permissive mask in $@ sets file to $@.", mod,
-  mod.toString(), source.getNode(), source.getNode().toString()
+select source.getNode(), source, sink,
+  "This overly permissive mask used in $@ allows read or write access to others.", mod,
+  mod.toString()

ruby/ql/src/queries/security/cwe-798/HardcodedCredentials.ql

@@ -154,4 +154,5 @@ class HardcodedCredentialsConfiguration extends DataFlow::Configuration {
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, HardcodedCredentialsConfiguration conf
 where conf.hasFlowPath(source, sink)
-select source.getNode(), source, sink, "Use of $@.", source.getNode(), "hardcoded credentials"
+select source.getNode(), source, sink, "This hardcoded value is $@.", sink.getNode(),
+  "used as credentials"

ruby/ql/src/queries/security/cwe-829/InsecureDownload.qhelp

@@ -4,7 +4,7 @@
 <qhelp>
 	<overview>
 		<p>
-			Downloading executeables or other sensitive files over an unencrypted connection 
+			Downloading executables or other sensitive files over an unencrypted connection 
 			can leave a server open to man-in-the-middle attacks (MITM). 
 			Such an attack can allow an attacker to insert arbitrary content 
 			into the downloaded file, and in the worst case, allow the attacker to execute 

ruby/ql/src/queries/security/cwe-912/HttpToFileAccess.ql

@@ -18,4 +18,5 @@ import codeql.ruby.security.HttpToFileAccessQuery
 
 from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "$@ flows to file system.", source.getNode(), "Untrusted data"
+select sink.getNode(), source, sink, "Write to file system depends on $@.", source.getNode(),
+  "untrusted data"