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

@@ -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!
 

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

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

app/models/user_upload.rb

@@ -0,0 +1,4 @@
+class UserUpload < ActiveRecord::Base
+  belongs_to :upload
+  belongs_to :user
+end

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

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

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

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

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
 

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)