From 68dcbcb7bf2cceb181a9b82d604d2c2829c0c78b Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 18 Jan 2023 16:47:56 +0100 Subject: [PATCH] Add more specific error messages to HTTP signature verification (#21617) * Return specific error on failure to parse Date header * Add error message when preferredUsername is not set * Change error report to be JSON and include more details * Change error report to differentiate unknown account and failed refresh * Add tests --- .../concerns/signature_verification.rb | 16 +-- .../activitypub/fetch_remote_actor_service.rb | 1 + .../concerns/signature_verification_spec.rb | 107 ++++++++++++++++-- 3 files changed, 109 insertions(+), 15 deletions(-) diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 4502da698..a9950d21f 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -46,11 +46,11 @@ module SignatureVerification end def require_account_signature! - render plain: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_account + render json: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_account end def require_actor_signature! - render plain: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_actor + render json: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_actor end def signed_request? @@ -97,11 +97,11 @@ module SignatureVerification actor = stoplight_wrap_request { actor_refresh_key!(actor) } - raise SignatureVerificationError, "Public key not found for key #{signature_params['keyId']}" if actor.nil? + raise SignatureVerificationError, "Could not refresh public key #{signature_params['keyId']}" if actor.nil? return actor unless verify_signature(actor, signature, compare_signed_string).nil? - fail_with! "Verification failed for #{actor.to_log_human_identifier} #{actor.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)" + fail_with! "Verification failed for #{actor.to_log_human_identifier} #{actor.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)", signed_string: compare_signed_string, signature: signature_params['signature'] rescue SignatureVerificationError => e fail_with! e.message rescue HTTP::Error, OpenSSL::SSL::SSLError => e @@ -118,8 +118,8 @@ module SignatureVerification private - def fail_with!(message) - @signature_verification_failure_reason = message + def fail_with!(message, **options) + @signature_verification_failure_reason = { error: message }.merge(options) @signed_request_actor = nil end @@ -209,8 +209,8 @@ module SignatureVerification end expires_time = Time.at(signature_params['expires'].to_i).utc if signature_params['expires'].present? - rescue ArgumentError - return false + rescue ArgumentError => e + raise SignatureVerificationError, "Invalid Date header: #{e.message}" end expires_time ||= created_time + 5.minutes unless created_time.nil? diff --git a/app/services/activitypub/fetch_remote_actor_service.rb b/app/services/activitypub/fetch_remote_actor_service.rb index a25fa54c4..4f60ea5e8 100644 --- a/app/services/activitypub/fetch_remote_actor_service.rb +++ b/app/services/activitypub/fetch_remote_actor_service.rb @@ -28,6 +28,7 @@ class ActivityPub::FetchRemoteActorService < BaseService raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context? raise Error, "Unexpected object type for actor #{uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_type? raise Error, "Actor #{uri} has moved to #{@json['movedTo']}" if break_on_redirect && @json['movedTo'].present? + raise Error, "Actor #{uri} has no 'preferredUsername', which is a requirement for Mastodon compatibility" unless @json['preferredUsername'].present? @uri = @json['id'] @username = @json['preferredUsername'] diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb index 6e73643b4..13655f313 100644 --- a/spec/controllers/concerns/signature_verification_spec.rb +++ b/spec/controllers/concerns/signature_verification_spec.rb @@ -16,6 +16,8 @@ describe ApplicationController, type: :controller do controller do include SignatureVerification + before_action :require_actor_signature!, only: [:signature_required] + def success head 200 end @@ -23,10 +25,17 @@ describe ApplicationController, type: :controller do def alternative_success head 200 end + + def signature_required + head 200 + end end before do - routes.draw { match via: [:get, :post], 'success' => 'anonymous#success' } + routes.draw do + match via: [:get, :post], 'success' => 'anonymous#success' + match via: [:get, :post], 'signature_required' => 'anonymous#signature_required' + end end context 'without signature header' do @@ -118,6 +127,37 @@ describe ApplicationController, type: :controller do end end + context 'with request with unparseable Date header' do + before do + get :success + + fake_request = Request.new(:get, request.url) + fake_request.add_headers({ 'Date' => 'wrong date' }) + fake_request.on_behalf_of(author) + + request.headers.merge!(fake_request.headers) + end + + describe '#signed_request?' do + it 'returns true' do + expect(controller.signed_request?).to be true + end + end + + describe '#signed_request_account' do + it 'returns nil' do + expect(controller.signed_request_account).to be_nil + end + end + + describe '#signature_verification_failure_reason' do + it 'contains an error description' do + controller.signed_request_account + expect(controller.signature_verification_failure_reason[:error]).to eq 'Invalid Date header: not RFC 2616 compliant date: "wrong date"' + end + end + end + context 'with request older than a day' do before do get :success @@ -140,6 +180,13 @@ describe ApplicationController, type: :controller do expect(controller.signed_request_account).to be_nil end end + + describe '#signature_verification_failure_reason' do + it 'contains an error description' do + controller.signed_request_account + expect(controller.signature_verification_failure_reason[:error]).to eq 'Signed request date outside acceptable time window' + end + end end context 'with inaccessible key' do @@ -171,6 +218,7 @@ describe ApplicationController, type: :controller do context 'with body' do before do + allow(controller).to receive(:actor_refresh_key!).and_return(author) post :success, body: 'Hello world' fake_request = Request.new(:post, request.url, body: 'Hello world') @@ -189,22 +237,67 @@ describe ApplicationController, type: :controller do it 'returns an account' do expect(controller.signed_request_account).to eq author end + end - it 'returns nil when path does not match' do + context 'when path does not match' do + before do request.path = '/alternative-path' - expect(controller.signed_request_account).to be_nil end - it 'returns nil when method does not match' do + describe '#signed_request_account' do + it 'returns nil' do + expect(controller.signed_request_account).to be_nil + end + end + + describe '#signature_verification_failure_reason' do + it 'contains an error description' do + controller.signed_request_account + expect(controller.signature_verification_failure_reason[:error]).to include('using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)') + expect(controller.signature_verification_failure_reason[:signed_string]).to include("(request-target): post /alternative-path\n") + end + end + end + + context 'when method does not match' do + before do get :success - expect(controller.signed_request_account).to be_nil end - it 'returns nil when body has been tampered' do + describe '#signed_request_account' do + it 'returns nil' do + expect(controller.signed_request_account).to be_nil + end + end + end + + context 'when body has been tampered' do + before do post :success, body: 'doo doo doo' - expect(controller.signed_request_account).to be_nil + end + + describe '#signed_request_account' do + it 'returns nil when body has been tampered' do + expect(controller.signed_request_account).to be_nil + end end end end end + + context 'when a signature is required' do + before do + get :signature_required + end + + context 'without signature header' do + it 'returns HTTP 401' do + expect(response).to have_http_status(401) + end + + it 'returns an error' do + expect(Oj.load(response.body)['error']).to eq 'Request not signed' + end + end + end end