Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

CSRF vuln on oauth grant approval #49

Merged
merged 2 commits into from

3 participants

@tmaher

PoC: http://www.breaksec.com/herokuoauthnir.html - this will grant the oauth client "PWN" access to all applications. While the Rack::CSRF module is present...

https://github.com/heroku/identity/blob/master/lib/identity/auth.rb#L101-L104

we're treating GETs the same as POSTs, so it won't get enforced.

@brandur brandur merged commit 29e975f into master
@brandur brandur deleted the brandur-close-csrf branch
@tmaher tmaher commented on the diff
test/auth_test.rb
@@ -81,6 +81,18 @@ def app
"?code=454118bc-902d-4a2c-9d5b-e2a2abb91f6e",
last_response.headers["Location"]
end
+
+ it "does not create an authorization if a user confirms via GET" do
+ post "/login", email: "kerry@heroku.com", password: "abcdefgh"
+
+ # post once to get parameters stored to session
+ post "/oauth/authorize", client_id: "untrusted"
+
+ # then try again, but with the wrong request method (should be POST)
+ get "/oauth/authorize", authorize: "Allow Access"
@tmaher
tmaher added a note

No client_id param?

@brandur Owner
brandur added a note

It gets stored in session on the first request (this is how it works IRL too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tbroyer

I'm coming here from https://blog.heroku.com/archives/2013/11/14/oauth-sso where you say OAuth is not clear and this led to this CSRF issue.
This may not be the best place to post this, but your blog doesn't accept comments; and this PR adds a similar comment to the code, so…

OAuth is actually really clear about the endpoint is: it's the URL the Client redirects to. OAuth doesn't care what you do to authorize: yes you'd probably POST something on the click to an Approve button, but OAuth doesn't care whether you do that with a form-post or through XHR (for example), and to which URL you're posting (Google posts to a distinct URL for example). The only thing that matters to OAuth is the interop between Clients and the Authorization Server, Clients need to redirect users to the Authorization Server (AS) at the Authorization Endpoint, then the AS has to redirect users back to the Clients at their Redirection Endpoint, and at some point (not necessarily at the Redirection Endpoint, there could be other redirections involved) Clients will make requests to the Token Endpoint of the AS. The Endpoints are defined as URLs that respond to specific HTTP methods and take specific parameters, everything else is up to the implementations are they don't change anything to the protocol between the Clients and the AS.

In the OAuth spec, the endpoints are all defined as HTTP resources that are used by the other party (notably, in this case, the Authorization Endpoint is “used by the client to obtain authorization from the resource owner via user-agent redirection.“) Because the approval is made by the user, not the Client, it's not made to the Authorization Endpoint. It could be the same URL but then you'd have to either not support “POST at the Authorization Endpoint” (which is allowed per spec), or be able to tell which kind of request this is (what you didn't do, and this PR fixed).

@brandur
Owner

@tbroyer Great points here, and thanks a lot for the detailed write-up! It's obvious that your understanding of the spec is much deeper than my own. I'm going to let @tmaher field this one. Maybe an edit to the original post is in order?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 26, 2013
  1. @brandur
  2. @brandur

    Comment on why GET is here

    brandur authored
This page is out of date. Refresh to see the latest.
View
101 lib/identity/auth.rb
@@ -98,9 +98,12 @@ class Auth < Sinatra::Base
end
namespace "/oauth" do
+ # OAuth 2 spec stipulates that the authorize endpoint MUST support GET
+ # (but that also means be very wary of CSRF):
+ #
+ # http://tools.ietf.org/html/rfc6749#section-3.1
get "/authorize" do
- # same as POST
- call(env.merge("REQUEST_METHOD" => "POST"))
+ call_authorize
end
if Identity::Config.development?
@@ -124,51 +127,7 @@ class Auth < Sinatra::Base
# successful login. If Identity's access token has expired, it is
# refreshed.
post "/authorize" do
- # if the user is submitting a confirmation form, pull from session,
- # otherwise get params from the request
- authorize_params = if params[:authorize]
- @cookie.authorize_params || {}
- else
- filter_params(%w{client_id response_type scope state}).tap do |p|
- p["scope"] = p["scope"].split(/[, ]+/).sort.uniq if p["scope"]
- end
- end
-
- # clear anything that might be left over in the session
- @cookie.authorize_params = nil
-
- begin
- # have the user login if we have no session for them
- raise Identity::Errors::NoSession if !@cookie.access_token
-
- # Try to perform an access token refresh if we know it's expired. At
- # the time of this writing, refresh tokens last 30 days (much longer
- # than the short-lived 2 hour access tokens).
- if Time.now > @cookie.access_token_expires_at
- perform_oauth_refresh_dance
- end
-
- # redirects back to the oauth client on success
- authorize(authorize_params, params[:authorize] == "Allow Access")
- # given client_id wasn't found (API throws a 400 status)
- rescue Excon::Errors::BadRequest
- flash[:error] = "Unknown OAuth client."
- redirect to("/login")
- # we couldn't track the user's session meaning that it's likely been
- # destroyed or expired, redirect to login
- rescue Excon::Errors::NotFound
- redirect to("/login")
- # refresh token dance was unsuccessful
- rescue Excon::Errors::Unauthorized, Identity::Errors::NoSession
- @cookie.authorize_params = authorize_params
- redirect to("/login")
- # client not yet authorized; show the user a confirmation dialog
- rescue Identity::Errors::UnauthorizedClient => e
- @cookie.authorize_params = authorize_params
- @client = e.client
- @scope = @cookie && @cookie.authorize_params["scope"] || nil
- slim :"clients/authorize", layout: :"layouts/zen_backdrop"
- end
+ call_authorize
end
# Exchanges a code and client_secret for a token set by proxying the
@@ -225,6 +184,54 @@ class Auth < Sinatra::Base
private
+ def call_authorize
+ # if the user is submitting a confirmation form, pull from session,
+ # otherwise get params from the request
+ authorize_params = if params[:authorize]
+ @cookie.authorize_params || {}
+ else
+ filter_params(%w{client_id response_type scope state}).tap do |p|
+ p["scope"] = p["scope"].split(/[, ]+/).sort.uniq if p["scope"]
+ end
+ end
+
+ # clear anything that might be left over in the session
+ @cookie.authorize_params = nil
+
+ begin
+ # have the user login if we have no session for them
+ raise Identity::Errors::NoSession if !@cookie.access_token
+
+ # Try to perform an access token refresh if we know it's expired. At
+ # the time of this writing, refresh tokens last 30 days (much longer
+ # than the short-lived 2 hour access tokens).
+ if Time.now > @cookie.access_token_expires_at
+ perform_oauth_refresh_dance
+ end
+
+ # redirects back to the oauth client on success
+ authorize(authorize_params, params[:authorize] == "Allow Access")
+ # given client_id wasn't found (API throws a 400 status)
+ rescue Excon::Errors::BadRequest
+ flash[:error] = "Unknown OAuth client."
+ redirect to("/login")
+ # we couldn't track the user's session meaning that it's likely been
+ # destroyed or expired, redirect to login
+ rescue Excon::Errors::NotFound
+ redirect to("/login")
+ # refresh token dance was unsuccessful
+ rescue Excon::Errors::Unauthorized, Identity::Errors::NoSession
+ @cookie.authorize_params = authorize_params
+ redirect to("/login")
+ # client not yet authorized; show the user a confirmation dialog
+ rescue Identity::Errors::UnauthorizedClient => e
+ @cookie.authorize_params = authorize_params
+ @client = e.client
+ @scope = @cookie && @cookie.authorize_params["scope"] || nil
+ slim :"clients/authorize", layout: :"layouts/zen_backdrop"
+ end
+ end
+
def filter_params(*safe_params)
safe_params.flatten!
params.dup.keep_if { |k, v| safe_params.include?(k) }
View
5 lib/identity/auth_helpers.rb
@@ -42,7 +42,10 @@ def authorize(params, confirm=false)
# if there is no authorization raise an error so that we can show a
# confirmation dialog to the user
- if !authorization && !confirm
+ #
+ # SECURITY NOTE: the confirm parameter is *only* respected if the
+ # request came in on a POST, otherwise a CSRF attack is possible
+ if !authorization && (!confirm || request.request_method != "POST")
raise Identity::Errors::UnauthorizedClient.new(client)
end
end
View
12 test/auth_test.rb
@@ -81,6 +81,18 @@ def app
"?code=454118bc-902d-4a2c-9d5b-e2a2abb91f6e",
last_response.headers["Location"]
end
+
+ it "does not create an authorization if a user confirms via GET" do
+ post "/login", email: "kerry@heroku.com", password: "abcdefgh"
+
+ # post once to get parameters stored to session
+ post "/oauth/authorize", client_id: "untrusted"
+
+ # then try again, but with the wrong request method (should be POST)
+ get "/oauth/authorize", authorize: "Allow Access"
@tmaher
tmaher added a note

No client_id param?

@brandur Owner
brandur added a note

It gets stored in session on the first request (this is how it works IRL too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ assert_equal 200, last_response.status
+ assert_match /Allow Access/, last_response.body
+ end
end
end
Something went wrong with that request. Please try again.