Merge branch 'main' into unsafeCodeConstruction

ruby/ql/src/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.4.5
+
+No user-facing changes.
+
 ## 0.4.4
 
 ### New Queries

ruby/ql/src/change-notes/2022-11-14-stack-trace-exposure.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/stack-trace-exposure`, to detect exposure of stack-traces to users via HTTP responses.

ruby/ql/src/change-notes/2022-11-22-kernel-open-sinks.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Extended the `rb/kernel-open` query with following sinks: `IO.write`, `IO.binread`, `IO.binwrite`, `IO.foreach`, `IO.readlines`, and `URI.open`.

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

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

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

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.4.4
+lastReleaseVersion: 0.4.5

ruby/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/ruby-queries
-version: 0.4.5-dev
+version: 0.4.6-dev
 groups: 
   - ruby
   - queries

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

@@ -6,15 +6,19 @@
 <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>.
+The same vulnerability applies to <code>IO.read</code>, <code>IO.write</code>,
+<code>IO.binread</code>, <code>IO.binwrite</code>, <code>IO.foreach</code>,
+<code>IO.readlines</code> and <code>URI.open</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
+does not have this vulnerability. Similarly, use the methods from the <code>File</code>
+class instead of the <code>IO</code> class e.g. <code>File.read</code> instead
 of <code>IO.read</code>.</p>
+<p>Instead of <code>URI.open</code> use <code>URI(..).open</code> or an HTTP Client.</p>
 
 </recommendation>
 <example>
@@ -36,6 +40,7 @@ user-supplied file path.
 <li>
 OWASP:
 <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Ruby_on_Rails_Cheat_Sheet.html#command-injection">Ruby on Rails Cheat Sheet: Command Injection</a>.
 </li>
 
 <li>

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

@@ -1,6 +1,7 @@
 /**
- * @name Use of `Kernel.open` or `IO.read` with user-controlled input
- * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ * @name Use of `Kernel.open`, `IO.read` or similar sinks with user-controlled input
+ * @description Using `Kernel.open`, `IO.read`, `IO.write`, `IO.binread`, `IO.binwrite`,
+ *              `IO.foreach`, `IO.readlines`, or `URI.open` may allow a malicious
  *              user to execute arbitrary system commands.
  * @kind path-problem
  * @problem.severity error
@@ -32,7 +33,8 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) {
     node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier
+    node instanceof StringConstArrayInclusionCallBarrier or
+    node instanceof Sanitizer
   }
 }
 

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

@@ -1,6 +1,7 @@
 /**
- * @name Use of `Kernel.open` or `IO.read` with a non-constant value
- * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ * @name Use of `Kernel.open` or `IO.read` or similar sinks with a non-constant value
+ * @description Using `Kernel.open`, `IO.read`, `IO.write`, `IO.binread`, `IO.binwrite`,
+ *              `IO.foreach`, `IO.readlines`, or `URI.open` may allow a malicious
  *              user to execute arbitrary system commands.
  * @kind problem
  * @problem.severity warning
@@ -15,15 +16,25 @@
  */
 
 import codeql.ruby.security.KernelOpenQuery
-import codeql.ruby.ast.Literal
+import codeql.ruby.AST
+import codeql.ruby.ApiGraphs
 
 from AmbiguousPathCall call
 where
-  // there is not a constant string argument
-  not exists(call.getPathArgument().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
+  not hasConstantPrefix(call.getPathArgument().getALocalSource().asExpr().getExpr()) and
+  not call.getPathArgument().getALocalSource() =
+    API::getTopLevelMember("File").getAMethodCall("join")
 select call,
   "Call to " + call.getName() + " with a non-constant value. Consider replacing it with " +
     call.getReplacement() + "."
+
+predicate hasConstantPrefix(Expr e) {
+  // if it's a format string, then the first argument is not a constant string
+  e.(StringlikeLiteral).getComponent(0) instanceof StringTextComponent
+  or
+  // it is not a constant string argument
+  exists(e.getConstantValue())
+  or
+  // not a concatenation that starts with a constant string
+  hasConstantPrefix(e.(AddExpr).getLeftOperand())
+}

ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+<p>
+Software developers often add stack traces to error messages, as a debugging
+aid. Whenever that error message occurs for an end user, the developer can use
+the stack trace to help identify how to fix the problem. In particular, stack
+traces can tell the developer more about the sequence of events that led to a
+failure, as opposed to merely the final state of the software when the error
+occurred.
+</p>
+
+<p>
+Unfortunately, the same information can be useful to an attacker.  The sequence
+of class or method names in a stack trace can reveal the structure of the
+application as well as any internal components it relies on.  Furthermore, the
+error message at the top of a stack trace can include information such as
+server-side file names and SQL code that the application relies on, allowing an
+attacker to fine-tune a subsequent injection attack.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Send the user a more generic error message that reveals less information.
+Either suppress the stack trace entirely, or log it only on the server.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, an exception is handled in two different ways. In the
+first version, labeled BAD, the exception is exposed to the remote user by
+rendering it as an HTTP response. As such, the user is able to see a detailed
+stack trace, which may contain sensitive information. In the second version, the
+error message is logged only on the server, and a generic error message is
+displayed to the user. That way, the developers can still access and use the
+error log, but remote users will not see the information.  </p>
+
+<sample src="examples/StackTraceExposure.rb" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://owasp.org/www-community/Improper_Error_Handling">Improper Error Handling</a>.</li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-209/StackTraceExposure.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Information exposure through an exception
+ * @description Leaking information about an exception, such as messages and stack traces, to an
+ *              external user can expose implementation details that are useful to an attacker for
+ *              developing a subsequent exploit.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 5.4
+ * @precision high
+ * @id rb/stack-trace-exposure
+ * @tags security
+ *       external/cwe/cwe-209
+ *       external/cwe/cwe-497
+ */
+
+import codeql.ruby.DataFlow
+import codeql.ruby.security.StackTraceExposureQuery
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ can be exposed to an external user.", source.getNode(),
+  "Error information"

ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb

@@ -0,0 +1,18 @@
+class UsersController < ApplicationController
+
+  def update_bad(id)
+    do_computation()
+  rescue => e
+    # BAD
+    render body: e.backtrace, content_type: "text/plain"
+  end
+
+  def update_good(id)
+    do_computation()
+  rescue => e
+    # GOOD
+    logger.error e.backtrace
+    render body: "Computation failed", content_type: "text/plain"
+  end
+
+end