From 59e60c6db1f5fb7fd6def68a22e9ece1714e4fae Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Wed, 31 Jul 2019 17:23:16 +0000 Subject: [PATCH 1/6] ostatus: explicitly disallow protocol downgrade from activitypub This closes embargoed bug #1135. --- CHANGELOG.md | 3 ++ lib/pleroma/web/ostatus/handlers/follow_handler.ex | 2 +- lib/pleroma/web/ostatus/handlers/note_handler.ex | 2 +- .../web/ostatus/handlers/unfollow_handler.ex | 2 +- lib/pleroma/web/ostatus/ostatus.ex | 17 +++++--- test/web/ostatus/ostatus_test.exs | 48 ++++++++++++++++++++-- 6 files changed, 63 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 816f5025f..877172a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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/). ## [Unreleased] +### Security +- OStatus: eliminate the possibility of a protocol downgrade attack. + ### Fixed - `pleroma_ctl` not detecting the master branch properly. If you get "Releases are built only for master and develop branches" error when updating, please add `-` to the end of the line in `releases/start_erl.data` diff --git a/lib/pleroma/web/ostatus/handlers/follow_handler.ex b/lib/pleroma/web/ostatus/handlers/follow_handler.ex index 263d3b2dc..03e4cbbb0 100644 --- a/lib/pleroma/web/ostatus/handlers/follow_handler.ex +++ b/lib/pleroma/web/ostatus/handlers/follow_handler.ex @@ -9,7 +9,7 @@ defmodule Pleroma.Web.OStatus.FollowHandler do alias Pleroma.Web.XML 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), followed_uri when not is_nil(followed_uri) <- XML.string_from_xpath("/entry/activity:object/id", entry), diff --git a/lib/pleroma/web/ostatus/handlers/note_handler.ex b/lib/pleroma/web/ostatus/handlers/note_handler.ex index ec6e5cfaf..5b4befbb0 100644 --- a/lib/pleroma/web/ostatus/handlers/note_handler.ex +++ b/lib/pleroma/web/ostatus/handlers/note_handler.ex @@ -108,7 +108,7 @@ defmodule Pleroma.Web.OStatus.NoteHandler do with id <- XML.string_from_xpath("//id", entry), activity when is_nil(activity) <- Activity.get_create_by_object_ap_id_with_object(id), [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), cw <- OStatus.get_cw(entry), in_reply_to <- XML.string_from_xpath("//thr:in-reply-to[1]/@ref", entry), diff --git a/lib/pleroma/web/ostatus/handlers/unfollow_handler.ex b/lib/pleroma/web/ostatus/handlers/unfollow_handler.ex index 6596ada3b..2062432e3 100644 --- a/lib/pleroma/web/ostatus/handlers/unfollow_handler.ex +++ b/lib/pleroma/web/ostatus/handlers/unfollow_handler.ex @@ -9,7 +9,7 @@ defmodule Pleroma.Web.OStatus.UnfollowHandler do alias Pleroma.Web.XML 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), followed_uri when not is_nil(followed_uri) <- XML.string_from_xpath("/entry/activity:object/id", entry), diff --git a/lib/pleroma/web/ostatus/ostatus.ex b/lib/pleroma/web/ostatus/ostatus.ex index 6ed089d84..56975926f 100644 --- a/lib/pleroma/web/ostatus/ostatus.ex +++ b/lib/pleroma/web/ostatus/ostatus.ex @@ -56,7 +56,7 @@ defmodule Pleroma.Web.OStatus do def handle_incoming(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) entries = :xmerl_xpath.string('//entry', doc) @@ -118,7 +118,7 @@ defmodule Pleroma.Web.OStatus do end 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), id when not is_nil(id) <- string_from_xpath("/entry/id", entry), {:ok, activity, _object} = ActivityPub.announce(actor, object, id, false) do @@ -136,7 +136,7 @@ defmodule Pleroma.Web.OStatus do end 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), id when not is_nil(id) <- string_from_xpath("/entry/id", entry), {:ok, activity, _object} = ActivityPub.like(actor, object, id, false) do @@ -262,11 +262,18 @@ defmodule Pleroma.Web.OStatus do 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) - 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) + else + {:ap_enabled, true} -> + {:error, :invalid_protocol} + + _ -> + {:error, :unknown_user} end end diff --git a/test/web/ostatus/ostatus_test.exs b/test/web/ostatus/ostatus_test.exs index f6be16862..a74d13d02 100644 --- a/test/web/ostatus/ostatus_test.exs +++ b/test/web/ostatus/ostatus_test.exs @@ -401,7 +401,7 @@ defmodule Pleroma.Web.OStatusTest do } 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" {:ok, user} = OStatus.find_or_make_user(uri) @@ -414,14 +414,56 @@ defmodule Pleroma.Web.OStatusTest do doc = XML.parse_document(File.read!("test/fixtures/23211.atom")) [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.name == old_name 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 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 describe "gathering user info from a user id" do From 2c2c075fd6839f540cc521def52206bf560dc148 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Wed, 31 Jul 2019 22:05:12 +0300 Subject: [PATCH 2/6] Disallow following locked accounts over OStatus --- lib/pleroma/web/ostatus/handlers/follow_handler.ex | 4 ++++ test/web/ostatus/ostatus_test.exs | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/lib/pleroma/web/ostatus/handlers/follow_handler.ex b/lib/pleroma/web/ostatus/handlers/follow_handler.ex index 03e4cbbb0..24513972e 100644 --- a/lib/pleroma/web/ostatus/handlers/follow_handler.ex +++ b/lib/pleroma/web/ostatus/handlers/follow_handler.ex @@ -14,9 +14,13 @@ defmodule Pleroma.Web.OStatus.FollowHandler do followed_uri when not is_nil(followed_uri) <- XML.string_from_xpath("/entry/activity:object/id", entry), {:ok, followed} <- OStatus.find_or_make_user(followed_uri), + {:locked, false} <- {:locked, followed.info.locked}, {:ok, activity} <- ActivityPub.follow(actor, followed, id, false) do User.follow(actor, followed) {:ok, activity} + else + {:locked, true} -> + {:error, "It's not possible to follow locked accounts over OStatus"} end end end diff --git a/test/web/ostatus/ostatus_test.exs b/test/web/ostatus/ostatus_test.exs index a74d13d02..77bccdaa1 100644 --- a/test/web/ostatus/ostatus_test.exs +++ b/test/web/ostatus/ostatus_test.exs @@ -302,6 +302,14 @@ defmodule Pleroma.Web.OStatusTest do assert User.following?(follower, followed) end + test "refuse following over OStatus if the followed's account is locked" do + incoming = File.read!("test/fixtures/follow.xml") + _user = insert(:user, info: %{locked: true}, ap_id: "https://pawoo.net/users/pekorino") + + {:ok, [{:error, "It's not possible to follow locked accounts over OStatus"}]} = + OStatus.handle_incoming(incoming) + end + test "handle incoming unfollows with existing follow" do incoming_follow = File.read!("test/fixtures/follow.xml") {:ok, [_activity]} = OStatus.handle_incoming(incoming_follow) From d18a7dc0de2535236a1e1f6d8d41388fdf69322f Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Wed, 31 Jul 2019 20:11:02 +0000 Subject: [PATCH 3/6] changelog update --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 877172a61..9a8059f7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] ### Security - OStatus: eliminate the possibility of a protocol downgrade attack. +- OStatus: prevent following locked accounts, bypassing the approval process. ### Fixed - `pleroma_ctl` not detecting the master branch properly. If you get "Releases are built only for master and develop branches" error when updating, please add `-` to the end of the line in `releases/start_erl.data` From e5cb15ce2b23f0b70fd80077a72d9399267663e2 Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Mon, 29 Jul 2019 20:00:57 +0000 Subject: [PATCH 4/6] twitter api: utils: rework do_remote_follow() to use CommonAPI Closes #1138 --- lib/pleroma/web/twitter_api/controllers/util_controller.ex | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/pleroma/web/twitter_api/controllers/util_controller.ex b/lib/pleroma/web/twitter_api/controllers/util_controller.ex index b1863528f..39bc2efce 100644 --- a/lib/pleroma/web/twitter_api/controllers/util_controller.ex +++ b/lib/pleroma/web/twitter_api/controllers/util_controller.ex @@ -13,7 +13,6 @@ defmodule Pleroma.Web.TwitterAPI.UtilController do alias Pleroma.Notification alias Pleroma.User alias Pleroma.Web - alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.CommonAPI alias Pleroma.Web.OStatus alias Pleroma.Web.WebFinger @@ -98,8 +97,7 @@ defmodule Pleroma.Web.TwitterAPI.UtilController do with %User{} = user <- User.get_cached_by_nickname(username), true <- Pbkdf2.checkpw(password, user.password_hash), %User{} = _followed <- User.get_cached_by_id(id), - {:ok, follower} <- User.follow(user, followee), - {:ok, _activity} <- ActivityPub.follow(follower, followee) do + {:ok, _follower, _followee, _activity} <- CommonAPI.follow(user, followee) do conn |> render("followed.html", %{error: false}) else @@ -120,8 +118,7 @@ defmodule Pleroma.Web.TwitterAPI.UtilController do def do_remote_follow(%{assigns: %{user: user}} = conn, %{"user" => %{"id" => id}}) do with %User{} = followee <- User.get_cached_by_id(id), - {:ok, follower} <- User.follow(user, followee), - {:ok, _activity} <- ActivityPub.follow(follower, followee) do + {:ok, _follower, _followee, _activity} <- CommonAPI.follow(user, followee) do conn |> render("followed.html", %{error: false}) else From d006742fd782ba52c82b4f8aee5b3c7d80e2cb6e Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Wed, 31 Jul 2019 20:14:53 +0000 Subject: [PATCH 5/6] changelog again --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a8059f7d..a112db41c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Security - OStatus: eliminate the possibility of a protocol downgrade attack. - OStatus: prevent following locked accounts, bypassing the approval process. +- TwitterAPI: use CommonAPI to handle remote follows instead of OStatus. ### Fixed - `pleroma_ctl` not detecting the master branch properly. If you get "Releases are built only for master and develop branches" error when updating, please add `-` to the end of the line in `releases/start_erl.data` From 2536628cac71dcc525ce74e72cdfa8d8562dbbf7 Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Wed, 31 Jul 2019 20:15:45 +0000 Subject: [PATCH 6/6] mix: update version to 1.0.3 --- CHANGELOG.md | 2 +- mix.exs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a112db41c..62fc3ac85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ 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/). -## [Unreleased] +## [1.0.3] - 2019-07-31 ### Security - OStatus: eliminate the possibility of a protocol downgrade attack. - OStatus: prevent following locked accounts, bypassing the approval process. diff --git a/mix.exs b/mix.exs index a1c68051f..399314ef4 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Pleroma.Mixfile do def project do [ app: :pleroma, - version: version("1.0.2"), + version: version("1.0.3"), elixir: "~> 1.7", elixirc_paths: elixirc_paths(Mix.env()), compilers: [:phoenix, :gettext] ++ Mix.compilers(),