From 85850cc74d6d886dd6faa761546057ad80f35c26 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sat, 2 Dec 2017 17:52:36 +0000 Subject: [PATCH] Harden bulk signature actions against attack Use a per-session secret key to encrypt and sign a list of ids presented to the moderator and ensure that when the bulk action is requested the selected set of ids is contained within the signed list of ids and that the list of ids hasn't been tampered with. This provides an additional layer of security should any vulnerabilities be discovered within the authlogic gem. Also limit the number of selected ids accepted by the bulk action to a maximum of fifty. However, this is somewhat redundant given the fact that there will be a maximum of fifty ids in the signed list of ids. --- .../admin/signatures_controller.rb | 12 +-- app/controllers/concerns/bulk_verification.rb | 49 ++++++++++ .../admin/signatures/_signature.html.erb | 2 +- app/views/admin/signatures/index.html.erb | 15 ++- config/application.rb | 5 +- .../admin/signatures_controller_spec.rb | 93 +++++++++++++++++-- 6 files changed, 154 insertions(+), 22 deletions(-) create mode 100644 app/controllers/concerns/bulk_verification.rb diff --git a/app/controllers/admin/signatures_controller.rb b/app/controllers/admin/signatures_controller.rb index 1441f9388..79df98b25 100644 --- a/app/controllers/admin/signatures_controller.rb +++ b/app/controllers/admin/signatures_controller.rb @@ -1,4 +1,6 @@ class Admin::SignaturesController < Admin::AdminController + include BulkVerification + before_action :fetch_signature, except: [:index, :bulk_validate, :bulk_invalidate, :bulk_destroy] before_action :fetch_signatures, only: [:index] @@ -10,7 +12,7 @@ def index def bulk_validate begin - Signature.validate!(signature_ids) + Signature.validate!(selected_ids) redirect_to admin_signatures_url(q: params[:q]), notice: :signatures_validated rescue StandardError => e Appsignal.send_exception e @@ -30,7 +32,7 @@ def validate def bulk_invalidate begin - Signature.invalidate!(signature_ids) + Signature.invalidate!(selected_ids) redirect_to admin_signatures_url(q: params[:q]), notice: :signatures_invalidated rescue StandardError => e Appsignal.send_exception e @@ -50,7 +52,7 @@ def invalidate def bulk_destroy begin - Signature.destroy!(signature_ids) + Signature.destroy!(selected_ids) redirect_to admin_signatures_url(q: params[:q]), notice: :signatures_deleted rescue StandardError => e Appsignal.send_exception e @@ -75,8 +77,4 @@ def fetch_signatures def fetch_signature @signature = Signature.find(params[:id]) end - - def signature_ids - params[:ids].to_s.split(",").map(&:to_i).reject(&:zero?) - end end diff --git a/app/controllers/concerns/bulk_verification.rb b/app/controllers/concerns/bulk_verification.rb new file mode 100644 index 000000000..0d812efc3 --- /dev/null +++ b/app/controllers/concerns/bulk_verification.rb @@ -0,0 +1,49 @@ +module BulkVerification + extend ActiveSupport::Concern + + class InvalidBulkRequest < RuntimeError; end + + included do + before_action :verify_bulk_request, if: :bulk_request? + + helper_method :bulk_verifier + + rescue_from ActiveSupport::MessageVerifier::InvalidSignature do + raise BulkVerification::InvalidBulkRequest, "Invalid bulk request for #{selected_ids.inspect}" + end + end + + private + + def bulk_request? + action_name =~ /\Abulk_/ + end + + def bulk_verification_token + session[:_bulk_verification_token] ||= SecureRandom.base64(32) + end + + def bulk_verifier + @_bulk_verifer ||= ActiveSupport::MessageVerifier.new(bulk_verification_token, serializer: JSON) + end + + def selected_ids + @_selected_ids ||= params[:selected_ids].to_s.split(",").map(&:to_i).reject(&:zero?).take(50) + end + + def all_ids + @_all_ids ||= bulk_verifier.verify(params[:all_ids]) + end + + def verify_bulk_request + selected_ids.all?(&method(:verify_bulk_request_id)) + end + + def verify_bulk_request_id(id) + all_ids.include?(id) || raise_bad_request(id) + end + + def raise_bad_request(id) + raise BulkVerification::InvalidBulkRequest, "Invalid bulk request - #{id} not present in #{all_ids.inspect}" + end +end diff --git a/app/views/admin/signatures/_signature.html.erb b/app/views/admin/signatures/_signature.html.erb index 1ef2a2288..914ffe873 100644 --- a/app/views/admin/signatures/_signature.html.erb +++ b/app/views/admin/signatures/_signature.html.erb @@ -1,7 +1,7 @@ <%= link_to signature.petition.action, admin_petition_path(signature.petition) %> diff --git a/app/views/admin/signatures/index.html.erb b/app/views/admin/signatures/index.html.erb index 250186322..d519e344f 100644 --- a/app/views/admin/signatures/index.html.erb +++ b/app/views/admin/signatures/index.html.erb @@ -37,18 +37,23 @@ <%= will_paginate @signatures %>
+ <% signature_ids = bulk_verifier.generate(@signatures.map(&:id)) %> + <%= form_tag validate_admin_signatures_path(q: params[:q]), method: :post, class: 'action validate-action' do %> - <%= hidden_field_tag 'ids', '' %> + <%= hidden_field_tag 'selected_ids', '' %> + <%= hidden_field_tag 'all_ids', signature_ids %> <%= submit_tag 'Validate', name: nil, class: 'button', disabled: true, data: { confirm: 'Validate selected signatures?' } %> <% end %> <%= form_tag invalidate_admin_signatures_path(q: params[:q]), method: :post, class: 'action invalidate-action' do %> - <%= hidden_field_tag 'ids', '' %> + <%= hidden_field_tag 'selected_ids', '' %> + <%= hidden_field_tag 'all_ids', signature_ids %> <%= submit_tag 'Invalidate', name: nil, class: 'button', disabled: true, data: { confirm: 'Invalidate selected signatures?' } %> <% end %> <%= form_tag admin_signatures_path(q: params[:q]), method: :delete, class: 'action delete-action' do %> - <%= hidden_field_tag 'ids', '' %> + <%= hidden_field_tag 'selected_ids', '' %> + <%= hidden_field_tag 'all_ids', signature_ids %> <%= submit_tag 'Delete', name: nil, class: 'button-warning', disabled: true, data: { confirm: 'Delete selected signatures?' } %> <% end %>
@@ -56,7 +61,7 @@ diff --git a/config/application.rb b/config/application.rb index 639373be2..7e008b902 100644 --- a/config/application.rb +++ b/config/application.rb @@ -38,9 +38,10 @@ class Application < Rails::Application # Remove the error wrapper from around the form element config.action_view.field_error_proc = -> (html_tag, instance) { html_tag } - # Add 503 Service Unavailable to the rescue response + # Add additional exceptions to the rescue responses config.action_dispatch.rescue_responses.merge!( - 'Site::ServiceUnavailable' => :service_unavailable + 'Site::ServiceUnavailable' => :service_unavailable, + 'BulkVerification::InvalidBulkRequest' => :bad_request ) config.action_dispatch.default_headers.merge!('X-UA-Compatible' => 'IE=edge') diff --git a/spec/controllers/admin/signatures_controller_spec.rb b/spec/controllers/admin/signatures_controller_spec.rb index e7e001e8a..4cc9b4797 100644 --- a/spec/controllers/admin/signatures_controller_spec.rb +++ b/spec/controllers/admin/signatures_controller_spec.rb @@ -41,7 +41,14 @@ context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } - before { login_as(user) } + let(:token) { SecureRandom.base64(32) } + let(:verifier) { ActiveSupport::MessageVerifier.new(token, serializer: JSON) } + let(:signature_ids) { verifier.generate([signature.id]) } + + before do + login_as(user) + session[:_bulk_verification_token] = token + end describe "GET /admin/signatures" do before { get :index, q: "Alice" } @@ -168,7 +175,7 @@ before do expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:validate!).and_return(true) - post :bulk_validate, ids: signature.id, q: "user@example.com" + post :bulk_validate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -187,7 +194,7 @@ expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:validate!).and_raise(exception) expect(Appsignal).to receive(:send_exception).with(exception) - post :bulk_validate, ids: signature.id, q: "user@example.com" + post :bulk_validate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -198,6 +205,30 @@ expect(flash[:alert]).to eq("Signatures could not be validated - please contact support") end end + + context "when the signature ids hmac is missing" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_validate, selected_ids: signature.id, all_ids: "", q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request for \[\d+\]/) + end + end + + context "when the selected_ids param contains an invalid id" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_validate, selected_ids: "1,2", all_ids: signature_ids, q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/) + end + end end describe "POST /admin/signatures/invalidate" do @@ -205,7 +236,7 @@ before do expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:invalidate!).and_return(true) - post :bulk_invalidate, ids: signature.id, q: "user@example.com" + post :bulk_invalidate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -224,7 +255,7 @@ expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:invalidate!).and_raise(exception) expect(Appsignal).to receive(:send_exception).with(exception) - post :bulk_invalidate, ids: signature.id, q: "user@example.com" + post :bulk_invalidate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -235,6 +266,30 @@ expect(flash[:alert]).to eq("Signatures could not be invalidated - please contact support") end end + + context "when the signature ids hmac is missing" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_invalidate, selected_ids: signature.id, all_ids: "", q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request for \[\d+\]/) + end + end + + context "when the selected_ids param contains an invalid id" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_invalidate, selected_ids: "1,2", all_ids: signature_ids, q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/) + end + end end describe "DELETE /admin/signatures" do @@ -242,7 +297,7 @@ before do expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:destroy!).and_return(true) - delete :bulk_destroy, ids: signature.id, q: "user@example.com" + delete :bulk_destroy, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -261,7 +316,7 @@ expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:destroy!).and_raise(exception) expect(Appsignal).to receive(:send_exception).with(exception) - delete :bulk_destroy, ids: signature.id, q: "user@example.com" + delete :bulk_destroy, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -272,6 +327,30 @@ expect(flash[:alert]).to eq("Signatures could not be removed - please contact support") end end + + context "when the signature ids hmac is missing" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_destroy, selected_ids: signature.id, all_ids: "", q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request for \[\d+\]/) + end + end + + context "when the selected_ids param contains an invalid id" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_destroy, selected_ids: "1,2", all_ids: signature_ids, q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/) + end + end end end end