From bb3849e4b19822884e67006a3e7ab8175467c8ee Mon Sep 17 00:00:00 2001 From: theworldbright Date: Fri, 11 Mar 2016 17:13:31 -0800 Subject: [PATCH] Fix API privilege scope escalation --- .../authorizations_controller.rb | 6 ++-- .../api/openid_connect/authorization.rb | 11 +++++++ .../authorizations_controller_spec.rb | 31 +++++++++++++++---- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/openid_connect/authorizations_controller.rb b/app/controllers/api/openid_connect/authorizations_controller.rb index 36417e20b..92d310b10 100644 --- a/app/controllers/api/openid_connect/authorizations_controller.rb +++ b/app/controllers/api/openid_connect/authorizations_controller.rb @@ -20,7 +20,8 @@ module Api before_action :auth_user_unless_prompt_none! def new - auth = Api::OpenidConnect::Authorization.find_by_client_id_and_user(params[:client_id], current_user) + auth = Api::OpenidConnect::Authorization.find_by_client_id_user_and_scopes(params[:client_id], + current_user, params[:scope]) reset_auth(auth) if logged_in_before?(params[:max_age]) reauthenticate(params) @@ -224,7 +225,8 @@ module Api def handle_prompt_with_signed_in_user client_id = params[:client_id] if client_id - auth = Api::OpenidConnect::Authorization.find_by_client_id_and_user(client_id, current_user) + auth = Api::OpenidConnect::Authorization.find_by_client_id_user_and_scopes(client_id, + current_user, params[:scope]) if auth process_authorization_consent("true") else diff --git a/app/models/api/openid_connect/authorization.rb b/app/models/api/openid_connect/authorization.rb index f0ecef411..6c72da734 100644 --- a/app/models/api/openid_connect/authorization.rb +++ b/app/models/api/openid_connect/authorization.rb @@ -53,6 +53,17 @@ module Api id_tokens.create!(nonce: nonce) end + def self.find_by_client_id_user_and_scopes(client_id, user, scopes) + app = Api::OpenidConnect::OAuthApplication.where(client_id: client_id) + authorizations = where(o_auth_application: app, user: user).all + authorizations.each do |authorization| + if authorization.scopes.uniq.sort == Array(scopes).uniq.sort + return authorization + end + end + nil + end + def self.find_by_client_id_and_user(client_id, user) app = Api::OpenidConnect::OAuthApplication.where(client_id: client_id) find_by(o_auth_application: app, user: user) diff --git a/spec/controllers/api/openid_connect/authorizations_controller_spec.rb b/spec/controllers/api/openid_connect/authorizations_controller_spec.rb index 09b7bc626..9448317d7 100644 --- a/spec/controllers/api/openid_connect/authorizations_controller_spec.rb +++ b/spec/controllers/api/openid_connect/authorizations_controller_spec.rb @@ -4,7 +4,6 @@ describe Api::OpenidConnect::AuthorizationsController, type: :controller do let!(:client) { FactoryGirl.create(:o_auth_application) } let!(:client_with_xss) { FactoryGirl.create(:o_auth_application_with_xss) } let!(:client_with_multiple_redirects) { FactoryGirl.create(:o_auth_application_with_multiple_redirects) } - let!(:auth_with_read) { FactoryGirl.create(:auth_with_read) } before do sign_in :user, alice @@ -98,7 +97,7 @@ describe Api::OpenidConnect::AuthorizationsController, type: :controller do end context "when redirect URI does not match pre-registered URIs" do - it "should return an invalid request error", focus: true do + it "should return an invalid request error" do post :new, client_id: client.client_id, redirect_uri: "http://localhost:2000/", response_type: "id_token", scope: "openid", nonce: SecureRandom.hex(16) expect(response.body).to include("Invalid client id or redirect uri") @@ -184,10 +183,10 @@ describe Api::OpenidConnect::AuthorizationsController, type: :controller do end context "when already authorized" do - let!(:auth) { - Api::OpenidConnect::Authorization.find_or_create_by(o_auth_application: client, user: alice, - redirect_uri: "http://localhost:3000/", scopes: ["openid"]) - } + before do + Api::OpenidConnect::Authorization.create!( + o_auth_application: client, user: alice, redirect_uri: "http://localhost:3000/", scopes: ["openid"]) + end context "when valid parameters are passed" do before do @@ -231,6 +230,24 @@ describe Api::OpenidConnect::AuthorizationsController, type: :controller do expect(response.body).to match("Diaspora Test Client") end end + + context "when scopes are escalated" do + before do + get :new, client_id: client.client_id, redirect_uri: "http://localhost:3000/", response_type: "id_token", + scope: "openid read", nonce: 413_093_098_3, state: 413_093_098_3 + end + + it "should receive another authorization request" do + expect(response.body).to match("Diaspora Test Client") + end + + it "should overwrite old authorization scope after approval" do + post :create, approve: "true" + authorization_with_old_scope = + Api::OpenidConnect::Authorization.find_by_client_id_user_and_scopes(client.client_id, alice, ["openid"]) + expect(authorization_with_old_scope).to be_nil + end + end end end @@ -342,6 +359,8 @@ describe Api::OpenidConnect::AuthorizationsController, type: :controller do end describe "#destroy" do + let!(:auth_with_read) { FactoryGirl.create(:auth_with_read) } + context "with existent authorization" do before do delete :destroy, id: auth_with_read.id