Address CVE-2019-16782 #151

kratob · 2019-12-22T13:40:15Z

rack and rails have fixed vulnerability CVE-2019-16782 by introducing secure variants of their session stores.

This PR makes ActiveRecord::SessionStore use the new ActionDispatch::Session::AbstractSecureStore, preventing the same attack.

rafaelfranca · 2019-12-27T20:46:29Z

lib/action_dispatch/session/active_record_store.rb

+      end
+
+      def get_session_with_fallback(sid)
+        if sid && !self.class.private_session_id?(sid.public_id)


Why this check? How the user will get access to a private id?

If we don't add this protection, the fallback mechanism allows an attacker to guess the private id with a timing attack similar to the original vulnerability.

Imagine public_id just being a private_id in disguise. The attacker would send an public_key which is in fact a private_id! Line 136 will fail because public_id (aka "evil private_id") will be a double hashed private_id. This will cause line 139 to be executed. Which is basically the same insecure behaviour is as it is right now.
This line prevents this. If public_id is in fact a private_id it won't get used at all and no lookup will be performed.
Instead a new and secure key/session will be generated and used ✅

rafaelfranca · 2019-12-27T20:46:49Z

lib/active_record/session_store/session.rb

@@ -71,6 +71,21 @@ def loaded?
        @data
      end

+      def secure!


Why this method was added?

I guess this should probably be documented.

Normally, sessions are only upgraded to secure sessions, when a user makes a request using the insecure session. This allows existing user sessions to be upgraded to secure sessions while the users are offline.

This will allow maintainers to create a migration which basically performs a ActionDispatch::Session::ActiveRecordStore.session_class.find_each(&:secure!) to migrate all existing insecure sessions to secure ones. The next lookup with the private_id will find the secured value in the DB and just work as for new secure sessions. The fallback case won't be executed and no additional DB query will be performed (faster/lower load).

An remark in the README.md would be great though 💡

borama · 2020-01-10T19:35:17Z

May I add a little bit of our own context to this PR? I got here while searching for a fix for a Sidekiq web interface issue which started after upgrading rack to 2.0.8 (a release addressing the CVE). Mounting the sidekiq UI in Rails routes now triggers a rack exception in ActiveRecordStorage:

ActiveRecord::StatementInvalid (can't quote Rack::Session::SessionId):

activerecord (6.0.2.1) lib/active_record/core.rb:206:in `rescue in find_by'
activerecord (6.0.2.1) lib/active_record/core.rb:203:in `find_by'
activerecord (6.0.2.1) lib/active_record/dynamic_matchers.rb:66:in `find_by_session_id'
activerecord-session_store (1.1.3) lib/action_dispatch/session/active_record_store.rb:124:in `block in get_session_model'
activerecord-session_store (1.1.3) lib/active_record/session_store/extension/logger_silencer.rb:47:in `silence_logger'
activerecord-session_store (1.1.3) lib/action_dispatch/session/active_record_store.rb:123:in `get_session_model'
activerecord-session_store (1.1.3) lib/action_dispatch/session/active_record_store.rb:83:in `block in write_session'
activerecord-session_store (1.1.3) lib/active_record/session_store/extension/logger_silencer.rb:47:in `silence_logger'
activerecord-session_store (1.1.3) lib/action_dispatch/session/active_record_store.rb:82:in `write_session'
rack (2.0.8) lib/rack/session/abstract/id.rb:381:in `commit_session'
rack (2.0.8) lib/rack/session/abstract/id.rb:261:in `context'
rack (2.0.8) lib/rack/session/abstract/id.rb:253:in `call'
actionpack (6.0.2.1) lib/action_dispatch/middleware/cookies.rb:648:in `call'
activerecord (6.0.2.1) lib/active_record/migration.rb:567:in `call'
actionpack (6.0.2.1) lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
activesupport (6.0.2.1) lib/active_support/callbacks.rb:101:in `run_callbacks'
actionpack (6.0.2.1) lib/action_dispatch/middleware/callbacks.rb:26:in `call' 
...

Some most relevant info about the rack breaking change can be found in this comment by Tenderlove (and the whole thread).

This PR is very similar to the Redis session store fix which fixes a problem very similar to ours. I tried to test this PR with Sidekiq Web UI and rack 2.0.8 and I would like to confirm that this PR indeed fixes our issue and the web GUI runs just fine. We would thus certainly appreciate if this PR was merged. :) Thank you all!

update: the exact same problem now occurs in the Flipper gem UI, too, which is also mounted in routes. And this PR fixes the Flipper issue, too.

thegcat · 2020-01-22T20:01:11Z

We are interested in an AR::SessionStore solution that uses the Rack AbstractSecureStore, but we would prefer an upstream solution rather than using the Rails LTS fork.

Is a fix intended in AR::SessionStore, and is there any help we could provide, for example by testing this or another Pull Request?

thegcat · 2020-03-10T08:28:15Z

Any news? Or should we just go ahead and use the rails-lts fork and/or maintain our own fork?

kalivara · 2020-03-25T15:40:40Z

@rafaelfranca, is this fix advancing? I think is very important and should be addressed. Thanks!

gatopanx · 2020-05-20T19:57:25Z

Bump

gatopanx · 2020-05-20T20:22:31Z

Short term fix: gem 'activerecord-session_store', github: 'rails-lts/activerecord-session_store', branch: 'secure-session-store'

nevinera · 2021-03-09T12:45:37Z

bundle-audit is now flagging this gem:

Updated ruby-advisory-db
ruby-advisory-db: 485 advisories
Name: activerecord-session_store
Version: 1.1.3
Advisory: CVE-2019-25025
Criticality: Medium
URL: GHSA-cvw2-xj8r-mjf7
Title: activerecord-session_store Timing Attack
Solution: remove or disable this gem until a patch is available!

I hate to point at a fork's branch in git for a production dependency :-(

This addresses CVE-2019-16782 There has been a vulnerability in the wild[1] around session hijacks in Rack and related frameworks for a while now, but this has been fixed in Rack and Rails for a while now. There's a fix for the upstream version of ActiverecordSessionStore since late 2019[2], but this hasn't been merged yet. We weren't aware of this issue until recently, as it's only just been added to the Ruby Advisory DB[3] This uses a fork of the upstream gem, as suggested in the original PR[4] to fix the immediate issue. [1] https://nvd.nist.gov/vuln/detail/CVE-2019-16782 [2] rails/activerecord-session_store#151 [3] rubysec/ruby-advisory-db#462 [4] rails/activerecord-session_store#151 (comment)

This addresses CVE-2019-16782 There has been a [vulnerability in the wild][1] around session hijacks in Rack and related frameworks for a while now, but this has been fixed in Rack and Rails for a while now. There's a [fix for the upstream version of ActiverecordSessionStore since late 2019][2], but this hasn't been merged yet. We weren't aware of this issue until recently, as it's only [just been added to the Ruby Advisory DB][3] This uses a fork of the upstream gem, [as suggested in the original PR][4] to fix the immediate issue. [1] https://nvd.nist.gov/vuln/detail/CVE-2019-16782 [2] rails/activerecord-session_store#151 [3] rubysec/ruby-advisory-db#462 [4] rails/activerecord-session_store#151 (comment)

This addresses CVE-2019-16782 There has been a [vulnerability in the wild][1] around session hijacks in Rack and related frameworks for a while now, but this has been fixed in Rack and Rails for a while now. There's a [fix for the upstream version of ActiverecordSessionStore since late 2019][2], but this hasn't been merged yet. We weren't aware of this issue until recently, as it's only [just been added to the Ruby Advisory DB][3] This uses a fork of the upstream gem, [as suggested in the original PR][4] to fix the immediate issue. [1]:https://nvd.nist.gov/vuln/detail/CVE-2019-16782 [2]:rails/activerecord-session_store#151 [3]:rubysec/ruby-advisory-db#462 [4]:rails/activerecord-session_store#151 (comment)

denny · 2021-03-09T15:21:14Z

I hate to point at a fork's branch in git for a production dependency :-(

I can't point at the above fork, it doesn't work with Ruby 3.0:

      RuntimeError:
        class variable @@silencer of ActiveSupport::Logger is overtaken by Logger

( Fixed in master last year, but not released so far: #159 )

kratob · 2021-03-09T16:26:14Z

@kratob are you able to fix merge conflicts? (if you are busy and don't have time, I can volunteer, just need access to your fork)

I wasn't aware this has become unmergable. I've resolved the conflicts.

ghiculescu · 2021-03-09T16:27:20Z

nice, that should also fix @denny's issue.

ghiculescu · 2021-03-09T16:28:06Z

@rafaelfranca or @sikachu are you able to take another look?

thorsteneckel

I had some troubles to reason about the changes too. However, after spending some time these look good to me ❤️ I added some comments in the hope to make it easier for the maintainers to reason about the changes as well. LGTM 🚢

thorsteneckel · 2021-03-09T17:05:06Z

lib/action_dispatch/session/active_record_store.rb

+      end
+
+      def get_session_with_fallback(sid)
+        if sid && !self.class.private_session_id?(sid.public_id)


Imagine public_id just being a private_id in disguise. The attacker would send an public_key which is in fact a private_id! Line 136 will fail because public_id (aka "evil private_id") will be a double hashed private_id. This will cause line 139 to be executed. Which is basically the same insecure behaviour is as it is right now.
This line prevents this. If public_id is in fact a private_id it won't get used at all and no lookup will be performed.
Instead a new and secure key/session will be generated and used ✅

thorsteneckel · 2021-03-09T17:08:35Z

lib/active_record/session_store/session.rb

@@ -71,6 +71,21 @@ def loaded?
        @data
      end

+      def secure!


This will allow maintainers to create a migration which basically performs a ActionDispatch::Session::ActiveRecordStore.session_class.find_each(&:secure!) to migrate all existing insecure sessions to secure ones. The next lookup with the private_id will find the secured value in the DB and just work as for new secure sessions. The fallback case won't be executed and no additional DB query will be performed (faster/lower load).

An remark in the README.md would be great though 💡

sikachu

This looks good to me.

As @thorsteneckel said, @kratob would you mind submitting another PR documenting the secure! method and how to use it in the README.md?

I'm going to merge this one in since it's been so long going back and forth. Once we get the documentation for secure! method in then we can cut a new version.

thorsteneckel · 2021-03-10T08:48:27Z

Thanks a ton @sikachu ! My proposal for the README change can be found in #175. Would be great if @kratob could give his thumb up. I'd love to see a soonish release 🙏

Address CVE-2019-16782

kratob added 2 commits December 21, 2019 15:10

secure session store [CVE-2019-16782]

81f551a

prevent unnecessary database writes

7564029

rafaelfranca reviewed Dec 27, 2019

View reviewed changes

add documentation for a new method

d091c7e

borama mentioned this pull request Jan 10, 2020

Sidekiq web failed to start with Ruby 2.7 when using active_record_store #152

Closed

zofrex mentioned this pull request Mar 5, 2021

Add CVE-2019-25025 for activerecord-session_store rubysec/ruby-advisory-db#462

Closed

pezholio mentioned this pull request Mar 9, 2021

Address CVE-2019-16782 dxw/transition#155

Closed

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'upstream/master' into secure-session-store

532a9a5

thorsteneckel reviewed Mar 9, 2021

View reviewed changes

zurbergram mentioned this pull request Mar 9, 2021

Remove Activerecord session store department-of-veterans-affairs/gibct-data-service#798

Merged

4 tasks

sikachu approved these changes Mar 10, 2021

View reviewed changes

sikachu merged commit 9d4dd11 into rails:master Mar 10, 2021

sikachu mentioned this pull request Mar 10, 2021

Vulnerabilities found activerecord-session_store Timing Attack #174

Closed

thorsteneckel mentioned this pull request Mar 10, 2021

Added documentation on how to mitigate CVE-2015-9284. #175

Merged

sikachu mentioned this pull request Mar 10, 2021

Update activerecord-session_store advisory info rubysec/ruby-advisory-db#463

Merged

suhiltzaile pushed a commit to bebanjo/activerecord-session_store that referenced this pull request Apr 12, 2021

Merge pull request rails#151 from rails-lts/secure-session-store

e7f6309

Address CVE-2019-16782

suhiltzaile pushed a commit to bebanjo/activerecord-session_store that referenced this pull request Apr 12, 2021

Merge pull request rails#151 from rails-lts/secure-session-store

c81ddfd

stevenharman mentioned this pull request Mar 28, 2023

Allow use of secure session only docsend/activerecord-session_store#1

Open

tmandke mentioned this pull request Mar 28, 2023

Allow use of secure session only #199

Open

biinari mentioned this pull request May 12, 2023

Incomplete fix to secure session store CVE-2019-16782 roidrage/redis-session-store#145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address CVE-2019-16782 #151

Address CVE-2019-16782 #151

kratob commented Dec 22, 2019

rafaelfranca Dec 27, 2019

kratob Dec 30, 2019

thorsteneckel Mar 9, 2021 •

edited

Loading

rafaelfranca Dec 27, 2019

kratob Dec 30, 2019

thorsteneckel Mar 9, 2021 •

edited

Loading

borama commented Jan 10, 2020 •

edited

Loading

thegcat commented Jan 22, 2020

thegcat commented Mar 10, 2020

kalivara commented Mar 25, 2020

gatopanx commented May 20, 2020

gatopanx commented May 20, 2020

nevinera commented Mar 9, 2021

denny commented Mar 9, 2021 •

edited

Loading

This comment has been minimized.

kratob commented Mar 9, 2021

ghiculescu commented Mar 9, 2021

ghiculescu commented Mar 9, 2021

thorsteneckel left a comment •

edited

Loading

thorsteneckel Mar 9, 2021 •

edited

Loading

thorsteneckel Mar 9, 2021 •

edited

Loading

sikachu left a comment

thorsteneckel commented Mar 10, 2021

Address CVE-2019-16782 #151

Address CVE-2019-16782 #151

Conversation

kratob commented Dec 22, 2019

rafaelfranca Dec 27, 2019

Choose a reason for hiding this comment

kratob Dec 30, 2019

Choose a reason for hiding this comment

thorsteneckel Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

rafaelfranca Dec 27, 2019

Choose a reason for hiding this comment

kratob Dec 30, 2019

Choose a reason for hiding this comment

thorsteneckel Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

borama commented Jan 10, 2020 • edited Loading

thegcat commented Jan 22, 2020

thegcat commented Mar 10, 2020

kalivara commented Mar 25, 2020

gatopanx commented May 20, 2020

gatopanx commented May 20, 2020

nevinera commented Mar 9, 2021

denny commented Mar 9, 2021 • edited Loading

This comment has been minimized.

kratob commented Mar 9, 2021

ghiculescu commented Mar 9, 2021

ghiculescu commented Mar 9, 2021

thorsteneckel left a comment • edited Loading

Choose a reason for hiding this comment

thorsteneckel Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

thorsteneckel Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

sikachu left a comment

Choose a reason for hiding this comment

thorsteneckel commented Mar 10, 2021

thorsteneckel Mar 9, 2021 •

edited

Loading

thorsteneckel Mar 9, 2021 •

edited

Loading

borama commented Jan 10, 2020 •

edited

Loading

denny commented Mar 9, 2021 •

edited

Loading

thorsteneckel left a comment •

edited

Loading

thorsteneckel Mar 9, 2021 •

edited

Loading

thorsteneckel Mar 9, 2021 •

edited

Loading