ostatus: explicitly disallow protocol downgrade from activitypub

This closes embargoed bug #1135.
This commit is contained in:
Ariadne Conill 2019-07-31 17:23:16 +00:00
parent 957de9d771
commit f72e0b7cad
6 changed files with 63 additions and 11 deletions

View File

@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
## [Unreleased] ## [Unreleased]
### Security
- OStatus: eliminate the possibility of a protocol downgrade attack.
### Changed ### Changed
- **Breaking:** Configuration: A setting to explicitly disable the mailer was added, defaulting to true, if you are using a mailer add `config :pleroma, Pleroma.Emails.Mailer, enabled: true` to your config - **Breaking:** Configuration: A setting to explicitly disable the mailer was added, defaulting to true, if you are using a mailer add `config :pleroma, Pleroma.Emails.Mailer, enabled: true` to your config
- **Breaking:** Configuration: `/media/` is now removed when `base_url` is configured, append `/media/` to your `base_url` config to keep the old behaviour if desired - **Breaking:** Configuration: `/media/` is now removed when `base_url` is configured, append `/media/` to your `base_url` config to keep the old behaviour if desired

View File

@ -9,7 +9,7 @@ defmodule Pleroma.Web.OStatus.FollowHandler do
alias Pleroma.Web.XML alias Pleroma.Web.XML
def handle(entry, doc) do def handle(entry, doc) do
with {:ok, actor} <- OStatus.find_make_or_update_user(doc), with {:ok, actor} <- OStatus.find_make_or_update_actor(doc),
id when not is_nil(id) <- XML.string_from_xpath("/entry/id", entry), id when not is_nil(id) <- XML.string_from_xpath("/entry/id", entry),
followed_uri when not is_nil(followed_uri) <- followed_uri when not is_nil(followed_uri) <-
XML.string_from_xpath("/entry/activity:object/id", entry), XML.string_from_xpath("/entry/activity:object/id", entry),

View File

@ -111,7 +111,7 @@ def handle_note(entry, doc \\ nil, options \\ []) do
with id <- XML.string_from_xpath("//id", entry), with id <- XML.string_from_xpath("//id", entry),
activity when is_nil(activity) <- Activity.get_create_by_object_ap_id_with_object(id), activity when is_nil(activity) <- Activity.get_create_by_object_ap_id_with_object(id),
[author] <- :xmerl_xpath.string('//author[1]', doc), [author] <- :xmerl_xpath.string('//author[1]', doc),
{:ok, actor} <- OStatus.find_make_or_update_user(author), {:ok, actor} <- OStatus.find_make_or_update_actor(author),
content_html <- OStatus.get_content(entry), content_html <- OStatus.get_content(entry),
cw <- OStatus.get_cw(entry), cw <- OStatus.get_cw(entry),
in_reply_to <- XML.string_from_xpath("//thr:in-reply-to[1]/@ref", entry), in_reply_to <- XML.string_from_xpath("//thr:in-reply-to[1]/@ref", entry),

View File

@ -9,7 +9,7 @@ defmodule Pleroma.Web.OStatus.UnfollowHandler do
alias Pleroma.Web.XML alias Pleroma.Web.XML
def handle(entry, doc) do def handle(entry, doc) do
with {:ok, actor} <- OStatus.find_make_or_update_user(doc), with {:ok, actor} <- OStatus.find_make_or_update_actor(doc),
id when not is_nil(id) <- XML.string_from_xpath("/entry/id", entry), id when not is_nil(id) <- XML.string_from_xpath("/entry/id", entry),
followed_uri when not is_nil(followed_uri) <- followed_uri when not is_nil(followed_uri) <-
XML.string_from_xpath("/entry/activity:object/id", entry), XML.string_from_xpath("/entry/activity:object/id", entry),

View File

@ -56,7 +56,7 @@ def remote_follow_path do
def handle_incoming(xml_string, options \\ []) do def handle_incoming(xml_string, options \\ []) do
with doc when doc != :error <- parse_document(xml_string) do with doc when doc != :error <- parse_document(xml_string) do
with {:ok, actor_user} <- find_make_or_update_user(doc), with {:ok, actor_user} <- find_make_or_update_actor(doc),
do: Pleroma.Instances.set_reachable(actor_user.ap_id) do: Pleroma.Instances.set_reachable(actor_user.ap_id)
entries = :xmerl_xpath.string('//entry', doc) entries = :xmerl_xpath.string('//entry', doc)
@ -120,7 +120,7 @@ def handle_incoming(xml_string, options \\ []) do
end end
def make_share(entry, doc, retweeted_activity) do def make_share(entry, doc, retweeted_activity) do
with {:ok, actor} <- find_make_or_update_user(doc), with {:ok, actor} <- find_make_or_update_actor(doc),
%Object{} = object <- Object.normalize(retweeted_activity), %Object{} = object <- Object.normalize(retweeted_activity),
id when not is_nil(id) <- string_from_xpath("/entry/id", entry), id when not is_nil(id) <- string_from_xpath("/entry/id", entry),
{:ok, activity, _object} = ActivityPub.announce(actor, object, id, false) do {:ok, activity, _object} = ActivityPub.announce(actor, object, id, false) do
@ -138,7 +138,7 @@ def handle_share(entry, doc) do
end end
def make_favorite(entry, doc, favorited_activity) do def make_favorite(entry, doc, favorited_activity) do
with {:ok, actor} <- find_make_or_update_user(doc), with {:ok, actor} <- find_make_or_update_actor(doc),
%Object{} = object <- Object.normalize(favorited_activity), %Object{} = object <- Object.normalize(favorited_activity),
id when not is_nil(id) <- string_from_xpath("/entry/id", entry), id when not is_nil(id) <- string_from_xpath("/entry/id", entry),
{:ok, activity, _object} = ActivityPub.like(actor, object, id, false) do {:ok, activity, _object} = ActivityPub.like(actor, object, id, false) do
@ -264,11 +264,18 @@ def maybe_update_ostatus(doc, user) do
end end
end end
def find_make_or_update_user(doc) do def find_make_or_update_actor(doc) do
uri = string_from_xpath("//author/uri[1]", doc) uri = string_from_xpath("//author/uri[1]", doc)
with {:ok, user} <- find_or_make_user(uri) do with {:ok, %User{} = user} <- find_or_make_user(uri),
{:ap_enabled, false} <- {:ap_enabled, User.ap_enabled?(user)} do
maybe_update(doc, user) maybe_update(doc, user)
else
{:ap_enabled, true} ->
{:error, :invalid_protocol}
_ ->
{:error, :unknown_user}
end end
end end

View File

@ -426,7 +426,7 @@ test "find_or_make_user sets all the nessary input fields" do
} }
end end
test "find_make_or_update_user takes an author element and returns an updated user" do test "find_make_or_update_actor takes an author element and returns an updated user" do
uri = "https://social.heldscal.la/user/23211" uri = "https://social.heldscal.la/user/23211"
{:ok, user} = OStatus.find_or_make_user(uri) {:ok, user} = OStatus.find_or_make_user(uri)
@ -439,14 +439,56 @@ test "find_make_or_update_user takes an author element and returns an updated us
doc = XML.parse_document(File.read!("test/fixtures/23211.atom")) doc = XML.parse_document(File.read!("test/fixtures/23211.atom"))
[author] = :xmerl_xpath.string('//author[1]', doc) [author] = :xmerl_xpath.string('//author[1]', doc)
{:ok, user} = OStatus.find_make_or_update_user(author) {:ok, user} = OStatus.find_make_or_update_actor(author)
assert user.avatar["type"] == "Image" assert user.avatar["type"] == "Image"
assert user.name == old_name assert user.name == old_name
assert user.bio == old_bio assert user.bio == old_bio
{:ok, user_again} = OStatus.find_make_or_update_user(author) {:ok, user_again} = OStatus.find_make_or_update_actor(author)
assert user_again == user assert user_again == user
end end
test "find_or_make_user disallows protocol downgrade" do
user = insert(:user, %{local: true})
{:ok, user} = OStatus.find_or_make_user(user.ap_id)
assert User.ap_enabled?(user)
user =
insert(:user, %{
ap_id: "https://social.heldscal.la/user/23211",
info: %{ap_enabled: true},
local: false
})
assert User.ap_enabled?(user)
{:ok, user} = OStatus.find_or_make_user(user.ap_id)
assert User.ap_enabled?(user)
end
test "find_make_or_update_actor disallows protocol downgrade" do
user = insert(:user, %{local: true})
{:ok, user} = OStatus.find_or_make_user(user.ap_id)
assert User.ap_enabled?(user)
user =
insert(:user, %{
ap_id: "https://social.heldscal.la/user/23211",
info: %{ap_enabled: true},
local: false
})
assert User.ap_enabled?(user)
{:ok, user} = OStatus.find_or_make_user(user.ap_id)
assert User.ap_enabled?(user)
doc = XML.parse_document(File.read!("test/fixtures/23211.atom"))
[author] = :xmerl_xpath.string('//author[1]', doc)
{:error, :invalid_protocol} = OStatus.find_make_or_update_actor(author)
end
end end
describe "gathering user info from a user id" do describe "gathering user info from a user id" do