From 69bc8f526a17b11f00c668bd9be3f82715b33f01 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 20 Sep 2018 15:33:10 +1000 Subject: [PATCH] SECURITY: only allow picking of avatars created by self (#6417) * SECURITY: only allow picking of avatars created by self Also adds origin tracking to all uploads including de-duplicated uploads --- app/controllers/users_controller.rb | 14 ++- app/models/upload.rb | 1 + app/models/user.rb | 1 + app/models/user_upload.rb | 4 + .../20180920042415_create_user_uploads.rb | 22 ++++ lib/guardian/user_guardian.rb | 17 +++ lib/upload_creator.rb | 9 +- .../components/guardian/user_guardian_spec.rb | 100 ++++++++++++++++++ spec/lib/upload_creator_spec.rb | 12 +++ spec/requests/users_controller_spec.rb | 27 ++++- 10 files changed, 196 insertions(+), 11 deletions(-) create mode 100644 app/models/user_upload.rb create mode 100644 db/migrate/20180920042415_create_user_uploads.rb create mode 100644 spec/components/guardian/user_guardian_spec.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 8268d2594d9..44df9dad04d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -865,16 +865,19 @@ class UsersController < ApplicationController end end - user.uploaded_avatar_id = upload_id + upload = Upload.find_by(id: upload_id) + + # old safeguard + user.create_user_avatar unless user.user_avatar + + guardian.ensure_can_pick_avatar!(user.user_avatar, upload) if AVATAR_TYPES_WITH_UPLOAD.include?(type) - # make sure the upload exists - unless Upload.where(id: upload_id).exists? + + if !upload return render_json_error I18n.t("avatar.missing") end - user.create_user_avatar unless user.user_avatar - if type == "gravatar" user.user_avatar.gravatar_upload_id = upload_id else @@ -882,6 +885,7 @@ class UsersController < ApplicationController end end + user.uploaded_avatar_id = upload_id user.save! user.user_avatar.save! diff --git a/app/models/upload.rb b/app/models/upload.rb index 140922862e7..60f23aeb46c 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -15,6 +15,7 @@ class Upload < ActiveRecord::Base has_many :posts, through: :post_uploads has_many :optimized_images, dependent: :destroy + has_many :user_uploads, dependent: :destroy attr_accessor :for_group_message attr_accessor :for_theme diff --git a/app/models/user.rb b/app/models/user.rb index fe0e40bf461..a7b667bd5d4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -53,6 +53,7 @@ class User < ActiveRecord::Base has_many :groups, through: :group_users has_many :secure_categories, through: :groups, source: :categories + has_many :user_uploads, dependent: :destroy has_many :user_emails, dependent: :destroy has_one :primary_email, -> { where(primary: true) }, class_name: 'UserEmail', dependent: :destroy diff --git a/app/models/user_upload.rb b/app/models/user_upload.rb new file mode 100644 index 00000000000..a72de8a0c6d --- /dev/null +++ b/app/models/user_upload.rb @@ -0,0 +1,4 @@ +class UserUpload < ActiveRecord::Base + belongs_to :upload + belongs_to :user +end diff --git a/db/migrate/20180920042415_create_user_uploads.rb b/db/migrate/20180920042415_create_user_uploads.rb new file mode 100644 index 00000000000..034bc02bf68 --- /dev/null +++ b/db/migrate/20180920042415_create_user_uploads.rb @@ -0,0 +1,22 @@ +class CreateUserUploads < ActiveRecord::Migration[5.2] + def up + create_table :user_uploads do |t| + t.integer :upload_id, null: false + t.integer :user_id, null: false + t.datetime :created_at, null: false + end + + add_index :user_uploads, [:upload_id, :user_id], unique: true + + execute <<~SQL + INSERT INTO user_uploads(upload_id, user_id, created_at) + SELECT id, user_id, COALESCE(created_at, current_timestamp) + FROM uploads + WHERE user_id IS NOT NULL + SQL + end + + def down + drop_table :user_uploads + end +end diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb index 787615b42e0..cd0bcc56426 100644 --- a/lib/guardian/user_guardian.rb +++ b/lib/guardian/user_guardian.rb @@ -1,6 +1,23 @@ # mixin for all Guardian methods dealing with user permissions module UserGuardian + def can_pick_avatar?(user_avatar, upload) + return false unless self.user + + return true if is_admin? + + # can always pick blank avatar + return true if !upload + + return true if user_avatar.contains_upload?(upload.id) + return true if upload.user_id == user_avatar.user_id || upload.user_id == user.id + + UserUpload.exists?( + upload_id: upload.id, + user_id: [upload.user_id, user.id] + ) + end + def can_edit_user?(user) is_me?(user) || is_staff? end diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index d1b1d5f6579..b457c65c09f 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -74,7 +74,10 @@ class UploadCreator end # return the previous upload if any - return @upload unless @upload.nil? + if @upload + UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id + return @upload + end fixed_original_filename = nil if is_image @@ -132,6 +135,10 @@ class UploadCreator Jobs.enqueue(:create_avatar_thumbnails, upload_id: @upload.id, user_id: user_id) end + if @upload.errors.empty? + UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id + end + @upload end ensure diff --git a/spec/components/guardian/user_guardian_spec.rb b/spec/components/guardian/user_guardian_spec.rb new file mode 100644 index 00000000000..3281b140afb --- /dev/null +++ b/spec/components/guardian/user_guardian_spec.rb @@ -0,0 +1,100 @@ +require 'rails_helper' + +describe UserGuardian do + + let :user do + Fabricate.build(:user, id: 1) + end + + let :moderator do + Fabricate.build(:moderator, id: 2) + end + + let :admin do + Fabricate.build(:admin, id: 3) + end + + let :user_avatar do + UserAvatar.new(user_id: user.id) + end + + let :users_upload do + Upload.new(user_id: user_avatar.user_id, id: 1) + end + + let :already_uploaded do + u = Upload.new(user_id: 999, id: 2) + user_avatar.custom_upload_id = u.id + u + end + + let :not_my_upload do + Upload.new(user_id: 999, id: 3) + end + + let(:moderator_upload) do + Upload.new(user_id: moderator.id, id: 4) + end + + describe '#can_pick_avatar?' do + + let :guardian do + Guardian.new(user) + end + + context 'anon user' do + let(:guardian) { Guardian.new } + + it "should return the right value" do + expect(guardian.can_pick_avatar?(user_avatar, users_upload)).to eq(false) + end + end + + context 'current user' do + it "can not set uploads not owned by current user" do + expect(guardian.can_pick_avatar?(user_avatar, users_upload)).to eq(true) + expect(guardian.can_pick_avatar?(user_avatar, already_uploaded)).to eq(true) + expect(guardian.can_pick_avatar?(user_avatar, not_my_upload)).to eq(false) + expect(guardian.can_pick_avatar?(user_avatar, nil)).to eq(true) + end + + it "can handle uploads that are associated but not directly owned" do + yes_my_upload = not_my_upload + UserUpload.create!(upload_id: yes_my_upload.id, user_id: user_avatar.user_id) + expect(guardian.can_pick_avatar?(user_avatar, yes_my_upload)).to eq(true) + + UserUpload.destroy_all + + UserUpload.create!(upload_id: yes_my_upload.id, user_id: yes_my_upload.user_id) + expect(guardian.can_pick_avatar?(user_avatar, yes_my_upload)).to eq(true) + end + end + + context 'moderator' do + + let :guardian do + Guardian.new(moderator) + end + + it "is secure" do + expect(guardian.can_pick_avatar?(user_avatar, moderator_upload)).to eq(true) + expect(guardian.can_pick_avatar?(user_avatar, users_upload)).to eq(true) + expect(guardian.can_pick_avatar?(user_avatar, already_uploaded)).to eq(true) + expect(guardian.can_pick_avatar?(user_avatar, not_my_upload)).to eq(false) + expect(guardian.can_pick_avatar?(user_avatar, nil)).to eq(true) + end + end + + context 'admin' do + let :guardian do + Guardian.new(admin) + end + + it "is secure" do + expect(guardian.can_pick_avatar?(user_avatar, not_my_upload)).to eq(true) + expect(guardian.can_pick_avatar?(user_avatar, nil)).to eq(true) + end + end + + end +end diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index 614916ec4fd..f82fa052891 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/spec/lib/upload_creator_spec.rb @@ -22,6 +22,18 @@ RSpec.describe UploadCreator do expect(upload.extension).to eq('txt') expect(File.extname(upload.url)).to eq('.txt') expect(upload.original_filename).to eq('utf-8.txt') + expect(user.user_uploads.count).to eq(1) + expect(upload.user_uploads.count).to eq(1) + + user2 = Fabricate(:user) + + expect do + UploadCreator.new(file, "utf-8\n.txt").create_for(user2.id) + end.to change { Upload.count }.by(0) + + expect(user.user_uploads.count).to eq(1) + expect(user2.user_uploads.count).to eq(1) + expect(upload.user_uploads.count).to eq(2) end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 717b8ccdd78..fe05f51ca45 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1764,9 +1764,13 @@ describe UsersController do end context 'while logged in' do + before do + sign_in(user) + end - let!(:user) { sign_in(Fabricate(:user)) } - let(:upload) { Fabricate(:upload) } + let(:upload) do + Fabricate(:upload, user: user) + end it "raises an error when you don't have permission to toggle the avatar" do another_user = Fabricate(:user) @@ -1803,6 +1807,9 @@ describe UsersController do end it 'can successfully pick a gravatar' do + + user.user_avatar.update_columns(gravatar_upload_id: upload.id) + put "/u/#{user.username}/preferences/avatar/pick.json", params: { upload_id: upload.id, type: "gravatar" } @@ -1812,6 +1819,16 @@ describe UsersController do expect(user.user_avatar.reload.gravatar_upload_id).to eq(upload.id) end + it 'can not pick uploads that were not created by user' do + upload2 = Fabricate(:upload) + + put "/u/#{user.username}/preferences/avatar/pick.json", params: { + upload_id: upload2.id, type: "custom" + } + + expect(response.status).to eq(403) + end + it 'can successfully pick a custom avatar' do put "/u/#{user.username}/preferences/avatar/pick.json", params: { upload_id: upload.id, type: "custom" @@ -2262,7 +2279,7 @@ describe UsersController do end it "raises an error when logged in" do - moderator = sign_in(Fabricate(:moderator)) + sign_in(Fabricate(:moderator)) post_user put "/u/update-activation-email.json", params: { @@ -2274,7 +2291,7 @@ describe UsersController do it "raises an error when the new email is taken" do active_user = Fabricate(:user) - user = post_user + post_user put "/u/update-activation-email.json", params: { email: active_user.email @@ -2284,7 +2301,7 @@ describe UsersController do end it "raises an error when the email is blacklisted" do - user = post_user + post_user SiteSetting.email_domains_blacklist = 'example.com' put "/u/update-activation-email.json", params: { email: 'test@example.com' } expect(response.status).to eq(422)