Improve signature verification safeguards (#8959)

* 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 a00ce8c92c06f42109aad5cfe65d46862cf037bb.

* 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

diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb
index 5f95fa3461ff298c19e6d9595278f8ab22145180..e5d5e2ca61914a0ab4fd56e16db16d29aae77d1f 100644 (file)
--- 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 8b694536c24886c4ab4e890ef88181fdfff1c9a0..35d5a09b766933269c74bb2a86e00b136dbea04f 100644 (file)
--- 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 = "<img draggable=\"false\" class=\"emojione\" alt=\":#{shortcode}:\" title=\":#{shortcode}:\" src=\"#{emoji}\" />"
+          replacement = "<img draggable=\"false\" class=\"emojione\" alt=\":#{encode(shortcode)}:\" title=\":#{encode(shortcode)}:\" src=\"#{encode(emoji)}\" />"
           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)
-    "<a href=\"#{tag_url(tag.downcase)}\" class=\"mention hashtag\" rel=\"tag\">#<span>#{tag}</span></a>"
+    "<a href=\"#{encode(tag_url(tag.downcase))}\" class=\"mention hashtag\" rel=\"tag\">#<span>#{encode(tag)}</span></a>"
   end
 
   def mention_html(account)
-    "<span class=\"h-card\"><a href=\"#{TagManager.instance.url_for(account)}\" class=\"u-url mention\">@<span>#{account.username}</span></a></span>"
+    "<span class=\"h-card\"><a href=\"#{encode(TagManager.instance.url_for(account))}\" class=\"u-url mention\">@<span>#{encode(account.username)}</span></a></span>"
   end
 end

diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb
index 3daf1fc4e8c1ecee99321fcc50ee356bc0041dfb..72069009716267e8b3b086f36c8c9b131705782a 100644 (file)
--- 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'