github · alexrford · Jan 5, 2022 · Dec 5, 2021 · Dec 5, 2021 · Dec 5, 2021
@@ -645,6 +645,37 @@ module Path {
   }
 }
 
+/**
+ * A data-flow node that may configure behavior relating to cookie security.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `CookieSecurityConfigurationSetting::Range` instead.
+ */
+class CookieSecurityConfigurationSetting extends DataFlow::Node instanceof CookieSecurityConfigurationSetting::Range {
+  /**
+   * Gets a description of how this cookie setting may weaken application security.
+   * This predicate has no results if the setting is considered to be safe.
+   */
+  string getSecurityWarningMessage() { result = super.getSecurityWarningMessage() }
+}
+
+/** Provides a class for modeling new cookie security setting APIs. */
+module CookieSecurityConfigurationSetting {
+  /**
+   * A data-flow node that may configure behavior relating to cookie security.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `CookieSecurityConfigurationSetting` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets a description of how this cookie setting may weaken application security.
+     * This predicate has no results if the setting is considered to be safe.
+     */
+    abstract string getSecurityWarningMessage();
+  }
+}
+
 /**
  * A data-flow node that logs data.
  *

@@ -12,6 +12,7 @@ private import codeql.ruby.frameworks.ActiveRecord
 private import codeql.ruby.frameworks.ActiveStorage
 private import codeql.ruby.ast.internal.Module
 private import codeql.ruby.ApiGraphs
+private import codeql.ruby.security.OpenSSL
 
 /**
  * A reference to either `Rails::Railtie`, `Rails::Engine`, or `Rails::Application`.
@@ -47,85 +48,220 @@ private DataFlow::CallNode getAConfigureCallNode() {
 }
 
 /**
- * An access to a Rails config object.
+ * Classes representing accesses to the Rails config object.
  */
-private class ConfigSourceNode extends DataFlow::LocalSourceNode {
-  ConfigSourceNode() {
-    // `Foo < Rails::Application ... config ...`
-    exists(MethodCall configCall | this.asExpr().getExpr() = configCall |
-      configCall.getMethodName() = "config" and
-      configCall.getEnclosingModule() instanceof RailtieClass
-    )
-    or
-    // `Rails.application.config`
-    this =
-      API::getTopLevelMember("Rails")
-          .getReturn("application")
-          .getReturn("config")
-          .getAnImmediateUse()
-    or
-    // `Rails.application.configure { ... config ... }`
-    // `Rails::Application.configure { ... config ... }`
-    exists(DataFlow::CallNode configureCallNode, Block block, MethodCall configCall |
-      configCall = this.asExpr().getExpr()
-    |
-      configureCallNode = getAConfigureCallNode() and
-      block = configureCallNode.getBlock().asExpr().getExpr() and
-      configCall.getParent+() = block and
-      configCall.getMethodName() = "config"
-    )
+private module Config {
+  /**
+   * An access to a Rails config object.
+   */
+  private class SourceNode extends DataFlow::LocalSourceNode {
+    SourceNode() {
+      // `Foo < Rails::Application ... config ...`
+      exists(MethodCall configCall | this.asExpr().getExpr() = configCall |
+        configCall.getMethodName() = "config" and
+        configCall.getEnclosingModule() instanceof RailtieClass
+      )
+      or
+      // `Rails.application.config`
+      this =
+        API::getTopLevelMember("Rails")
+            .getReturn("application")
+            .getReturn("config")
+            .getAnImmediateUse()
+      or
+      // `Rails.application.configure { ... config ... }`
+      // `Rails::Application.configure { ... config ... }`
+      exists(DataFlow::CallNode configureCallNode, Block block, MethodCall configCall |
+        configCall = this.asExpr().getExpr()
+      |
+        configureCallNode = getAConfigureCallNode() and
+        block = configureCallNode.asExpr().getExpr().(MethodCall).getBlock() and
+        configCall.getParent+() = block and
+        configCall.getMethodName() = "config"
+      )
+    }
+  }
+
+  /**
+   * A reference to the Rails config object.
+   */
+  class Node extends DataFlow::Node {
+    Node() { exists(SourceNode src | src.flowsTo(this)) }
   }
-}
 
-private class ConfigNode extends DataFlow::Node {
-  ConfigNode() { exists(ConfigSourceNode src | src.flowsTo(this)) }
+  /**
+   * A reference to the ActionController config object.
+   */
+  class ActionControllerNode extends DataFlow::Node {
+    ActionControllerNode() {
+      exists(DataFlow::CallNode source |
+        source.getReceiver() instanceof Node and
+        source.getMethodName() = "action_controller"
+      |
+        source.flowsTo(this)
+      )
+    }
+  }
+
+  /**
+   * A reference to the ActionDispatch config object.
+   */
+  class ActionDispatchNode extends DataFlow::Node {
+    ActionDispatchNode() {
+      exists(DataFlow::CallNode source |
+        source.getReceiver() instanceof Node and
+        source.getMethodName() = "action_dispatch"
+      |
+        source.flowsTo(this)
+      )
+    }
+  }
 }
 
-// A call where the Rails application config is the receiver
-private class CallAgainstConfig extends DataFlow::CallNode {
-  CallAgainstConfig() { this.getReceiver() instanceof ConfigNode }
+/**
+ * Classes representing nodes that set a Rails configuration value.
+ */
+private module Settings {
+  private predicate isInTestConfiguration(Location loc) {
+    loc.getFile().getRelativePath().matches("%test/%") or
+    loc.getFile().getStem() = "test"
+  }
+
+  private class Setting extends DataFlow::CallNode {
+    Setting() {
+      // exclude some test configuration
+      not isInTestConfiguration(this.getLocation()) and
+      this.getReceiver+() instanceof Config::Node and
+      this.asExpr().getExpr() instanceof SetterMethodCall
+    }
+  }
+
+  private class LiteralSetting extends Setting {
+    Literal valueLiteral;
+
+    LiteralSetting() {
+      exists(DataFlow::LocalSourceNode lsn |
+        lsn.asExpr().getExpr() = valueLiteral and
+        lsn.flowsTo(this.getArgument(0))
+      )
+    }
+
+    string getValueText() { result = valueLiteral.getValueText() }
+
+    string getSettingString() { result = this.getMethodName() + this.getValueText() }
+  }
+
+  /**
+   * A node that sets a boolean value.
+   */
+  class BooleanSetting extends LiteralSetting {
+    override BooleanLiteral valueLiteral;
+
+    boolean getValue() { result = valueLiteral.getValue() }
+  }
+
+  /**
+   * A node that sets a Stringlike value.
+   */
+  class StringlikeSetting extends LiteralSetting {
+    override StringlikeLiteral valueLiteral;
+  }
+
+  /**
+   * A node that sets a Stringlike value, or `nil`.
+   */
+  class NillableStringlikeSetting extends LiteralSetting {
+    NillableStringlikeSetting() {
+      valueLiteral instanceof StringlikeLiteral or
+      valueLiteral instanceof NilLiteral
+    }
+
+    string getStringValue() { result = valueLiteral.(StringlikeLiteral).getValueText() }
 
-  MethodCall getCall() { result = this.asExpr().getExpr() }
+    predicate isNilValue() { valueLiteral instanceof NilLiteral }
+  }
 }
 
-private class ActionControllerConfigNode extends DataFlow::Node {
-  ActionControllerConfigNode() {
-    exists(CallAgainstConfig source | source.getCall().getMethodName() = "action_controller" |
-      source.flowsTo(this)
-    )
+/**
+ * A `DataFlow::Node` that may enable or disable Rails CSRF protection in
+ * production code.
+ */
+private class AllowForgeryProtectionSetting extends Settings::BooleanSetting,
+  CSRFProtectionSetting::Range {
+  AllowForgeryProtectionSetting() {
+    this.getReceiver() instanceof Config::ActionControllerNode and
+    this.getMethodName() = "allow_forgery_protection="
   }
+
+  override boolean getVerificationSetting() { result = this.getValue() }
 }
 
-/** Holds if `node` can contain `value`. */
-private predicate hasBooleanValue(DataFlow::Node node, boolean value) {
-  exists(DataFlow::LocalSourceNode literal |
-    literal.asExpr().getExpr().(BooleanLiteral).getValue() = value and
-    literal.flowsTo(node)
-  )
+/**
+ * Sets the cipher to be used for encrypted cookies. Defaults to "aes-256-gcm".
+ * This can be set to any cipher supported by
+ * https://ruby-doc.org/stdlib-2.7.1/libdoc/openssl/rdoc/OpenSSL/Cipher.html
+ */
+private class EncryptedCookieCipherSetting extends Settings::StringlikeSetting,
+  CookieSecurityConfigurationSetting::Range {
+  EncryptedCookieCipherSetting() {
+    this.getReceiver() instanceof Config::ActionDispatchNode and
+    this.getMethodName() = "encrypted_cookie_cipher="
+  }
+
+  OpenSSLCipher getCipher() { this.getValueText() = result.getName() }
+
+  OpenSSLCipher getDefaultCipher() { result.getName() = "aes-256-gcm" }
+
+  override string getSecurityWarningMessage() {
+    this.getCipher().isWeak() and
+    result = this.getValueText() + " is a weak cipher."
+  }
 }
 
-// `<actionControllerConfig>.allow_forgery_protection = <verificationSetting>`
-private DataFlow::CallNode getAnAllowForgeryProtectionCall(boolean verificationSetting) {
-  // exclude some test configuration
-  not (
-    result.getLocation().getFile().getRelativePath().matches("%test/%") or
-    result.getLocation().getFile().getStem() = "test"
-  ) and
-  result.getReceiver() instanceof ActionControllerConfigNode and
-  result.asExpr().getExpr().(MethodCall).getMethodName() = "allow_forgery_protection=" and
-  hasBooleanValue(result.getArgument(0), verificationSetting)
+/**
+ * If true, signed and encrypted cookies will use the AES-256-GCM cipher rather
+ * than the older AES-256-CBC cipher. Defaults to true.
+ */
+private class UseAuthenticatedCookieEncryptionSetting extends Settings::BooleanSetting,
+  CookieSecurityConfigurationSetting::Range {
+  UseAuthenticatedCookieEncryptionSetting() {
+    this.getReceiver() instanceof Config::ActionDispatchNode and
+    this.getMethodName() = "use_authenticated_cookie_encryption="
+  }
+
+  boolean getDefaultValue() { result = true }
+
+  override string getSecurityWarningMessage() {
+    this.getValue() = false and
+    result = this.getSettingString() + " selects a weaker block mode for authenticated cookies."
+  }
 }
 
+// TODO: this may also take a proc that specifies how to handle specific requests
 /**
- * A `DataFlow::Node` that may enable or disable Rails CSRF protection in
- * production code.
+ * Configures the default value of the `SameSite` attribute when setting cookies.
+ * Valid string values are `strict`, `lax`, and `none`.
+ * The attribute can be omitted by setting this to `nil`.
+ * The default if unset is `:lax`.
+ * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#strict
  */
-private class AllowForgeryProtectionSetting extends CSRFProtectionSetting::Range {
-  private boolean verificationSetting;
+private class CookiesSameSiteProtectionSetting extends Settings::NillableStringlikeSetting,
+  CookieSecurityConfigurationSetting::Range {
+  CookiesSameSiteProtectionSetting() {
+    this.getReceiver() instanceof Config::ActionDispatchNode and
+    this.getMethodName() = "cookies_same_site_protection="
+  }
 
-  AllowForgeryProtectionSetting() { this = getAnAllowForgeryProtectionCall(verificationSetting) }
+  string getDefaultValue() { result = "lax" }
 
-  override boolean getVerificationSetting() { result = verificationSetting }
+  override string getSecurityWarningMessage() {
+    // Mark unset as being potentially dangerous, as not all browsers default to "lax"
+    this.getStringValue().toLowerCase() = "none" and
+    result = "Setting 'SameSite' to 'None' may make an application more vulnerable to CSRF attacks."
+    or
+    this.isNilValue() and
+    result = "Unsetting 'SameSite' can disable same-site cookie restrictions in some browsers."
+  }
 }
 // TODO: initialization hooks, e.g. before_configuration, after_initialize...
 // TODO: initializers
@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+lgtm,codescanning
+* Added a new query, `rb/weak-cookie-configuration`. The query finds cases where cookie configuration options are set to values that may make an application more vulnerable to certain attacks.
@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+  <overview>
+    <p>
+      Cookies can be used for security measures, such as authenticating a user
+      based on cookies sent with a request. Misconfiguration of cookie settings
+      in a web application can expose users to attacks that compromise these
+      security measures.
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      Modern web frameworks typically have good default configuration for cookie
+      settings. If an application overrides these settings, then take care to
+      ensure that these changes are necessary and that they don't weaken the
+      cookie configuration.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      In the first example, the value of
+      <code>config.action_dispatch.cookies_same_site_protection</code> is set to
+      <code>:none</code>. This has the effect of setting the default
+      <code>SameSite</code> attribute sent by the server when setting a cookie
+      to <code>None</code> rather than the default of <code>Lax</code>. This may
+      make the application more vulnerable to cross-site request forgery
+      attacks.
+    </p>
+
+    <p>
+      In the second example, this option is instead set to <code>:strict</code>.
+      This is a stronger restriction than the default of <code>:lax</code>, and
+      doesn't compromise on cookie security.
+    </p>
+
+    <sample src="examples/weak_cookie_configuration.rb" />
+  </example>
+
+  <references>
+    <li>OWASP: <a href="https://owasp.org/www-community/SameSite">SameSite</a>.</li>
+    <li>Rails: <a href="https://guides.rubyonrails.org/configuring.html#configuring-action-dispatch">Configuring Action Dispatch</a>.</li>
+  </references>
+</qhelp>