From 0a5b65533dc457d9fd97b41dd9afbc19667556d9 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 12 Oct 2018 00:15:55 +0200 Subject: [PATCH] Improve signature verification safeguards (#8959) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Downcase signed_headers string before building the signed string The HTTP Signatures draft does not mandate the “headers” field to be downcased, but mandates the header field names to be downcased in the signed string, which means that prior to this patch, Mastodon could fail to process signatures from some compliant clients. It also means that it would not actually check the Digest of non-compliant clients that wouldn't use a lowercased Digest field name. Thankfully, I don't know of any such client. * Revert "Remove dead code (#8919)" This reverts commit 65d1a2d10a3bb924d41313637bdfb1aa4863c5b4. * Restore time window checking, change it to 12 hours By checking the Date header, we can prevent replaying old vulnerable signatures. The focus is to prevent replaying old vulnerable requests from software that has been fixed in the meantime, so a somewhat long window should be fine and accounts for timezone misconfiguration. * Escape users' URLs when formatting them Fixes possible HTML injection * Escape all string interpolations in Formatter class Slightly improve performance by reducing class allocations from repeated Formatter#encode calls * Fix code style issues --- .../concerns/signature_verification.rb | 18 +++++++++++++- app/lib/formatter.rb | 16 ++++++++----- .../concerns/signature_verification_spec.rb | 24 +++++++++++++++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 5f95fa3461..e5d5e2ca61 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -22,6 +22,12 @@ module SignatureVerification return end + if request.headers['Date'].present? && !matches_time_window? + @signature_verification_failure_reason = 'Signed request date outside acceptable time window' + @signed_request_account = nil + return + end + raw_signature = request.headers['Signature'] signature_params = {} @@ -76,7 +82,7 @@ module SignatureVerification def build_signed_string(signed_headers) signed_headers = 'date' if signed_headers.blank? - signed_headers.split(' ').map do |signed_header| + signed_headers.downcase.split(' ').map do |signed_header| if signed_header == Request::REQUEST_TARGET "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" elsif signed_header == 'digest' @@ -87,6 +93,16 @@ module SignatureVerification end.join("\n") end + def matches_time_window? + begin + time_sent = Time.httpdate(request.headers['Date']) + rescue ArgumentError + return false + end + + (Time.now.utc - time_sent).abs <= 12.hours + end + def body_digest "SHA-256=#{Digest::SHA256.base64digest(request_body)}" end diff --git a/app/lib/formatter.rb b/app/lib/formatter.rb index 8b694536c2..35d5a09b76 100644 --- a/app/lib/formatter.rb +++ b/app/lib/formatter.rb @@ -90,8 +90,12 @@ class Formatter private + def html_entities + @html_entities ||= HTMLEntities.new + end + def encode(html) - HTMLEntities.new.encode(html) + html_entities.encode(html) end def encode_and_link_urls(html, accounts = nil, options = {}) @@ -143,7 +147,7 @@ class Formatter emoji = emoji_map[shortcode] if emoji - replacement = "\":#{shortcode}:\"" + replacement = "\":#{encode(shortcode)}:\"" before_html = shortname_start_index.positive? ? html[0..shortname_start_index - 1] : '' html = before_html + replacement + html[i + 1..-1] i += replacement.size - (shortcode.size + 2) - 1 @@ -212,7 +216,7 @@ class Formatter return link_to_account(acct) unless linkable_accounts account = linkable_accounts.find { |item| TagManager.instance.same_acct?(item.acct, acct) } - account ? mention_html(account) : "@#{acct}" + account ? mention_html(account) : "@#{encode(acct)}" end def link_to_account(acct) @@ -221,7 +225,7 @@ class Formatter domain = nil if TagManager.instance.local_domain?(domain) account = EntityCache.instance.mention(username, domain) - account ? mention_html(account) : "@#{acct}" + account ? mention_html(account) : "@#{encode(acct)}" end def link_to_hashtag(entity) @@ -239,10 +243,10 @@ class Formatter end def hashtag_html(tag) - "##{tag}" + "##{encode(tag)}" end def mention_html(account) - "@#{account.username}" + "@#{encode(account.username)}" end end diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb index 3daf1fc4e8..7206900971 100644 --- a/spec/controllers/concerns/signature_verification_spec.rb +++ b/spec/controllers/concerns/signature_verification_spec.rb @@ -73,6 +73,30 @@ describe ApplicationController, type: :controller do end end + context 'with request older than a day' do + before do + get :success + + fake_request = Request.new(:get, request.url) + fake_request.add_headers({ 'Date' => 2.days.ago.utc.httpdate }) + 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 + end + context 'with body' do before do post :success, body: 'Hello world'