From b3a2c0c45b369b263ee50d3018a21e9472fe0bb1 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 22 Jan 2015 12:20:17 -0500 Subject: [PATCH] SECURITY: The SSO `return_path` was an open redirect This security fix needs SSO to be configured, and the user has to go through the entire auth process before being redirected to the wrong host so it is probably lower priority for most installs. --- app/controllers/session_controller.rb | 11 +++++++ spec/controllers/session_controller_spec.rb | 34 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 66a8c99512a..12c736d1d20 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -72,6 +72,17 @@ class SessionController < ApplicationController else log_on_user user end + + # If it's not a relative URL check the host + if return_path !~ /^\/[^\/]/ + begin + uri = URI(return_path) + return_path = "/" unless uri.host == Discourse.current_hostname + rescue + return_path = "/" + end + end + redirect_to return_path else render text: I18n.t("sso.not_found"), status: 500 diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 584e1f3e6eb..2a6dccae2a0 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -26,6 +26,8 @@ describe SessionController do @sso_url = "http://somesite.com/discourse_sso" @sso_secret = "shjkfdhsfkjh" + request.host = Discourse.current_hostname + SiteSetting.enable_sso = true SiteSetting.sso_url = @sso_url SiteSetting.sso_secret = @sso_secret @@ -79,7 +81,39 @@ describe SessionController do logged_on_user = Discourse.current_user_provider.new(request.env).current_user expect(logged_on_user.admin).to eq(true) + end + it 'redirects to a non-relative url' do + sso = get_sso("#{Discourse.base_url}/b/") + sso.external_id = '666' # the number of the beast + sso.email = 'bob@bob.com' + sso.name = 'Sam Saffron' + sso.username = 'sam' + + get :sso_login, Rack::Utils.parse_query(sso.payload) + expect(response).to redirect_to('/b/') + end + + it 'redirects to root if the host of the return_path is different' do + sso = get_sso('//eviltrout.com') + sso.external_id = '666' # the number of the beast + sso.email = 'bob@bob.com' + sso.name = 'Sam Saffron' + sso.username = 'sam' + + get :sso_login, Rack::Utils.parse_query(sso.payload) + expect(response).to redirect_to('/') + end + + it 'redirects to root if the host of the return_path is different' do + sso = get_sso('http://eviltrout.com') + sso.external_id = '666' # the number of the beast + sso.email = 'bob@bob.com' + sso.name = 'Sam Saffron' + sso.username = 'sam' + + get :sso_login, Rack::Utils.parse_query(sso.payload) + expect(response).to redirect_to('/') end it 'allows you to create an account' do