Ruby: Add rb/log-injection query

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

@@ -0,0 +1,68 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries.
+ */
+
+import ruby
+import codeql.ruby.Concepts
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import codeql.ruby.dataflow.RemoteFlowSources
+import codeql.ruby.frameworks.Core
+
+/**
+ * A data flow source for user input used in log entries.
+ */
+abstract class Source extends DataFlow::Node { }
+
+/**
+ * A data flow sink for user input used in log entries.
+ */
+abstract class Sink extends DataFlow::Node { }
+
+/**
+ * A sanitizer for malicious user input used in log entries.
+ */
+abstract class Sanitizer extends DataFlow::Node { }
+
+/**
+ * A taint-tracking configuration for untrusted user input used in log entries.
+ */
+class LogInjectionConfiguration extends TaintTracking::Configuration {
+  LogInjectionConfiguration() { this = "LogInjection" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+}
+
+/**
+ * A source of remote user controlled input.
+ */
+class RemoteSource extends Source instanceof RemoteFlowSource { }
+
+/**
+ * An input to a logging mechanism.
+ */
+class LoggingSink extends Sink {
+  LoggingSink() { this = any(Logging logging).getAnInput() }
+}
+
+/**
+ * A call to `String#replace` that replaces `\n` is considered to sanitize the replaced string (reduce false positive).
+ */
+class StringReplaceSanitizer extends Sanitizer {
+  StringReplaceSanitizer() {
+    exists(string s | this.(StringSubstitutionCall).replaces(s, "") and s.regexpMatch("\\n")) and
+    // exclude replacement methods that may not fully sanitize the string
+    this.(StringSubstitutionCall).isGlobal()
+  }
+}
+
+/**
+ * A call to an HTML escape method is considered to sanitize its input.
+ */
+class HtmlEscapingAsSanitizer extends Sanitizer {
+  HtmlEscapingAsSanitizer() { this = any(HtmlEscaping esc).getOutput() }
+}

ruby/ql/src/change-notes/2022-08-10-log-injection-query.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/log-inection`, to detect cases where a malicious user may be able to forge log entries.

ruby/ql/src/queries/security/cwe-117/LogInjection.qhelp

@@ -0,0 +1,62 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+If unsanitized user input is written to a log entry, a malicious user may
+able to forge new log entries.
+</p>
+
+<p>
+Forgery can occur if a user provides some input with characters that are
+interpreted when the log output is displayed. If the log is displayed as a plain
+text file, then new line characters can be used by a malicious user. If the log
+is displayed as HTML, then arbitrary HTML may be included to spoof log entries.
+</p>
+</overview>
+
+<recommendation>
+<p>
+User input should be suitably sanitized before it is logged. Suitable means of
+sanitization depend on how the log entries will be displayed or consumed.
+</p>
+
+<p>
+If the log entries are in plain text then line breaks should be removed from
+user input, using <code>String#gsub</code> or similar. Care should also be
+taken that user input is clearly marked in log entries.
+</p>
+
+<p>
+For log entries that will be displayed in HTML, user input should be
+HTML-encoded before being logged, to prevent forgery and other forms of HTML
+injection.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the example, a username, provided by the user, is logged using `Logger#info`.
+</p>
+
+<p>
+In the first case, it is logged without any sanitization. If a malicious user
+provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter, the
+log entry will be split in two different lines, where the second line will
+be `[INFO]+User:+Admin`.
+</p>
+<sample src="examples/log_injection_bad.rb" />
+
+<p>
+In the second example, <code>String#gsub</code> is used to ensure no line
+endings are present in the user input.
+</p>
+<sample src="examples/log_injection_good.rb" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-117/LogInjection.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Log injection
+ * @description Building log entries from user-controlled sources is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.8
+ * @precision medium
+ * @id rb/log-injection
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import ruby
+import DataFlow::PathGraph
+import codeql.ruby.security.LogInjectionQuery
+
+from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(),
+  "User-provided value"

ruby/ql/src/queries/security/cwe-117/examples/log_injection_bad.rb

@@ -0,0 +1,13 @@
+require 'logger'
+
+class UsersController < ApplicationController
+  def login
+    logger = Logger.new STDOUT
+    username = params[:username]
+
+    # BAD: log message constructed with unsanitized user input
+    logger.info "attempting to login user: " + username
+
+    # ... login logic ...
+  end
+end

ruby/ql/src/queries/security/cwe-117/examples/log_injection_good.rb

@@ -0,0 +1,14 @@
+require 'logger'
+
+class UsersController < ApplicationController
+  def login
+    logger = Logger.new STDOUT
+    username = params[:username]
+
+    # GOOD: log message constructed with unsanitized user input
+    sanitized_username = username.gsub("\n", "")
+    logger.info "attempting to login user: " + sanitized_username
+
+    # ... login logic ...
+  end
+end

ruby/ql/test/query-tests/security/cwe-117/LogInjection.expected

@@ -0,0 +1,36 @@
+edges
+| app/controllers/users_controller.rb:15:19:15:24 | call to params :  | app/controllers/users_controller.rb:15:19:15:30 | ...[...] :  |
+| app/controllers/users_controller.rb:15:19:15:30 | ...[...] :  | app/controllers/users_controller.rb:16:19:16:29 | unsanitized |
+| app/controllers/users_controller.rb:15:19:15:30 | ...[...] :  | app/controllers/users_controller.rb:17:19:17:41 | ... + ... |
+| app/controllers/users_controller.rb:15:19:15:30 | ...[...] :  | app/controllers/users_controller.rb:23:20:23:30 | unsanitized :  |
+| app/controllers/users_controller.rb:23:5:23:44 | ... = ... :  | app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 |
+| app/controllers/users_controller.rb:23:20:23:30 | unsanitized :  | app/controllers/users_controller.rb:23:20:23:44 | call to sub :  |
+| app/controllers/users_controller.rb:23:20:23:44 | call to sub :  | app/controllers/users_controller.rb:23:5:23:44 | ... = ... :  |
+| app/controllers/users_controller.rb:23:20:23:44 | call to sub :  | app/controllers/users_controller.rb:27:16:27:39 | ... + ... |
+| app/controllers/users_controller.rb:33:5:33:31 | ... = ... :  | app/controllers/users_controller.rb:34:33:34:43 | unsanitized |
+| app/controllers/users_controller.rb:33:5:33:31 | ... = ... :  | app/controllers/users_controller.rb:35:33:35:55 | ... + ... |
+| app/controllers/users_controller.rb:33:19:33:25 | call to cookies :  | app/controllers/users_controller.rb:33:19:33:31 | ...[...] :  |
+| app/controllers/users_controller.rb:33:19:33:31 | ...[...] :  | app/controllers/users_controller.rb:33:5:33:31 | ... = ... :  |
+nodes
+| app/controllers/users_controller.rb:15:19:15:24 | call to params :  | semmle.label | call to params :  |
+| app/controllers/users_controller.rb:15:19:15:30 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/users_controller.rb:16:19:16:29 | unsanitized | semmle.label | unsanitized |
+| app/controllers/users_controller.rb:17:19:17:41 | ... + ... | semmle.label | ... + ... |
+| app/controllers/users_controller.rb:23:5:23:44 | ... = ... :  | semmle.label | ... = ... :  |
+| app/controllers/users_controller.rb:23:20:23:30 | unsanitized :  | semmle.label | unsanitized :  |
+| app/controllers/users_controller.rb:23:20:23:44 | call to sub :  | semmle.label | call to sub :  |
+| app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 | semmle.label | unsanitized2 |
+| app/controllers/users_controller.rb:27:16:27:39 | ... + ... | semmle.label | ... + ... |
+| app/controllers/users_controller.rb:33:5:33:31 | ... = ... :  | semmle.label | ... = ... :  |
+| app/controllers/users_controller.rb:33:19:33:25 | call to cookies :  | semmle.label | call to cookies :  |
+| app/controllers/users_controller.rb:33:19:33:31 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/users_controller.rb:34:33:34:43 | unsanitized | semmle.label | unsanitized |
+| app/controllers/users_controller.rb:35:33:35:55 | ... + ... | semmle.label | ... + ... |
+subpaths
+#select
+| app/controllers/users_controller.rb:16:19:16:29 | unsanitized | app/controllers/users_controller.rb:15:19:15:24 | call to params :  | app/controllers/users_controller.rb:16:19:16:29 | unsanitized | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
+| app/controllers/users_controller.rb:17:19:17:41 | ... + ... | app/controllers/users_controller.rb:15:19:15:24 | call to params :  | app/controllers/users_controller.rb:17:19:17:41 | ... + ... | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
+| app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 | app/controllers/users_controller.rb:15:19:15:24 | call to params :  | app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
+| app/controllers/users_controller.rb:27:16:27:39 | ... + ... | app/controllers/users_controller.rb:15:19:15:24 | call to params :  | app/controllers/users_controller.rb:27:16:27:39 | ... + ... | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
+| app/controllers/users_controller.rb:34:33:34:43 | unsanitized | app/controllers/users_controller.rb:33:19:33:25 | call to cookies :  | app/controllers/users_controller.rb:34:33:34:43 | unsanitized | $@ flows to log entry. | app/controllers/users_controller.rb:33:19:33:25 | call to cookies | User-provided value |
+| app/controllers/users_controller.rb:35:33:35:55 | ... + ... | app/controllers/users_controller.rb:33:19:33:25 | call to cookies :  | app/controllers/users_controller.rb:35:33:35:55 | ... + ... | $@ flows to log entry. | app/controllers/users_controller.rb:33:19:33:25 | call to cookies | User-provided value |

ruby/ql/test/query-tests/security/cwe-117/LogInjection.qlref

@@ -0,0 +1 @@
+queries/security/cwe-117/LogInjection.ql

ruby/ql/test/query-tests/security/cwe-117/app/controllers/users_controller.rb

@@ -0,0 +1,45 @@
+require 'logger'
+
+class UsersController < ApplicationController
+  include ERB::Util
+
+  def init_logger
+    if @logger == nil
+      @logger = Logger.new STDOUT
+    end
+  end
+
+  def read_from_params
+    init_logger
+
+    unsanitized = params[:foo]
+    @logger.debug unsanitized             # BAD: unsanitized user input
+    @logger.error "input: " + unsanitized # BAD: unsanitized user input
+
+    sanitized = unsanitized.gsub("\n", "")
+    @logger.fatal sanitized            # GOOD: sanitized user input
+    @logger.warn "input: " + sanitized # GOOD: sanitized user input
+
+    unsanitized2 = unsanitized.sub("\n", "")
+    @logger.info do
+      unsanitized2                      # BAD: partially sanitized user input
+    end
+    @logger << "input: " + unsanitized2 # BAD: partially sanitized user input
+  end
+
+  def read_from_cookies
+    init_logger
+
+    unsanitized = cookies[:bar]
+    @logger.add(Logger::INFO) { unsanitized }             # BAD: unsanitized user input
+    @logger.log(Logger::WARN) { "input: " + unsanitized } # BAD: unsanitized user input
+  end
+
+  def html_sanitization
+    init_logger
+
+    sanitized = html_escape params[:baz]
+    @logger.debug unsanitized             # GOOD: sanitized user input
+    @logger.debug "input: " + unsanitized # GOOD: sanitized user input
+  end
+end