From 040a02c9d4b5dd2f91c5c29c0008a47cde6ee99a Mon Sep 17 00:00:00 2001 From: NAKANO Hideo Date: Wed, 1 Jul 2020 13:49:01 +0900 Subject: [PATCH] [fix][cms] open redirect vulnerability on member login (#3646) * [fix] open redirect vulnerability * [add] reproducible spec --- .../concerns/member/login_filter.rb | 5 -- .../member/agents/nodes/login_controller.rb | 27 ++++++- app/models/member/node.rb | 39 ++++++++++ .../members/agents/nodes/login_spec.rb | 27 ++++++- spec/models/member/node/login_spec.rb | 73 +++++++++++++++++++ 5 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 spec/models/member/node/login_spec.rb diff --git a/app/controllers/concerns/member/login_filter.rb b/app/controllers/concerns/member/login_filter.rb index 2952216dc5e..4a2283e15e6 100644 --- a/app/controllers/concerns/member/login_filter.rb +++ b/app/controllers/concerns/member/login_filter.rb @@ -68,11 +68,6 @@ def member_login_path "#{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 diff --git a/app/controllers/member/agents/nodes/login_controller.rb b/app/controllers/member/agents/nodes/login_controller.rb index a51005fe959..129ab6eb957 100644 --- a/app/controllers/member/agents/nodes/login_controller.rb +++ b/app/controllers/member/agents/nodes/login_controller.rb @@ -21,13 +21,36 @@ def set_member_and_redirect(member) 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 diff --git a/app/models/member/node.rb b/app/models/member/node.rb index 58085079ebd..20b9759e6e7 100644 --- a/app/models/member/node.rb +++ b/app/models/member/node.rb @@ -22,6 +22,45 @@ class Login 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 diff --git a/spec/features/members/agents/nodes/login_spec.rb b/spec/features/members/agents/nodes/login_spec.rb index 972d109e4b7..32eadc5e178 100644 --- a/spec/features/members/agents/nodes/login_spec.rb +++ b/spec/features/members/agents/nodes/login_spec.rb @@ -1,6 +1,6 @@ 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) } @@ -46,4 +46,29 @@ 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 00000000000..17ccd065b9d --- /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