From fcd450af3fc8d0d5b7e71d162d46c7dad9915e47 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20G=C3=BCnther?= <mail@oliverguenther.de>
Date: Sun, 13 Sep 2015 19:56:12 +0200
Subject: [PATCH] Fix redirect vulnerability

`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
---
 app/controllers/application_controller.rb   |  58 ++------
 app/policies/redirect_policy.rb             | 148 ++++++++++++++++++++
 spec/controllers/account_controller_spec.rb |   6 +-
 spec/policies/redirect_policy_spec.rb       | 133 ++++++++++++++++++
 4 files changed, 292 insertions(+), 53 deletions(-)
 create mode 100644 app/policies/redirect_policy.rb
 create mode 100644 spec/policies/redirect_policy_spec.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 47a37f217c3..889716f770f 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -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 = {})
diff --git a/app/policies/redirect_policy.rb b/app/policies/redirect_policy.rb
new file mode 100644
index 00000000000..707b575b4dd
--- /dev/null
+++ b/app/policies/redirect_policy.rb
@@ -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
diff --git a/spec/controllers/account_controller_spec.rb b/spec/controllers/account_controller_spec.rb
index dc6269e0e8e..916fc383eae 100644
--- a/spec/controllers/account_controller_spec.rb
+++ b/spec/controllers/account_controller_spec.rb
@@ -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
diff --git a/spec/policies/redirect_policy_spec.rb b/spec/policies/redirect_policy_spec.rb
new file mode 100644
index 00000000000..3c0aa9a5d7e
--- /dev/null
+++ b/spec/policies/redirect_policy_spec.rb
@@ -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