CVE-2020-5607: URL Redirection to Untrusted Site ('Open Redirect') (CWE-601)
value: Open redirect vulnerability in SHIRASAGI v1.13.1 and earlier allows remote attackers to redirect users to arbitrary web sites and conduct phishing attacks via unspecified vectors.
CWE description: A web application accepts a user-controlled input that specifies a link to an external site, and uses that link in a Redirect. This simplifies phishing attacks.
CVSS v3 base score: 6.1
Published: 2020-07-10T02:15Z
Repository: https://github.com/shirasagi/shirasagi
Language: Ruby
Files changed: app/controllers/concerns/member/login_filter.rb, app/controllers/member/agents/nodes/login_controller.rb, app/models/member/node.rb, spec/features/members/agents/nodes/login_spec.rb, spec/models/member/node/login_spec.rb
Commit date: 2020-07-01 13:49:01 +0900
Commit message: [fix][cms] open redirect vulnerability on member login (#3646)

* [fix] open redirect vulnerability

* [add] reproducible spec
Diff: diff --git a/app/controllers/concerns/member/login_filter.rb b/app/controllers/concerns/member/login_filter.rb
index 2952216dc..4a2283e15 100644
--- a/app/controllers/concerns/member/login_filter.rb
+++ b/app/controllers/concerns/member/login_filter.rb
@@ -66,15 +66,10 @@ module Member::LoginFilter
   def member_login_path
     return false unless member_login_node
     "#{member_login_node.url}login.html"
   end
 
-  def redirect_url
-    return "/" unless member_login_node
-    member_login_node.redirect_url || "/"
-  end
-
   def translate_redirect_option(opts)
     case opts[:redirect]
     when true
       REDIRECT_OPTION_ENABLED
     when false
diff --git a/app/controllers/member/agents/nodes/login_controller.rb b/app/controllers/member/agents/nodes/login_controller.rb
index a51005fe9..129ab6eb9 100644
--- a/app/controllers/member/agents/nodes/login_controller.rb
+++ b/app/controllers/member/agents/nodes/login_controller.rb
@@ -19,17 +19,40 @@ class Member::Agents::Nodes::LoginController < ApplicationController
       cur_member: member,
       activity_type: "login",
       remote_addr: remote_addr,
       user_agent: request.user_agent)
 
-    ref = URI::decode(params[:ref] || flash[:ref] || "")
-    ref = redirect_url if ref.blank?
+    ref = @cur_node.make_trusted_full_url(params[:ref] || flash[:ref])
+    ref = @cur_node.redirect_full_url if ref.blank?
+    ref = @cur_site.full_url if ref.blank?
     flash.discard(:ref)
 
     redirect_to ref
   end
 
+  def make_url_if_trusted(ref)
+    return if ref.blank?
+
+    ref = URI::decode(ref)
+
+    site_url = URI.parse(@cur_site.full_url)
+    full_url = URI.join(site_url, ref) rescue nil
+    return if full_url.blank?
+    return unless %w(http https).include?(full_url.scheme)
+
+    # normalize full url
+    full_url.fragment = nil
+    full_url.query = nil
+
+    # trusted?
+    return if full_url.scheme != site_url.scheme
+    return if full_url.host != site_url.host
+    return if full_url.port != site_url.port
+
+    full_url.to_s
+  end
+
   public
 
   def login
     @item = Cms::Member.new
     unless request.post?
diff --git a/app/models/member/node.rb b/app/models/member/node.rb
index 58085079e..20b9759e6 100644
--- a/app/models/member/node.rb
+++ b/app/models/member/node.rb
@@ -20,10 +20,49 @@ module Member::Node
     include Cms::Addon::Release
     include Cms::Addon::GroupPermission
     include History::Addon::Backup
 
     default_scope ->{ where(route: "member/login") }
+
+    def redirect_full_url
+      return if redirect_url.blank?
+
+      ret = make_full_url(redirect_url)
+      return if ret.blank?
+      return unless trusted?(ret)
+
+      ret.to_s
+    end
+
+    def make_trusted_full_url(ref)
+      return if ref.blank?
+
+      full_url = make_full_url(URI::decode(ref))
+      return if full_url.blank?
+
+      # normalize full url
+      full_url.fragment = nil
+      full_url.query = nil
+
+      # trusted?
+      return unless trusted?(full_url)
+
+      full_url.to_s
+    end
+
+    private
+
+    def make_full_url(path)
+      site_root_url = URI.parse(site.full_root_url)
+      URI.join(site_root_url, path) rescue nil
+    end
+
+    def trusted?(full_url)
+      return false if full_url.blank?
+
+      %w(http https).include?(full_url.scheme) && full_url.to_s.start_with?(site.full_url)
+    end
   end
 
   class Mypage
     include Cms::Model::Node
     include Cms::Addon::NodeSetting
diff --git a/spec/features/members/agents/nodes/login_spec.rb b/spec/features/members/agents/nodes/login_spec.rb
index 972d109e4..32eadc5e1 100644
--- a/spec/features/members/agents/nodes/login_spec.rb
+++ b/spec/features/members/agents/nodes/login_spec.rb
@@ -1,8 +1,8 @@
 require 'spec_helper'
 
-describe 'members/agents/nodes/login', type: :feature, dbscope: :example do
+describe 'members/agents/nodes/login', type: :feature, dbscope: :example, js: true do
   describe "ss-2724" do
     let(:root_site) { cms_site }
     let(:site1) { create(:cms_site_subdir, domains: root_site.domains) }
     let(:site2) { create(:cms_site_subdir, domains: root_site.domains) }
 
@@ -44,6 +44,31 @@ describe 'members/agents/nodes/login', type: :feature, dbscope: :example do
 
       visit site2_my_profile.full_url
       expect(page).to have_css(".form-login")
     end
   end
+
+  describe "open redirect" do
+    let(:site) { cms_site }
+    let(:layout) { create_cms_layout(cur_site: site) }
+    let(:index_page) { create :cms_page, cur_site: site, layout: layout, filename: "index.html", html: "you've been logged in" }
+    let!(:login_node) do
+      create(
+        :member_node_login, cur_site: site, layout: layout, redirect_url: index_page.url, form_auth: "enabled"
+      )
+    end
+    let!(:member) { create :cms_member, cur_site: site }
+
+    it do
+      visit "#{login_node.full_url}login.html" + "?" + { ref: URI.encode("https://www.ss-proj.org/") }.to_query
+
+      within ".form-login" do
+        fill_in "item[email]", with: member.email
+        fill_in "item[password]", with: member.in_password
+
+        click_on I18n.t("ss.login")
+      end
+
+      expect(page).to have_css(".body", text: "you've been logged in")
+    end
+  end
 end
diff --git a/spec/models/member/node/login_spec.rb b/spec/models/member/node/login_spec.rb
new file mode 100644
index 000000000..17ccd065b
--- /dev/null
+++ b/spec/models/member/node/login_spec.rb
@@ -0,0 +1,73 @@
+require 'spec_helper'
+
+describe Member::Node::Login, type: :model, dbscope: :example do
+  describe "#redirect_full_url" do
+    let(:site) { cms_site }
+
+    context "when redirect_url is missing" do
+      let(:node) { create :member_node_login, cur_site: site, redirect_url: nil }
+      it do
+        expect(node.redirect_full_url).to be_blank
+      end
+    end
+
+    context "when redirect_url is full path" do
+      let(:node) { create :member_node_login, cur_site: site, redirect_url: "/#{unique_id}" }
+      it do
+        expect(node.redirect_full_url).to eq URI.join(node.site.full_url, node.redirect_url).to_s
+      end
+    end
+
+    context "when redirect_url is full url" do
+      let(:node) { create :member_node_login, cur_site: site, redirect_url: "#{site.full_url}#{unique_id}" }
+      it do
+        expect(node.redirect_full_url).to eq URI.join(node.site.full_url, node.redirect_url).to_s
+      end
+    end
+
+    context "when redirect_url is other site url" do
+      let(:node) { create :member_node_login, cur_site: site, redirect_url: unique_url }
+      it do
+        expect(node.redirect_full_url).to be_blank
+      end
+    end
+  end
+
+  describe "#make_trusted_full_url" do
+    let(:site) { cms_site }
+    let(:node) { create :member_node_login, cur_site: site }
+
+    context "when nil is given" do
+      it do
+        expect(node.make_trusted_full_url(nil)).to be_blank
+      end
+    end
+
+    context "when empty string is given" do
+      it do
+        expect(node.make_trusted_full_url("")).to be_blank
+      end
+    end
+
+    context "when full path is given" do
+      let(:path) { "/#{unique_id}" }
+      it do
+        expect(node.make_trusted_full_url(path)).to eq URI.join(node.site.full_url, path[1..-1]).to_s
+      end
+    end
+
+    context "when full url is given" do
+      let(:url) { "#{site.full_url}#{unique_id}" }
+      it do
+        expect(node.make_trusted_full_url(url)).to eq url
+      end
+    end
+
+    context "when other site url is given" do
+      let(:url) { unique_url }
+      it do
+        expect(node.make_trusted_full_url(url)).to be_blank
+      end
+    end
+  end
+end

Vulnerable code:   def redirect_url
    return "/" unless member_login_node
    member_login_node.redirect_url || "/"
  end

    ref = URI::decode(params[:ref] || flash[:ref] || "")
    ref = redirect_url if ref.blank?
describe 'members/agents/nodes/login', type: :feature, dbscope: :example do
Fixed code:     ref = @cur_node.make_trusted_full_url(params[:ref] || flash[:ref])
    ref = @cur_node.redirect_full_url if ref.blank?
    ref = @cur_site.full_url if ref.blank?
  def make_url_if_trusted(ref)
    return if ref.blank?

    ref = URI::decode(ref)

    site_url = URI.parse(@cur_site.full_url)
    full_url = URI.join(site_url, ref) rescue nil
    return if full_url.blank?
    return unless %w(http https).include?(full_url.scheme)

    # normalize full url
    full_url.fragment = nil
    full_url.query = nil

    # trusted?
    return if full_url.scheme != site_url.scheme
    return if full_url.host != site_url.host
    return if full_url.port != site_url.port

    full_url.to_s
  end


    def redirect_full_url
      return if redirect_url.blank?

      ret = make_full_url(redirect_url)
      return if ret.blank?
      return unless trusted?(ret)

      ret.to_s
    end

    def make_trusted_full_url(ref)
      return if ref.blank?

      full_url = make_full_url(URI::decode(ref))
      return if full_url.blank?

      # normalize full url
      full_url.fragment = nil
      full_url.query = nil

      # trusted?
      return unless trusted?(full_url)

      full_url.to_s
    end

    private

    def make_full_url(path)
      site_root_url = URI.parse(site.full_root_url)
      URI.join(site_root_url, path) rescue nil
    end

    def trusted?(full_url)
      return false if full_url.blank?

      %w(http https).include?(full_url.scheme) && full_url.to_s.start_with?(site.full_url)
    end
describe 'members/agents/nodes/login', type: :feature, dbscope: :example, js: true do

  describe "open redirect" do
    let(:site) { cms_site }
    let(:layout) { create_cms_layout(cur_site: site) }
    let(:index_page) { create :cms_page, cur_site: site, layout: layout, filename: "index.html", html: "you've been logged in" }
    let!(:login_node) do
      create(
        :member_node_login, cur_site: site, layout: layout, redirect_url: index_page.url, form_auth: "enabled"
      )
    end
    let!(:member) { create :cms_member, cur_site: site }

    it do
      visit "#{login_node.full_url}login.html" + "?" + { ref: URI.encode("https://www.ss-proj.org/") }.to_query

      within ".form-login" do
        fill_in "item[email]", with: member.email
        fill_in "item[password]", with: member.in_password

        click_on I18n.t("ss.login")
      end

      expect(page).to have_css(".body", text: "you've been logged in")
    end
  end
require 'spec_helper'

describe Member::Node::Login, type: :model, dbscope: :example do
  describe "#redirect_full_url" do
    let(:site) { cms_site }

    context "when redirect_url is missing" do
      let(:node) { create :member_node_login, cur_site: site, redirect_url: nil }
      it do
        expect(node.redirect_full_url).to be_blank
      end
    end

    context "when redirect_url is full path" do
      let(:node) { create :member_node_login, cur_site: site, redirect_url: "/#{unique_id}" }
      it do
        expect(node.redirect_full_url).to eq URI.join(node.site.full_url, node.redirect_url).to_s
      end
    end

    context "when redirect_url is full url" do
      let(:node) { create :member_node_login, cur_site: site, redirect_url: "#{site.full_url}#{unique_id}" }
      it do
        expect(node.redirect_full_url).to eq URI.join(node.site.full_url, node.redirect_url).to_s
      end
    end

    context "when redirect_url is other site url" do
      let(:node) { create :member_node_login, cur_site: site, redirect_url: unique_url }
      it do
        expect(node.redirect_full_url).to be_blank
      end
    end
  end

  describe "#make_trusted_full_url" do
    let(:site) { cms_site }
    let(:node) { create :member_node_login, cur_site: site }

    context "when nil is given" do
      it do
        expect(node.make_trusted_full_url(nil)).to be_blank
      end
    end

    context "when empty string is given" do
      it do
        expect(node.make_trusted_full_url("")).to be_blank
      end
    end

    context "when full path is given" do
      let(:path) { "/#{unique_id}" }
      it do
        expect(node.make_trusted_full_url(path)).to eq URI.join(node.site.full_url, path[1..-1]).to_s
      end
    end

    context "when full url is given" do
      let(:url) { "#{site.full_url}#{unique_id}" }
      it do
        expect(node.make_trusted_full_url(url)).to eq url
      end
    end

    context "when other site url is given" do
      let(:url) { unique_url }
      it do
        expect(node.make_trusted_full_url(url)).to be_blank
      end
    end
  end
end