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 a00ce8c92c.

* 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
This commit is contained in:
Eugen Rochko 2018-10-12 00:15:55 +02:00
parent 65662b3847
commit 1787704e1c
3 changed files with 43 additions and 9 deletions

View File

@ -22,6 +22,12 @@ module SignatureVerification
return return
end 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'] raw_signature = request.headers['Signature']
signature_params = {} signature_params = {}
@ -76,7 +82,7 @@ module SignatureVerification
def build_signed_string(signed_headers) def build_signed_string(signed_headers)
signed_headers = 'date' if signed_headers.blank? 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 if signed_header == Request::REQUEST_TARGET
"#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}"
elsif signed_header == 'digest' elsif signed_header == 'digest'
@ -89,12 +95,12 @@ module SignatureVerification
def matches_time_window? def matches_time_window?
begin begin
time_sent = DateTime.httpdate(request.headers['Date']) time_sent = Time.httpdate(request.headers['Date'])
rescue ArgumentError rescue ArgumentError
return false return false
end end
(Time.now.utc - time_sent).abs <= 30 (Time.now.utc - time_sent).abs <= 12.hours
end end
def body_digest def body_digest

View File

@ -90,8 +90,12 @@ class Formatter
private private
def html_entities
@html_entities ||= HTMLEntities.new
end
def encode(html) def encode(html)
HTMLEntities.new.encode(html) html_entities.encode(html)
end end
def encode_and_link_urls(html, accounts = nil, options = {}) def encode_and_link_urls(html, accounts = nil, options = {})
@ -143,7 +147,7 @@ class Formatter
emoji = emoji_map[shortcode] emoji = emoji_map[shortcode]
if emoji 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] : '' before_html = shortname_start_index.positive? ? html[0..shortname_start_index - 1] : ''
html = before_html + replacement + html[i + 1..-1] html = before_html + replacement + html[i + 1..-1]
i += replacement.size - (shortcode.size + 2) - 1 i += replacement.size - (shortcode.size + 2) - 1
@ -212,7 +216,7 @@ class Formatter
return link_to_account(acct) unless linkable_accounts return link_to_account(acct) unless linkable_accounts
account = linkable_accounts.find { |item| TagManager.instance.same_acct?(item.acct, acct) } account = linkable_accounts.find { |item| TagManager.instance.same_acct?(item.acct, acct) }
account ? mention_html(account) : "@#{acct}" account ? mention_html(account) : "@#{encode(acct)}"
end end
def link_to_account(acct) def link_to_account(acct)
@ -221,7 +225,7 @@ class Formatter
domain = nil if TagManager.instance.local_domain?(domain) domain = nil if TagManager.instance.local_domain?(domain)
account = EntityCache.instance.mention(username, domain) account = EntityCache.instance.mention(username, domain)
account ? mention_html(account) : "@#{acct}" account ? mention_html(account) : "@#{encode(acct)}"
end end
def link_to_hashtag(entity) def link_to_hashtag(entity)
@ -239,10 +243,10 @@ class Formatter
end end
def hashtag_html(tag) 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 end
def mention_html(account) 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
end end

View File

@ -73,6 +73,30 @@ describe ApplicationController, type: :controller do
end end
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 context 'with body' do
before do before do
post :success, body: 'Hello world' post :success, body: 'Hello world'