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

Optionally allow ActionCable requests from the same host as origin #26568

Merged
merged 1 commit into from Oct 11, 2016

Conversation

@skateman
Copy link
Contributor

@skateman skateman commented Sep 21, 2016

Summary

When the allow_same_origin_as_host is set to true, the request
forgery protection permits HTTP_ORIGIN values starting with the
corresponding proto:// prefix followed by HTTP_HOST. This way
it is not required to specify the list of allowed URLs.

Other Information

https://groups.google.com/forum/?fromgroups#!topic/rubyonrails-core/4U5pKIgtJFU

@rails-bot
Copy link

@rails-bot rails-bot commented Sep 21, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@skateman
Copy link
Contributor Author

@skateman skateman commented Sep 21, 2016

FYI @matthewd

@skateman skateman force-pushed the skateman:cable-sameorigin-as-host branch Sep 21, 2016
@matthewd
Copy link
Member

@matthewd matthewd commented Sep 21, 2016

I think we can enable this always, without needing a config option.

The point of the origin check is to protect against cross-origin websocket connections, which could give privileged access because of a cookie; if the origin matches the request hostname, then the originating page already had access to any such cookie... right?

cc @rails/security

actioncable/lib/action_cable/connection/base.rb Outdated
if Array(server.config.allowed_request_origins).any? { |allowed_origin| allowed_origin === env["HTTP_ORIGIN"] }
true
elsif server.config.allow_same_origin_as_host && env["HTTP_ORIGIN"].start_with?("#{proto}://#{env['HTTP_HOST']}")

This comment has been minimized.

@matthewd

matthewd Sep 21, 2016
Member

Origin doesn't have a suffix, so this should be an == comparison.

This comment has been minimized.

@skateman

skateman Sep 21, 2016
Author Contributor

@matthewd fixed

@matthewd matthewd assigned matthewd and unassigned rafaelfranca Sep 21, 2016
@skateman skateman force-pushed the skateman:cable-sameorigin-as-host branch Sep 21, 2016
actioncable/test/connection/cross_site_forgery_test.rb Outdated
@server.config.allow_same_origin_as_host = true
assert_origin_allowed "http://#{HOST}"
assert_origin_not_allowed "http://hax.com"
assert_origin_not_allowed "http://rails.co.uk/"

This comment has been minimized.

@matthewd

matthewd Sep 21, 2016
Member

I think the allowed one should be a literal string.. probably. And this last line needs to lose the trailing / to have much meaning.

Also, is this test leaving the config option set? The teardown makes sure to reset the others.

This comment has been minimized.

@skateman

skateman Sep 21, 2016
Author Contributor

@matthewd both fixed

@skateman skateman force-pushed the skateman:cable-sameorigin-as-host branch Sep 21, 2016
When the `allow_same_origin_as_host` is set to `true`, the request
forgery protection permits `HTTP_ORIGIN` values starting with the
corresponding `proto://` prefix followed by `HTTP_HOST`. This way
it is not required to specify the list of allowed URLs.
@skateman skateman force-pushed the skateman:cable-sameorigin-as-host branch to 268c340 Sep 21, 2016
@jeremy
Copy link
Member

@jeremy jeremy commented Sep 21, 2016

if the origin matches the request hostname

That protects against same-origin cookies (barring DNS rebinding which would poke through in any case) conferring auth, but not against accepting the connection in the first place, affecting apps with non-authenticated conns and (marginally) increasing DoS risk.

@matthewd
Copy link
Member

@matthewd matthewd commented Sep 21, 2016

Even DNS rebinding is okay, because that only gets the attacker access to cookies belonging to their rebound name, not the "real" one.

If the code making the request came from the same origin as the hostname you've been supplied (here, a DNS rebinding disclaimer is indeed required), then regardless of whether the hostname is the real one or an attacker-owned fake pointing at your IP, it's your code.

AIUI, the only real exposure here is if the connection-initiating-party can fake one or both of those headers (giving a host header that didn't resolve to this server, or an origin that isn't true)... and at that point, you're dealing with something other than a browser, which would be just as capable of claiming whatever origin you're explicitly checking for.

@matthewd matthewd merged commit 268c340 into rails:master Oct 11, 2016
2 checks passed
2 checks passed
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
matthewd added a commit that referenced this pull request Oct 11, 2016
Optionally allow ActionCable requests from the same host as origin
@matthewd
Copy link
Member

@matthewd matthewd commented Oct 11, 2016

Defaulted to on in dae4044; I decided to stop short of removing the option altogether.

@skateman skateman deleted the skateman:cable-sameorigin-as-host branch Oct 11, 2016
@matthewd
Copy link
Member

@matthewd matthewd commented Oct 11, 2016

Backported to 5-0-stable: db70978 + 549d732

matthewd added a commit that referenced this pull request Oct 11, 2016
Optionally allow ActionCable requests from the same host as origin
matthewd referenced this pull request Oct 11, 2016
WebSocket always defers the decision to the server, because it didn't
have to deal with legacy compatibility... but the same-origin policy is
still a reasonable default.

Origin checks do not protect against a directly connecting attacker --
they can lie about their host, but can also lie about their origin.
Origin checks protect against a connection from 3rd-party controlled
script in a context where a victim browser's cookies will be passed
along. And if an attacker has breached that protection, they've already
compromised the HTTP session, so treating the WebSocket connection in
the same way seems reasonable.

In case this logic proves incorrect (or anyone just wants to be more
paranoid), we retain a config option to disable it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants