references/openproject
1
0
Fork 0
mirror of https://github.com/opf/openproject.git synced 2026-06-14 03:30:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix redirect vulnerability

Browse Source 
`redirect_back_or_default` was vulnerable to some of the URLs found to
be vulnerable in redmine, such as `@test.foo`.

This commit extracts the whole functionality into a policy and alters
the constraints with a path check to avoid these cases.
Thanks to @marutosi for pointing this out.

http://www.redmine.org/projects/redmine/repository/revisions/14560
This commit is contained in:
Oliver Günther
2015-09-13 19:56:12 +02:00
parent a2d08cc06f
commit fcd450af3f
4 changed files with 292 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/controllers/application_controller.rb
+8 -50
View File
@@ -439,57 +439,15 @@ class ApplicationController < ActionController::Base
end
def redirect_back_or_default(default, escape = true, use_escaped = true)
escaped_back_url = if escape
URI.escape(CGI.unescape(params[:back_url].to_s))
else
params[:back_url]
end
policy = RedirectPolicy.new(
params[:back_url],
hostname: request.host,
default: default,
escape: escape,
return_escaped: use_escaped,
)
# if we have a back_url it must not contain two consecutive dots
if escaped_back_url.present? && !escaped_back_url.match(/\.\./)
begin
uri = URI.parse(escaped_back_url)
# do not redirect user to another host (even protocol relative urls have the host set)
# whenever a host is set it must match the request's host
uri_local_to_host = uri.host.nil? || uri.host == request.host
# do not redirect user to the login or register page
uri_path_allowed = !uri.path.match(ignored_back_url_regex)
# do not redirect to another subdirectory
uri_subdir_allowed = relative_url_root.blank? || uri.path.match(/\A#{relative_url_root}/)
if uri_local_to_host && uri_path_allowed && uri_subdir_allowed
if use_escaped
redirect_to(escaped_back_url)
else
redirect_to(back_url)
end
return
end
rescue URI::InvalidURIError
# redirect to default
end
end
redirect_to default
false
end
##
# URLs that match the returned regex must be ignored when they are the back url.
def ignored_back_url_regex
%r{/(
# Ignore login since redirect to back url is result of successful login.
login |
# When signing out with a direct login provider enabled you will be left at the logout
# page with a message indicating that you were logged out. Logging in from there would
# normally cause you to be redirected to this page. As it is the logout page, however,
# this would log you right out again after a successful login.
logout |
# TODO explain reasoning for this
account/register
)}x # ignore whitespace
redirect_to policy.redirect_url
end
def render_400(options = {})
app/policies/redirect_policy.rb
+148
View File
@@ -0,0 +1,148 @@
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See doc/COPYRIGHT.rdoc for more details.
#++
require 'uri'
require 'cgi'
# This capsulates the validation of a requested redirect URL.
#
class RedirectPolicy
attr_reader :validated_redirect_url, :request
def initialize(requested_url, hostname:, default:, escape: true, return_escaped: true)
@current_host = hostname
@return_escaped = return_escaped
@requested_url = preprocess(requested_url, escape)
@default_url = default
end
##
# Performs all validations for the requested URL
def valid?
return false if @requested_url.nil?
[
# back_url must not contain two consecutive dots
:no_upper_levels,
# Require the path to begin with a slash
:path_has_slash,
# do not redirect user to another host
:same_host,
# do not redirect user to the login or register page
:path_not_blacklisted,
# do not redirect to another subdirectory
:matches_relative_root
].all? { |check| send(check) }
end
##
# Return a valid redirect URI.
# If the validation check on the current back URL apply
def redirect_url
if valid?
postprocess(@requested_url)
else
@default_url
end
end
private
##
# Preprocesses the requested redirect URL.
# - Escapes it when necessary
# - Tries to parse it
# - Escapes the redirect URL when requested so.
def preprocess(requested, escape)
url = escape ? URI.escape(CGI.unescape(requested.to_s)) : requested
URI.parse(url)
rescue URI::InvalidURIError => e
Rails.logger.warn("Encountered invalid redirect URL '#{requested}': #{e.message}")
nil
end
##
# Postprocesses the validated URL
def postprocess(redirect_url)
# Remove basic auth credentials
redirect_url.userinfo = ''
if @return_escaped
redirect_url.to_s
else
URI.unescape(redirect_url.to_s)
end
end
##
# Avoid paths with references to parent paths
def no_upper_levels
!@requested_url.path.include? '..'
end
##
# Require URLs to contain a path slash.
# This will always be the case for parsed URLs unless
# +URI.parse('@foo.bar')+ or a non-root relative URL +URI.parse('foo')+
def path_has_slash
@requested_url.path =~ %r{\A/([^/]|\z)}
end
##
# do not redirect user to another host (even protocol relative urls have the host set)
# whenever a host is set it must match the request's host
def same_host
@requested_url.host.nil? || @requested_url.host == @current_host
end
##
# Avoid redirect URLs to specific locations, such as login page
def path_not_blacklisted
!@requested_url.path.match(
%r{/(
# Ignore login since redirect to back url is result of successful login.
login |
# When signing out with a direct login provider enabled you will be left at the logout
# page with a message indicating that you were logged out. Logging in from there would
# normally cause you to be redirected to this page. As it is the logout page, however,
# this would log you right out again after a successful login.
logout |
# TODO explain reasoning for this
account/register
)}x # ignore whitespace
)
end
##
# Requires the redirect URL to reside inside the relative root, when given.
def matches_relative_root
relative_root = OpenProject::Configuration['rails_relative_url_root']
relative_root.blank? || @requested_url.path.starts_with?(relative_root)
end
end
spec/controllers/account_controller_spec.rb
+3 -3
View File
@@ -91,12 +91,12 @@ describe AccountController, type: :controller do
context 'with a relative url root' do
before do
@old_relative_url_root = ApplicationController.relative_url_root
ApplicationController.relative_url_root = '/openproject'
@old_relative_url_root = OpenProject::Configuration['rails_relative_url_root']
OpenProject::Configuration['rails_relative_url_root'] = '/openproject'
end
after do
ApplicationController.relative_url_root = @old_relative_url_root
OpenProject::Configuration['rails_relative_url_root'] = @old_relative_url_root
end
it 'should redirect to the same subdirectory with an absolute path' do
spec/policies/redirect_policy_spec.rb
+133
View File
@@ -0,0 +1,133 @@
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See doc/COPYRIGHT.rdoc for more details.
#++
require File.expand_path('../../spec_helper', __FILE__)
describe RedirectPolicy, type: :controller do
let(:host) { 'test.host' }
let(:escape) { true }
let(:return_escaped) { true }
let(:default) { 'http://test.foo/default' }
let(:policy) {
described_class.new(
back_url,
default: default,
hostname: host,
escape: escape,
return_escaped: return_escaped
)
}
let(:subject) { policy.redirect_url }
shared_examples 'redirects to default' do |url|
let(:back_url) { url }
it "#{url} redirects to the default URL" do
expect(subject).to eq(default)
end
end
shared_examples 'valid redirect URL' do |url|
let(:back_url) { url }
it "#{url} is valid" do
expect(subject).to eq(url)
end
end
shared_examples 'valid redirect, escaped URL' do |input, output|
let(:back_url) { input }
it "#{input} is valid, but escaped to #{output}" do
expect(subject).to eq(output)
end
end
shared_examples 'ignores invalid URLs' do
uris = %w(
//test.foo/fake
//bar@test.foo
//test.foo
////test.foo
@test.foo
fake@test.foo
//foo:bar@test.foo
)
uris.each do |uri|
it_behaves_like 'redirects to default', uri
end
end
context 'with escaped URLs' do
let(:escape) { true }
it_behaves_like 'ignores invalid URLs'
it_behaves_like 'valid redirect URL', '/work_packages/1234?filter=[foo,bar]'
it_behaves_like 'valid redirect, escaped URL',
'http://test.host/?a=\11\15',
'http://test.host/?a=%5C11%5C15'
end
context 'with escaped URLs, but unescaped return URLs' do
let(:escape) { true }
let(:return_escaped) { false }
it_behaves_like 'valid redirect URL', '/work_packages/1234?filter=[foo,bar]'
it_behaves_like 'valid redirect URL', 'http://test.host/?a=\11\15'
end
context 'without escaped URLs' do
let(:escape) { false }
it_behaves_like 'ignores invalid URLs'
it_behaves_like 'valid redirect URL', '/work_packages/1234?filter=[foo,bar]'
it_behaves_like 'redirects to default', 'http://test.host/?a=\11\15'
end
context 'with relative root' do
let(:relative_root) { '/mysubdir' }
before do
allow(OpenProject::Configuration)
.to receive(:[]).with('rails_relative_url_root')
.and_return(relative_root)
end
it_behaves_like 'valid redirect URL', '/mysubdir/work_packages/1234'
it_behaves_like 'valid redirect URL', '/mysubdir'
it_behaves_like 'redirects to default', '/'
it_behaves_like 'redirects to default', '/foobar'
end
describe 'auth credentials' do
let(:back_url) { 'http://user:pass@test.host/work_packages/123' }
it 'removes the credentials' do
expect(subject).to eq('http://test.host/work_packages/123')
end
end
end