From 5d50f85599d1e2f5f881263314750dc5efbeda71 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Tue, 18 Mar 2014 15:43:32 +0000 Subject: [PATCH] fixes #4457 - Session fixation, new session IDs are not generated on login (CVE-2014-0090) --- app/controllers/application_controller.rb | 17 +++-- .../concerns/foreman/controller/authentication.rb | 29 +++++--- app/controllers/users_controller.rb | 4 +- config/routes/test.rb | 2 + .../api/base_controller_subclass_test.rb | 35 +++++++++- .../application_controller_subclass_test.rb | 80 ++++++++++++++++++++++ test/functional/locations_controller_test.rb | 2 +- test/functional/organizations_controller_test.rb | 2 +- test/functional/users_controller_test.rb | 28 ++++++++ test/lib/foreman/access_permissions_test.rb | 4 +- test/test_helper.rb | 7 +- 11 files changed, 184 insertions(+), 26 deletions(-) create mode 100644 test/functional/application_controller_subclass_test.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 70984eb..d4589d6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,7 +14,7 @@ class ApplicationController < ActionController::Base before_filter :require_ssl, :require_login before_filter :set_gettext_locale_db, :set_gettext_locale - before_filter :session_expiry, :update_activity_time, :unless => proc {|c| c.remote_user_provided? || c.api_request? } if SETTINGS[:login] + before_filter :session_expiry, :update_activity_time, :unless => proc {|c| !SETTINGS[:login] || c.remote_user_provided? || c.api_request? } before_filter :set_taxonomy, :require_mail, :check_empty_taxonomy before_filter :welcome, :only => :index, :unless => :api_request? before_filter :authorize @@ -193,17 +193,22 @@ class ApplicationController < ActionController::Base def session_expiry if session[:expires_at].blank? or (session[:expires_at].utc - Time.now.utc).to_i < 0 - # Before we expire the session, save the current taxonomies and the originally - # requested URL so they can be restored later. - save_items = session.to_hash.slice("organization_id", "location_id").merge("original_uri" => request.fullpath) - expire_session - session.merge!(save_items) + session[:original_uri] = request.fullpath + backup_session_content { expire_session } end rescue => e logger.warn "failed to determine if user sessions needs to be expired, expiring anyway: #{e}" expire_session end + # Backs up some state from a user's session around a supplied block, which + # will usually expire or reset the session in some way + def backup_session_content + save_items = session.to_hash.slice('organization_id', 'location_id', 'original_uri').symbolize_keys + yield if block_given? + session.merge!(save_items) + end + def update_activity_time session[:expires_at] = Setting[:idle_timeout].minutes.from_now.utc end diff --git a/app/controllers/concerns/foreman/controller/authentication.rb b/app/controllers/concerns/foreman/controller/authentication.rb index 34889b9..0eee7e8 100644 --- a/app/controllers/concerns/foreman/controller/authentication.rb +++ b/app/controllers/concerns/foreman/controller/authentication.rb @@ -15,9 +15,7 @@ module Foreman::Controller::Authentication if user.is_a?(User) logger.info("Authorized user #{user.login}(#{user.to_label})") - User.current = user - session[:user] = User.current.id unless api_request? - return User.current.present? + set_current_user user else if api_request? false @@ -28,14 +26,11 @@ module Foreman::Controller::Authentication (redirect_to @available_sso.login_url and return) unless @available_sso.has_rendered end - end - else - # We assume we always have a user logged in, if authentication is disabled, the user is the built-in admin account. - User.current = User.admin - session[:user] = User.current.id unless api_request? - true + # We assume we always have a user logged in + # if authentication is disabled, the user is the built-in admin account + set_current_user User.admin end end @@ -55,6 +50,8 @@ module Foreman::Controller::Authentication return false end + private + def sso_authentication if available_sso.present? if available_sso.authenticated? @@ -75,5 +72,17 @@ module Foreman::Controller::Authentication user end -end + def set_current_user(user) + User.current = user + # API access shouldn't modify the session, its authentication should be + # stateless. Other successful logins should create new session IDs. + unless api_request? + backup_session_content { reset_session } + session[:user] = user.id + update_activity_time + end + user.present? + end + +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 907000f..1df3e77 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -62,9 +62,9 @@ class UsersController < ApplicationController # Called from the login form. # Stores the user id in the session and redirects required URL or default homepage def login - session[:user] = User.current = nil - session[:locale] = nil + User.current = nil if request.post? + backup_session_content { reset_session } user = User.try_to_login(params[:login]['login'].downcase, params[:login]['password']) if user.nil? #failed to authenticate, and/or to generate the account on the fly diff --git a/config/routes/test.rb b/config/routes/test.rb index 051c4e8..c0749d7 100644 --- a/config/routes/test.rb +++ b/config/routes/test.rb @@ -1,5 +1,7 @@ if Rails.env.test? Foreman::Application.routes.draw do + resources :testable, :only => :index + namespace :api do resources :testable, :only => :index do get :raise_error, :on => :collection diff --git a/test/functional/api/base_controller_subclass_test.rb b/test/functional/api/base_controller_subclass_test.rb index c5f7467..59b6616 100644 --- a/test/functional/api/base_controller_subclass_test.rb +++ b/test/functional/api/base_controller_subclass_test.rb @@ -26,9 +26,10 @@ class Api::TestableControllerTest < ActionController::TestCase end end - context "API authentication" do + context "API usage when authentication is disabled" do setup do User.current = nil + request.env['HTTP_AUTHORIZATION'] = nil SETTINGS[:login] = false end @@ -36,7 +37,7 @@ class Api::TestableControllerTest < ActionController::TestCase SETTINGS[:login] = true end - it "does not need an username and password when Settings[:login]=false" do + it "does not need a username and password" do get :index assert_response :success end @@ -47,8 +48,36 @@ class Api::TestableControllerTest < ActionController::TestCase end end + context "API usage when authentication is enabled" do + setup do + User.current = nil + request.env['HTTP_AUTHORIZATION'] = nil + SETTINGS[:login] = true + end + + it "requires a username and password" do + @controller.stubs(:available_sso).returns(nil) + get :index + assert_response :unauthorized + end + + context "and SSO (plain) authenticates" do + setup do + @sso = mock('dummy_sso') + @sso.stubs(:authenticated?).returns(true) + @sso.stubs(:user).returns(users(:admin).login) + @controller.stubs(:available_sso).returns(@sso) + end + + it "doesn't escalate privileges in the session" do + get :index + refute session[:user], "session contains user #{session[:user]}" + end + end + end + context 'errors' do - test "top level key is error, no metadata included" do + test "top level key is error, no metadata included" do get :raise_error assert_equal ['error'], ActiveSupport::JSON.decode(@response.body).keys end diff --git a/test/functional/application_controller_subclass_test.rb b/test/functional/application_controller_subclass_test.rb new file mode 100644 index 0000000..aa093cb --- /dev/null +++ b/test/functional/application_controller_subclass_test.rb @@ -0,0 +1,80 @@ +require 'test_helper' + +class ::TestableController < ::ApplicationController + def index + render :text => 'dummy', :status => 200 + end +end + +class TestableControllerTest < ActionController::TestCase + tests ::TestableController + + context "when authentication is disabled" do + setup do + User.current = nil + SETTINGS[:login] = false + end + + teardown do + SETTINGS[:login] = true + end + + it "does not need a username and password" do + get :index + assert_response :success + end + end + + context "when authentication is enabled" do + setup do + User.current = nil + SETTINGS[:login] = true + end + + it "requires a username and password" do + get :index + assert_response :redirect + end + + it "retains original request URI in session" do + get :index + assert_equal '/testable', session[:original_uri] + end + + context "and SSO authenticates" do + setup do + @sso = mock('dummy_sso') + @sso.stubs(:authenticated?).returns(true) + @sso.stubs(:user).returns(users(:admin).login) + @controller.stubs(:available_sso).returns(@sso) + end + + it "sets the session user" do + get :index + assert_response :success + assert_equal users(:admin).id, session[:user] + end + + it "changes the session ID to prevent fixation" do + @controller.expects(:reset_session) + get :index + end + + it "doesn't escalate privileges in the old session" do + old_session = session + get :index + refute old_session.keys.include?(:user), "old session contains user" + assert session[:user], "new session doesn't contain user" + end + + it "retains taxonomy session attributes in new session" do + get :index, {}, {:location_id => taxonomies(:location1).id, + :organization_id => taxonomies(:organization1).id, + :foo => 'bar'} + assert_equal taxonomies(:location1).id, session[:location_id] + assert_equal taxonomies(:organization1).id, session[:organization_id] + refute session[:foo], "session contains 'foo', but should have been reset" + end + end + end +end diff --git a/test/functional/locations_controller_test.rb b/test/functional/locations_controller_test.rb index 0cfd69d..3362232 100644 --- a/test/functional/locations_controller_test.rb +++ b/test/functional/locations_controller_test.rb @@ -76,7 +76,7 @@ class LocationsControllerTest < ActionController::TestCase # session is reset, redirected to login, but org id remains assert_redirected_to "/users/login" assert_match /Your session has expired, please login again/, flash[:warning] - assert_equal session["location_id"], taxonomies(:location1).id + assert_equal session[:location_id], taxonomies(:location1).id end test "should display a warning if current location has been deleted" do diff --git a/test/functional/organizations_controller_test.rb b/test/functional/organizations_controller_test.rb index 6b4b2e1..01e5657 100644 --- a/test/functional/organizations_controller_test.rb +++ b/test/functional/organizations_controller_test.rb @@ -73,7 +73,7 @@ class OrganizationsControllerTest < ActionController::TestCase # session is reset, redirected to login, but org id remains assert_redirected_to "/users/login" assert_match /Your session has expired, please login again/, flash[:warning] - assert_equal session["organization_id"], taxonomies(:organization1).id + assert_equal session[:organization_id], taxonomies(:organization1).id end test "should display a warning if current organization has been deleted" do diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 06e702b..a9ac0cb 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -258,4 +258,32 @@ class UsersControllerTest < ActionController::TestCase assert_response :redirect end + test "#login sets the session user" do + post :login, {:login => {'login' => users(:admin).login, 'password' => 'secret'}} + assert_redirected_to hosts_path + assert_equal users(:admin).id, session[:user] + end + + test "#login resets the session ID to prevent fixation" do + @controller.expects(:reset_session) + post :login, {:login => {'login' => users(:admin).login, 'password' => 'secret'}} + end + + test "#login doesn't escalate privileges in the old session" do + old_session = session + post :login, {:login => {'login' => users(:admin).login, 'password' => 'secret'}} + refute old_session.keys.include?(:user), "old session contains user" + assert session[:user], "new session doesn't contain user" + end + + test "#login retains taxonomy session attributes in new session" do + post :login, {:login => {'login' => users(:admin).login, 'password' => 'secret'}}, + {:location_id => taxonomies(:location1).id, + :organization_id => taxonomies(:organization1).id, + :foo => 'bar'} + assert_equal taxonomies(:location1).id, session[:location_id] + assert_equal taxonomies(:organization1).id, session[:organization_id] + refute session[:foo], "session contains 'foo', but should have been reset" + end + end diff --git a/test/lib/foreman/access_permissions_test.rb b/test/lib/foreman/access_permissions_test.rb index fe97b5c..5823cbd 100644 --- a/test/lib/foreman/access_permissions_test.rb +++ b/test/lib/foreman/access_permissions_test.rb @@ -26,8 +26,8 @@ class AccessPermissionsTest < ActiveSupport::TestCase # Apipie "apipie/apipies/index", "apipie/apipies/apipie_checksum", - # API app controller stub - "api/testable/index", "api/testable/raise_error" + # App controller stubs + "testable/index", "api/testable/index", "api/testable/raise_error" ] MAY_SKIP_AUTHORIZED = [ "about/index" ] diff --git a/test/test_helper.rb b/test/test_helper.rb index a359c5b..e8c6d8f 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -235,7 +235,12 @@ Spork.each_run do def set_api_user return unless self.class.to_s[/api/i] - @request.env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(users(:apiadmin).login, "secret") + set_basic_auth(users(:apiadmin), "secret") + end + + def set_basic_auth(user, password) + login = user.is_a?(User) ? user.login : user + @request.env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(login, password) end end -- 1.8.5.3