[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

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'
+})

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"

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?

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
 

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}")

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?

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
 
 

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)