From 73219f38ce5d724b718ee5727456fd3e6e71e62a Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 2 Feb 2020 22:27:49 -0600 Subject: [PATCH] dmails: fix security issues with dmail permalinks. Fix a couple security issues related to dmail permalinks. Dmails have a permalink that you can give to a Mod to let them read the dmail. This is done with a key param that grants access when the dmail is opened by another user. The key param had several problems: * The key contained a full copy of the message's title and body encoded in base64. This meant that anyone given a dmail permalink could read the full dmail just by decoding the key in the link, without even having to open the link. * The key was derived from the dmail's title and body. If you knew or could guess a dmail's title and body you could open the dmail. One case when this was possible was when sending dmails. You could send someone a dmail, take the permalink from your sent copy of the dmail, then increment the dmail id to open the receiver's copy of the dmail. Since the sent copy and the received copy both had the same title and body, they both had the same dmail key. This let you check whether a person had read your dmail, and what time they read it at. * The key verification was done with an insecure string comparison rather than a secure constant-time comparison. This was potentially vulnerable to timing attacks. * Opening a dmail belonging to another user would mark it as read for them. The fix to all this is to use the dmail's id as the key instead of the dmail's title and body. This means that old permalinks no longer work. This is unavoidable given the issues above. Other changes: * The name of the 'Permalink' link is now 'Share'. * Anyone with the 'Share' link can view the dmail, not just Mods. --- app/controllers/dmails_controller.rb | 6 ++++- app/logical/danbooru/message_verifier.rb | 19 +++++++++++++++ app/models/dmail.rb | 28 +++++++++++++++-------- app/views/dmails/show.html.erb | 2 +- test/functional/dmails_controller_test.rb | 18 +++++++++++++++ 5 files changed, 62 insertions(+), 11 deletions(-) create mode 100644 app/logical/danbooru/message_verifier.rb diff --git a/app/controllers/dmails_controller.rb b/app/controllers/dmails_controller.rb index 32f347476..a421c7bb1 100644 --- a/app/controllers/dmails_controller.rb +++ b/app/controllers/dmails_controller.rb @@ -22,7 +22,11 @@ class DmailsController < ApplicationController def show @dmail = Dmail.find(params[:id]) check_privilege(@dmail) - @dmail.update!(is_read: true) if request.format.html? + + if request.format.html? && @dmail.owner == CurrentUser.user + @dmail.update!(is_read: true) + end + respond_with(@dmail) end diff --git a/app/logical/danbooru/message_verifier.rb b/app/logical/danbooru/message_verifier.rb new file mode 100644 index 000000000..602d9e71e --- /dev/null +++ b/app/logical/danbooru/message_verifier.rb @@ -0,0 +1,19 @@ +module Danbooru + class MessageVerifier + attr_reader :purpose, :secret, :verifier + + def initialize(purpose) + @purpose = purpose + @secret = Rails.application.key_generator.generate_key(purpose.to_s) + @verifier = ActiveSupport::MessageVerifier.new(secret, serializer: JSON, digest: "SHA256") + end + + def generate(*options) + verifier.generate(*options, purpose: purpose) + end + + def verified(*options) + verifier.verified(*options, purpose: purpose) + end + end +end diff --git a/app/models/dmail.rb b/app/models/dmail.rb index 956fc022e..eb9c81ab8 100644 --- a/app/models/dmail.rb +++ b/app/models/dmail.rb @@ -148,6 +148,25 @@ class Dmail < ApplicationRecord end end + concerning :AuthorizationMethods do + def verifier + @verifier ||= Danbooru::MessageVerifier.new(:dmail_link) + end + + def key + verifier.generate(id) + end + + def valid_key?(key) + decoded_id = verifier.verified(key) + id == decoded_id + end + + def visible_to?(user, key) + owner_id == user.id || valid_key?(key) + end + end + include AddressMethods include FactoryMethods extend SearchMethods @@ -193,15 +212,6 @@ class Dmail < ApplicationRecord end end - def key - verifier = ActiveSupport::MessageVerifier.new(Danbooru.config.email_key, serializer: JSON, digest: "SHA256") - verifier.generate("#{title} #{body}") - end - - def visible_to?(user, key) - owner_id == user.id || (user.is_moderator? && key == self.key) - end - def reportable_by?(user) is_recipient? && !is_automated? && !from.is_moderator? end diff --git a/app/views/dmails/show.html.erb b/app/views/dmails/show.html.erb index 0bd18b085..6741660d1 100644 --- a/app/views/dmails/show.html.erb +++ b/app/views/dmails/show.html.erb @@ -27,7 +27,7 @@

<%= link_to "Respond", new_dmail_path(:respond_to_id => @dmail) %> | <%= link_to "Forward", new_dmail_path(:respond_to_id => @dmail, :forward => true) %> - | <%= link_to "Permalink", dmail_path(@dmail, :key => @dmail.key), :title => "Use this URL to privately share with a moderator" %> + | <%= link_to "Share", dmail_path(@dmail, key: @dmail.key), title: "Anyone with this link will be able to view this dmail." %> <% if @dmail.is_spam? %> | <%= link_to "Not spam", dmail_path(@dmail, format: :js), remote: :true, method: :put, "data-params": "dmail[is_spam]=false" %> <% else %> diff --git a/test/functional/dmails_controller_test.rb b/test/functional/dmails_controller_test.rb index eb2942003..d3b756538 100644 --- a/test/functional/dmails_controller_test.rb +++ b/test/functional/dmails_controller_test.rb @@ -79,6 +79,16 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest assert_response(403) end + should "show dmails not owned by the current user when given a valid key" do + get_auth dmail_path(@dmail, key: @dmail.key), @unrelated_user + assert_response :success + end + + should "not show dmails not owned by the current user when given an invalid key" do + get_auth dmail_path(@dmail, key: @dmail.key + "blah"), @unrelated_user + assert_response 403 + end + should "mark dmails as read" do assert_equal(false, @dmail.is_read) get_auth dmail_path(@dmail), @dmail.owner @@ -94,6 +104,14 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_equal(false, @dmail.reload.is_read) end + + should "not mark dmails as read when viewing dmails owned by another user" do + assert_equal(false, @dmail.is_read) + get_auth dmail_path(@dmail, key: @dmail.key), @unrelated_user + + assert_response :success + assert_equal(false, @dmail.reload.is_read) + end end context "create action" do