Ruby: Add `rb/weak-cookie-configuration` query #7313

alexrford · 2021-12-05T23:54:33Z

This PR adds a query to check for potential weak cookie configuration options for Rails - in theory we could extend this to cover other frameworks.

The defaults for Rails seem quite safe here, so we only look for cases where these defaults are overriden. In particular, we check for three settings in non-test configuration:

config.action_dispatch.encrypted_cookie_cipher set to a cipher that we believe to be weak
config.action_dispatch.use_authenticated_cookie_encryption set to false
config.action_dispatch.cookies_same_site_protection set to either :none/"none"/etc. or nil

In the last of these, the option can also be set to a proc that can take a request and behave differently depending on this request - this PR doesn't cover that case.

The text was updated successfully, but these errors were encountered:

nickrolfe

Very nice!

nickrolfe · 2021-12-15T18:19:47Z

ruby/change-notes/2021-12-06-weak-cookie-configuration.md

@@ -0,0 +1,2 @@
+lgtm,codescanning


The new guidance in #7400 suggests this changenote should go in ruby/ql/src/change-notes.

Indeed. Also, note the new format for the metadata at the top of the change note.

Hopefully in the correct format now.

ruby/ql/lib/codeql/ruby/Concepts.qll

nickrolfe · 2021-12-15T18:19:47Z

ruby/ql/lib/codeql/ruby/frameworks/Rails.qll

+    loc.getFile().getStem() = "test"
+  }
+
+  private DataFlow::Node getTransitiveReceiver(DataFlow::CallNode c) {


I think this predicate is equivalent to c.getReceiver+().

Nice! For some reason I thought that +/* relied on the result type being guaranteed to have a member predicate with the same name and result type, but this doesn't seem to be the case.

nickrolfe

LGTM

alexrford · 2021-12-16T17:05:08Z

I'll hold off on merging this until #7273 is in.

Co-authored-by: Nick Rolfe <nickrolfe@github.com>

github-actions · 2021-12-22T17:57:43Z

QHelp previews:

ruby/ql/src/queries/security/cwe-732/WeakCookieConfiguration.qhelp

Weak cookie configuration

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.

Recommendation

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.

Example

In the first example, the value of config.action_dispatch.cookies_same_site_protection is set to :none. This has the effect of setting the default SameSite attribute sent by the server when setting a cookie to None rather than the default of Lax. This may make the application more vulnerable to cross-site request forgery attacks.

In the second example, this option is instead set to :strict. This is a stronger restriction than the default of :lax, and doesn't compromise on cookie security.

module App
  class Application < Rails::Application
    # Sets default `Set-Cookie` `SameSite` attribute to `None`
    config.action_dispatch.cookies_same_site_protection = :none

    # Sets default `Set-Cookie` `SameSite` attribute to `Strict`
    config.action_dispatch.cookies_same_site_protection = :strict
  end
end

References

OWASP: SameSite.
Rails: Configuring Action Dispatch.
Common Weakness Enumeration: CWE-732.
Common Weakness Enumeration: CWE-1275.

alexrford · 2021-12-22T17:58:44Z

Leaving a note-to-self to double check this, as the merge conflict resolution for Rails.qll was a little bit hairy.

github-actions · 2022-01-04T15:38:27Z

QHelp previews:

ruby/ql/src/queries/security/cwe-732/WeakCookieConfiguration.qhelp

Weak cookie configuration

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.

Recommendation

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.

Example

In the first example, the value of config.action_dispatch.cookies_same_site_protection is set to :none. This has the effect of setting the default SameSite attribute sent by the server when setting a cookie to None rather than the default of Lax. This may make the application more vulnerable to cross-site request forgery attacks.

In the second example, this option is instead set to :strict. This is a stronger restriction than the default of :lax, and doesn't compromise on cookie security.

module App
  class Application < Rails::Application
    # Sets default `Set-Cookie` `SameSite` attribute to `None`
    config.action_dispatch.cookies_same_site_protection = :none

    # Sets default `Set-Cookie` `SameSite` attribute to `Strict`
    config.action_dispatch.cookies_same_site_protection = :strict
  end
end

References

OWASP: SameSite.
Rails: Configuring Action Dispatch.
Common Weakness Enumeration: CWE-732.
Common Weakness Enumeration: CWE-1275.

github-actions · 2022-01-04T15:44:35Z

QHelp previews:

ruby/ql/src/queries/security/cwe-732/WeakCookieConfiguration.qhelp

Weak cookie configuration

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.

Recommendation

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.

Example

In the first example, the value of config.action_dispatch.cookies_same_site_protection is set to :none. This has the effect of setting the default SameSite attribute sent by the server when setting a cookie to None rather than the default of Lax. This may make the application more vulnerable to cross-site request forgery attacks.

In the second example, this option is instead set to :strict. This is a stronger restriction than the default of :lax, and doesn't compromise on cookie security.

module App
  class Application < Rails::Application
    # Sets default `Set-Cookie` `SameSite` attribute to `None`
    config.action_dispatch.cookies_same_site_protection = :none

    # Sets default `Set-Cookie` `SameSite` attribute to `Strict`
    config.action_dispatch.cookies_same_site_protection = :strict
  end
end

References

OWASP: SameSite.
Rails: Configuring Action Dispatch.
Common Weakness Enumeration: CWE-732.
Common Weakness Enumeration: CWE-1275.

github-actions · 2022-01-04T16:43:00Z

QHelp previews:

ruby/ql/src/queries/security/cwe-732/WeakCookieConfiguration.qhelp

Weak cookie configuration

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.

Recommendation

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.

Example

In the first example, the value of config.action_dispatch.cookies_same_site_protection is set to :none. This has the effect of setting the default SameSite attribute sent by the server when setting a cookie to None rather than the default of Lax. This may make the application more vulnerable to cross-site request forgery attacks.

In the second example, this option is instead set to :strict. This is a stronger restriction than the default of :lax, and doesn't compromise on cookie security.

module App
  class Application < Rails::Application
    # Sets default `Set-Cookie` `SameSite` attribute to `None`
    config.action_dispatch.cookies_same_site_protection = :none

    # Sets default `Set-Cookie` `SameSite` attribute to `Strict`
    config.action_dispatch.cookies_same_site_protection = :strict
  end
end

References

OWASP: SameSite.
Rails: Configuring Action Dispatch.
Common Weakness Enumeration: CWE-732.
Common Weakness Enumeration: CWE-1275.

tausbn · 2022-01-04T18:01:04Z

It's not clear to me why this needs a review from the Python team. Am I missing something?

alexrford · 2022-01-04T18:04:44Z

It's not clear to me why this needs a review from the Python team. Am I missing something?

Thanks for noticing this, the Python and JS review requests weren't necessary. They were left over from before the merge conflict was resolved (as this PR included changes to CryptoAlgorithms.qll that were merged in #7273).

nickrolfe · 2022-01-05T13:23:03Z

ruby/ql/lib/codeql/ruby/frameworks/Rails.qll

+    }
+  }
+
+  private class LiteralSetting extends Setting {


I noticed some results for this class are actually reads, not writes. For example: https://github.com/gitlabhq/gitlabhq/blob/6d29831123c8c806dc75d64e29b7691576cbea7f/lib/gitlab/database.rb#L187

It's a private class, and the way it gets used with specific setter method names means it should be ok, but I wonder if we should save ourselves some headaches for future uses, either by restricting the class more, or adding a note in a comment.

Definitely makes sense to restrict these classes further - I've restricted Setting nodes to ones that correspond to a SetterMethodCall.

github-actions · 2022-01-05T14:12:54Z

QHelp previews:

ruby/ql/src/queries/security/cwe-732/WeakCookieConfiguration.qhelp

Weak cookie configuration

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.

Recommendation

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.

Example

In the first example, the value of config.action_dispatch.cookies_same_site_protection is set to :none. This has the effect of setting the default SameSite attribute sent by the server when setting a cookie to None rather than the default of Lax. This may make the application more vulnerable to cross-site request forgery attacks.

In the second example, this option is instead set to :strict. This is a stronger restriction than the default of :lax, and doesn't compromise on cookie security.

module App
  class Application < Rails::Application
    # Sets default `Set-Cookie` `SameSite` attribute to `None`
    config.action_dispatch.cookies_same_site_protection = :none

    # Sets default `Set-Cookie` `SameSite` attribute to `Strict`
    config.action_dispatch.cookies_same_site_protection = :strict
  end
end

References

OWASP: SameSite.
Rails: Configuring Action Dispatch.
Common Weakness Enumeration: CWE-732.
Common Weakness Enumeration: CWE-1275.

nickrolfe

lgtm

alexrford added the Ruby label Dec 5, 2021

alexrford requested a review from as a code owner Dec 5, 2021

github-actions bot added the documentation label Dec 5, 2021

alexrford force-pushed the ruby/rails-cookie-config branch from b6cd9ae to 3128380 Dec 6, 2021

alexrford force-pushed the ruby/crypto-algorithms branch from b80633a to 0481385 Dec 11, 2021

alexrford force-pushed the ruby/rails-cookie-config branch from 3128380 to d69e5f1 Dec 13, 2021

nickrolfe reviewed Dec 15, 2021

View changes

alexrford mentioned this pull request Dec 16, 2021

Ruby: add CryptoAlgorithms library #7273

Merged

nickrolfe reviewed Dec 16, 2021

View changes

alexrford force-pushed the ruby/crypto-algorithms branch from cb516f8 to 3da98ec Dec 22, 2021

alexrford requested review from as code owners Dec 22, 2021

Base automatically changed from ruby/crypto-algorithms to main Dec 22, 2021

alexrford and others added 11 commits Dec 22, 2021

Ruby: add CookieSecurityConfigurationSetting concept

8a3d1fe

Ruby: add rb/weak-cookie-configuration query

737f733

Ruby: Tidy Rails.qll to make adding new settings modeling easier

5ce6e63

Ruby: Model some Rails cookie configuration settings

8976469

Ruby: add some rb/weak-cookie-configuration tests

71c5711

Ruby: add a change note for rb/weak-cookie-configuration

db967bd

Ruby: fix import

2cd0215

Ruby: behaviour -> behavior

9821c4a

Co-authored-by: Nick Rolfe <nickrolfe@github.com>

Ruby: remove unnecessary custom transitive version of getReceiver

d977e8a

Ruby: use new changenote format for rb/weak-cookie-configuration

7f01be7

Merge remote-tracking branch 'origin/main' into ruby/rails-cookie-config

7d3932d

alexrford dismissed nickrolfe’s stale review via 7d3932d Dec 22, 2021

alexrford force-pushed the ruby/rails-cookie-config branch from 38822b6 to 7d3932d Dec 22, 2021

alexrford marked this pull request as draft Dec 22, 2021

Merge branch 'main' into ruby/rails-cookie-config

dadaf25

Ruby: behaviour -> behavior

36ea360

Ruby: formatting

712972c

alexrford marked this pull request as ready for review Jan 4, 2022

alexrford removed request for Jan 4, 2022

nickrolfe reviewed Jan 5, 2022

View changes

Ruby: Restrict Rails Setting nodes to SetterMethodCalls

da8c745

nickrolfe approved these changes Jan 5, 2022

View changes

alexrford merged commit f935df9 into main Jan 5, 2022
28 checks passed

alexrford deleted the ruby/rails-cookie-config branch Jan 5, 2022

github / codeql Public

Ruby: Add rb/weak-cookie-configuration query #7313

Ruby: Add rb/weak-cookie-configuration query #7313

Conversation

alexrford commented Dec 5, 2021

nickrolfe left a comment

nickrolfe Dec 15, 2021

Choose a reason for hiding this comment

dbartol Dec 15, 2021 • edited

Choose a reason for hiding this comment

alexrford Dec 16, 2021

Choose a reason for hiding this comment

nickrolfe Dec 15, 2021

Choose a reason for hiding this comment

alexrford Dec 16, 2021

Choose a reason for hiding this comment

nickrolfe left a comment

alexrford commented Dec 16, 2021

github-actions bot commented Dec 22, 2021

Weak cookie configuration

Recommendation

Example

References

alexrford commented Dec 22, 2021

github-actions bot commented Jan 4, 2022

Weak cookie configuration

Recommendation

Example

References

github-actions bot commented Jan 4, 2022

Weak cookie configuration

Recommendation

Example

References

github-actions bot commented Jan 4, 2022

Weak cookie configuration

Recommendation

Example

References

tausbn commented Jan 4, 2022

alexrford commented Jan 4, 2022 • edited

nickrolfe Jan 5, 2022

Choose a reason for hiding this comment

alexrford Jan 5, 2022

Choose a reason for hiding this comment

github-actions bot commented Jan 5, 2022

Weak cookie configuration

Recommendation

Example

References

nickrolfe left a comment

Ruby: Add `rb/weak-cookie-configuration` query #7313

Ruby: Add `rb/weak-cookie-configuration` query #7313

dbartol Dec 15, 2021 •

edited

alexrford commented Jan 4, 2022 •

edited