From 190fceaf5ccda14c93292a4a8eb1c24efd0c2939 Mon Sep 17 00:00:00 2001 From: Maxwell Salzberg Date: Fri, 29 Jun 2012 22:45:11 -0700 Subject: [PATCH] [SECURITY FIX] please update your pod ASAP This is a fix for public messages, where a malicious pod could spoof a message from someone a user was connected to, as the verified signatures were not checked that the object was also from said sender. This hole only affected public messages, and the private part of code had the correct checks THX to s-f-s(Stephan Schulz) for reporting and tracking down this issue, and props to Raven24(florian.staudacher@gmx.at) for helping me test the patch --- .../javascripts/app/collections/posts.js | 4 +++ db/schema.rb | 6 +--- lib/federation_logger.rb | 2 +- lib/postzord/receiver.rb | 7 +++++ lib/postzord/receiver/private.rb | 28 ++++++++----------- lib/postzord/receiver/public.rb | 18 ++++++++++++ spec/integration/attack_vectors_spec.rb | 21 +++++++++++++- spec/lib/postzord/receiver/public_spec.rb | 6 +++- 8 files changed, 67 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/app/collections/posts.js b/app/assets/javascripts/app/collections/posts.js index 47c60b682..9805e75e8 100644 --- a/app/assets/javascripts/app/collections/posts.js +++ b/app/assets/javascripts/app/collections/posts.js @@ -2,3 +2,7 @@ app.collections.Posts = Backbone.Collection.extend({ model: app.models.Post, url : "/posts" }); + +app.collection.PublicPosts = app.collection.Posts.extend({ + url : '/public' +}) \ No newline at end of file diff --git a/db/schema.rb b/db/schema.rb index 7cb2c7b83..b71f2408e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -107,8 +107,6 @@ ActiveRecord::Schema.define(:version => 20120521191429) do t.datetime "updated_at", :null => false end - add_index "conversations", ["author_id"], :name => "conversations_author_id_fk" - create_table "invitation_codes", :force => true do |t| t.string "token" t.integer "user_id" @@ -146,7 +144,6 @@ ActiveRecord::Schema.define(:version => 20120521191429) do t.string "target_type", :limit => 60, :null => false end - add_index "likes", ["author_id"], :name => "likes_author_id_fk" add_index "likes", ["guid"], :name => "index_likes_on_guid", :unique => true add_index "likes", ["target_id", "author_id", "target_type"], :name => "index_likes_on_target_id_and_author_id_and_target_type", :unique => true add_index "likes", ["target_id"], :name => "index_likes_on_post_id" @@ -172,7 +169,6 @@ ActiveRecord::Schema.define(:version => 20120521191429) do end add_index "messages", ["author_id"], :name => "index_messages_on_author_id" - add_index "messages", ["conversation_id"], :name => "messages_conversation_id_fk" create_table "notification_actors", :force => true do |t| t.integer "notification_id" @@ -204,7 +200,7 @@ ActiveRecord::Schema.define(:version => 20120521191429) do t.text "data", :null => false end - add_index "o_embed_caches", ["url"], :name => "index_o_embed_caches_on_url", :length => {"url"=>255} + add_index "o_embed_caches", ["url"], :name => "index_o_embed_caches_on_url" create_table "participations", :force => true do |t| t.string "guid" diff --git a/lib/federation_logger.rb b/lib/federation_logger.rb index c9ac91a5f..aa6097d15 100644 --- a/lib/federation_logger.rb +++ b/lib/federation_logger.rb @@ -46,7 +46,7 @@ class Formatter end def random - @random ||= COLORS['FG'].keys[Random.rand(8)] + @random ||= COLORS['FG'].keys[3] end def colors? diff --git a/lib/postzord/receiver.rb b/lib/postzord/receiver.rb index 9d9bbee19..9b0ad6408 100644 --- a/lib/postzord/receiver.rb +++ b/lib/postzord/receiver.rb @@ -10,5 +10,12 @@ class Postzord::Receiver def perform! self.receive! end + + def author_does_not_match_xml_author? + if (@author.diaspora_handle != xml_author) + FEDERATION_LOGGER.info("event=receive status=abort reason='author in xml does not match retrieved person' payload_type=#{@object.class} sender=#{@author.diaspora_handle}") + return true + end + end end diff --git a/lib/postzord/receiver/private.rb b/lib/postzord/receiver/private.rb index be786657a..6215e0476 100644 --- a/lib/postzord/receiver/private.rb +++ b/lib/postzord/receiver/private.rb @@ -62,6 +62,17 @@ class Postzord::Receiver::Private < Postzord::Receiver @salmon ||= Salmon::EncryptedSlap.from_xml(@salmon_xml, @user) end + def validate_object + raise "Contact required unless request" if contact_required_unless_request + raise "Relayable object, but no parent object found" if relayable_without_parent? + + assign_sender_handle_if_request + + raise "Author does not match XML author" if author_does_not_match_xml_author? + + @object + end + def xml_author if @object.respond_to?(:relayable?) #if A and B are friends, and A sends B a comment from C, we delegate the validation to the owner of the post being commented on @@ -73,16 +84,6 @@ class Postzord::Receiver::Private < Postzord::Receiver xml_author end - def validate_object - raise "Contact required unless request" if contact_required_unless_request - raise "Relayable object, but no parent object found" if relayable_without_parent? - - assign_sender_handle_if_request - - raise "Author does not match XML author" if author_does_not_match_xml_author? - - @object - end def set_author! return unless @author @@ -100,13 +101,6 @@ class Postzord::Receiver::Private < Postzord::Receiver end end - def author_does_not_match_xml_author? - if (@author.diaspora_handle != xml_author) - FEDERATION_LOGGER.error("event=receive status=abort reason='author in xml does not match retrieved person' payload_type=#{@object.class} recipient=#{@user_person.diaspora_handle} sender=#{@sender.diaspora_handle}") - return true - end - end - def contact_required_unless_request unless @object.is_a?(Request) || @user.contact_for(@sender) FEDERATION_LOGGER.error("event=receive status=abort reason='sender not connected to recipient' recipient=#{@user_person.diaspora_handle} sender=#{@sender.diaspora_handle}") diff --git a/lib/postzord/receiver/public.rb b/lib/postzord/receiver/public.rb index ac92ca3cd..834f8e2ab 100644 --- a/lib/postzord/receiver/public.rb +++ b/lib/postzord/receiver/public.rb @@ -21,6 +21,8 @@ class Postzord::Receiver::Public < Postzord::Receiver # @return [void] def receive! return false unless verified_signature? + # return false unless account_deletion_is_from_author + return false unless save_object FEDERATION_LOGGER.info("received a #{@object.inspect}") @@ -50,6 +52,7 @@ class Postzord::Receiver::Public < Postzord::Receiver def save_object @object = Diaspora::Parser::from_xml(@salmon.parsed_data) raise "Object is not public" if object_can_be_public_and_it_is_not? + raise "Author does not match XML author" if author_does_not_match_xml_author? @object.save! if @object end @@ -58,8 +61,23 @@ class Postzord::Receiver::Public < Postzord::Receiver User.all_sharing_with_person(@author).select('users.id').map!{ |u| u.id } end + def xml_author + if @object.respond_to?(:relayable?) + #this is public, so it would only be owners sending us other people comments etc + @object.parent.author.local? ? @object.diaspora_handle : @object.parent.diaspora_handle + else + @object.diaspora_handle + end + end + private + def account_deletion_is_from_author + return true unless @object.is_a?(AccountDeletion) + return false if @object.diaspora_handle != @author.diaspora_handle + return true + end + # @return [Boolean] def object_can_be_public_and_it_is_not? @object.respond_to?(:public) && !@object.public? diff --git a/spec/integration/attack_vectors_spec.rb b/spec/integration/attack_vectors_spec.rb index c75129abc..110c1d500 100644 --- a/spec/integration/attack_vectors_spec.rb +++ b/spec/integration/attack_vectors_spec.rb @@ -13,6 +13,14 @@ def receive(post, opts) zord.perform! end +def receive_public(post, opts) + sender = opts.fetch(:from) + salmon_xml = Salmon::Slap.create_by_user_and_activity(sender, post.to_diaspora_xml).xml_for(nil) + post.destroy + zord = Postzord::Receiver::Public.new(salmon_xml) + zord.perform! +end + def temporary_user(&block) user = Factory(:user) block_return_value = yield user @@ -27,11 +35,14 @@ def temporary_post(user, &block) block_return_value end -def expect_error(partial_message, &block) +def expect_error(partial_message, &block)# DOES NOT REQUIRE ERROR!! begin yield rescue => e e.message.should match partial_message + + ensure + raise "no error occured where expected" unless e.present? end end @@ -120,6 +131,14 @@ describe "attack vectors" do }.should_not change(eve.profile, :first_name) end end + + + it 'public stuff should not be spoofed from another author' do + post = Factory(:status_message, :public => true, :author => eve.person) + expect_error /Author does not match XML author/ do + receive_public(post, :from => alice) + end + end end diff --git a/spec/lib/postzord/receiver/public_spec.rb b/spec/lib/postzord/receiver/public_spec.rb index 5eb84c230..d0a8bc8bd 100644 --- a/spec/lib/postzord/receiver/public_spec.rb +++ b/spec/lib/postzord/receiver/public_spec.rb @@ -16,9 +16,13 @@ describe Postzord::Receiver::Public do context 'round trips works with' do it 'a comment' do - comment = bob.build_comment(:text => 'yo', :post => Factory(:status_message)) + sm = Factory(:status_message, :author => alice.person) + + comment = bob.build_comment(:text => 'yo', :post => sm) comment.save + #bob signs his comment, and then sends it up xml = Salmon::Slap.create_by_user_and_activity(bob, comment.to_diaspora_xml).xml_for(nil) + bob.destroy comment.destroy expect{ receiver = Postzord::Receiver::Public.new(xml)