Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address CVE-2019-16782 #151

Merged
merged 4 commits into from
Mar 10, 2021
Merged

Conversation

kratob
Copy link
Contributor

@kratob kratob commented Dec 22, 2019

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.

end

def get_session_with_fallback(sid)
if sid && !self.class.private_session_id?(sid.public_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@thorsteneckel thorsteneckel Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ✅

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

def secure!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this method was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@thorsteneckel thorsteneckel Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

borama commented Jan 10, 2020

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
Copy link

thegcat commented Jan 22, 2020

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
Copy link

thegcat commented Mar 10, 2020

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

@kalivara
Copy link

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

@gatopanx
Copy link

Bump

@gatopanx
Copy link

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

@nevinera
Copy link

nevinera commented Mar 9, 2021

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 :-(

pezholio added a commit to dxw/transition that referenced this pull request Mar 9, 2021
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)
pezholio added a commit to dxw/transition that referenced this pull request Mar 9, 2021
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)
pezholio added a commit to dxw/transition that referenced this pull request Mar 9, 2021
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
Copy link

denny commented Mar 9, 2021

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 )

@ghiculescu

This comment has been minimized.

@kratob
Copy link
Contributor Author

kratob commented Mar 9, 2021

@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
Copy link
Member

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

@ghiculescu
Copy link
Member

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

Copy link
Contributor

@thorsteneckel thorsteneckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🚢

end

def get_session_with_fallback(sid)
if sid && !self.class.private_session_id?(sid.public_id)
Copy link
Contributor

@thorsteneckel thorsteneckel Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ✅

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

def secure!
Copy link
Contributor

@thorsteneckel thorsteneckel Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 💡

Copy link
Member

@sikachu sikachu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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 🙏

This was referenced Mar 10, 2021
suhiltzaile pushed a commit to bebanjo/activerecord-session_store that referenced this pull request Apr 12, 2021
suhiltzaile pushed a commit to bebanjo/activerecord-session_store that referenced this pull request Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.