Skip to content
Browse files

[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
  • Loading branch information...
1 parent 827bb82 commit 190fceaf5ccda14c93292a4a8eb1c24efd0c2939 @maxwell maxwell committed Jun 29, 2012
View
4 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'
+})
View
6 db/schema.rb
@@ -107,8 +107,6 @@
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 @@
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 @@
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 @@
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"
View
2 lib/federation_logger.rb
@@ -46,7 +46,7 @@ def initialize
end
def random
- @random ||= COLORS['FG'].keys[Random.rand(8)]
+ @random ||= COLORS['FG'].keys[3]
end
def colors?
View
7 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
View
28 lib/postzord/receiver/private.rb
@@ -62,6 +62,17 @@ def salmon
@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 @@ def xml_author
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 @@ def relayable_without_parent?
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}")
View
18 lib/postzord/receiver/public.rb
@@ -21,6 +21,8 @@ def verified_signature?
# @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 @@ def receive_relayable
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 @@ def recipient_user_ids
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?
View
21 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 @@ def legit_post_from_user1_to_user2(user1, user2)
}.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
View
6 spec/lib/postzord/receiver/public_spec.rb
@@ -16,9 +16,13 @@
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)

0 comments on commit 190fcea

Please sign in to comment.
Something went wrong with that request. Please try again.