sergiotarxz/Pleroma
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Merge branch 'security/spoofing-hardening' into 'develop'

Browse Source 
security: spoofing hardening

Closes #380, #381, and #382

See merge request pleroma/pleroma!461
This commit is contained in:
lambda 2018-11-17 21:52:51 +00:00
commit d73c7cc0ca
7 changed files with 195 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
lib/pleroma/web/activity_pub/activity_pub.ex
View File

@ -628,9 +628,7 @@ def user_data_from_user_object(data) do
end
def fetch_and_prepare_user_from_ap_id(ap_id) do
with {:ok, %{status_code: 200, body: body}} <-
@httpoison.get(ap_id, [Accept: "application/activity+json"], follow_redirect: true),
{:ok, data} <- Jason.decode(body) do
with {:ok, data} <- fetch_and_contain_remote_object_from_id(ap_id) do
user_data_from_user_object(data)
else
e -> Logger.error("Could not decode user at fetch #{ap_id}, #{inspect(e)}")
@ -732,16 +730,7 @@ def fetch_object_from_id(id) do
else
Logger.info("Fetching #{id} via AP")
with true <- String.starts_with?(id, "http"),
{:ok, %{body: body, status_code: code}} when code in 200..299 <-
@httpoison.get(
id,
[Accept: "application/activity+json"],
follow_redirect: true,
timeout: 10000,
recv_timeout: 20000
),
{:ok, data} <- Jason.decode(body),
with {:ok, data} <- fetch_and_contain_remote_object_from_id(id),
nil <- Object.normalize(data),
params <- %{
"type" => "Create",
@ -771,6 +760,27 @@ def fetch_object_from_id(id) do
end
end
def fetch_and_contain_remote_object_from_id(id) do
Logger.info("Fetching #{id} via AP")
with true <- String.starts_with?(id, "http"),
{:ok, %{body: body, status_code: code}} when code in 200..299 <-
@httpoison.get(
id,
[Accept: "application/activity+json"],
follow_redirect: true,
timeout: 10000,
recv_timeout: 20000
),
{:ok, data} <- Jason.decode(body),
:ok <- Transmogrifier.contain_origin_from_id(id, data) do
{:ok, data}
else
e ->
{:error, e}
end
end
def is_public?(activity) do
"https://www.w3.org/ns/activitystreams#Public" in (activity.data["to"] ++
(activity.data["cc"] || []))

24
lib/pleroma/web/activity_pub/transmogrifier.ex
View File

@ -50,6 +50,19 @@ def contain_origin(id, %{"actor" => actor} = params) do
end
end
def contain_origin_from_id(id, %{"id" => nil}), do: :error
def contain_origin_from_id(id, %{"id" => other_id} = params) do
id_uri = URI.parse(id)
other_uri = URI.parse(other_id)
if id_uri.host == other_uri.host do
:ok
else
:error
end
end
@doc """
Modifies an incoming AP object (mastodon format) to our internal format.
"""
@ -454,15 +467,20 @@ def handle_incoming(
end
end
# TODO: Make secure.
# TODO: We presently assume that any actor on the same origin domain as the object being
# deleted has the rights to delete that object. A better way to validate whether or not
# the object should be deleted is to refetch the object URI, which should return either
# an error or a tombstone. This would allow us to verify that a deletion actually took
# place.
def handle_incoming(
%{"type" => "Delete", "object" => object_id, "actor" => actor, "id" => _id} = data
%{"type" => "Delete", "object" => object_id, "actor" => _actor, "id" => _id} = data
) do
object_id = Utils.get_ap_id(object_id)
with actor <- get_actor(data),
%User{} = _actor <- User.get_or_fetch_by_ap_id(actor),
%User{} = actor <- User.get_or_fetch_by_ap_id(actor),
{:ok, object} <- get_obj_helper(object_id) || fetch_obj_helper(object_id),
:ok <- contain_origin(actor.ap_id, object.data),
{:ok, activity} <- ActivityPub.delete(object, false) do
{:ok, activity}
else

8
lib/pleroma/web/federator/federator.ex
View File

@ -101,17 +101,23 @@ def handle(:incoming_ap_doc, params) do
params = Utils.normalize_params(params)
# NOTE: we use the actor ID to do the containment, this is fine because an
# actor shouldn't be acting on objects outside their own AP server.
with {:ok, _user} <- ap_enabled_actor(params["actor"]),
nil <- Activity.normalize(params["id"]),
{:ok, _activity} <- Transmogrifier.handle_incoming(params) do
:ok <- Transmogrifier.contain_origin_from_id(params["actor"], params),
{:ok, activity} <- Transmogrifier.handle_incoming(params) do
{:ok, activity}
else
%Activity{} ->
Logger.info("Already had #{params["id"]}")
:error
_e ->
# Just drop those for now
Logger.info("Unhandled activity")
Logger.info(Poison.encode!(params, pretty: 2))
:error
end
end

13
test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json vendored Normal file
View File

@ -0,0 +1,13 @@
{
"@context": "https://www.w3.org/ns/activitystreams",
"attributedTo": "http://mastodon.example.org/users/admin",
"attachment": [],
"content": "<p>this post was not actually written by Haelwenn</p>",
"id": "http://mastodon.example.org/users/admin/activities/1234",
"published": "2018-09-01T22:15:00Z",
"tag": [],
"to": [
"https://www.w3.org/ns/activitystreams#Public"
],
"type": "Note"
}

16
test/support/httpoison_mock.ex
View File

@ -56,6 +56,14 @@ def get("https://info.pleroma.site/activity3.json", _, _) do
}}
end
def get("https://info.pleroma.site/activity4.json", _, _) do
{:ok,
%Response{
status_code: 200,
body: File.read!("test/fixtures/httpoison_mock/https__info.pleroma.site_activity4.json")
}}
end
def get("https://info.pleroma.site/actor.json", _, _) do
{:ok,
%Response{
@ -756,6 +764,14 @@ def get("https://niu.moe/users/rye", [Accept: "application/activity+json"], _) d
}}
end
def get("https://n1u.moe/users/rye", [Accept: "application/activity+json"], _) do
{:ok,
%Response{
status_code: 200,
body: File.read!("test/fixtures/httpoison_mock/rye.json")
}}
end
def get(
"https://mst3k.interlinked.me/users/luciferMysticus",
[Accept: "application/activity+json"],

77
test/web/activity_pub/transmogrifier_test.exs
View File

@ -361,6 +361,26 @@ test "it works for incoming deletes" do
refute Repo.get(Activity, activity.id)
end
test "it fails for incoming deletes with spoofed origin" do
activity = insert(:note_activity)
data =
File.read!("test/fixtures/mastodon-delete.json")
|> Poison.decode!()
object =
data["object"]
|> Map.put("id", activity.data["object"]["id"])
data =
data
|> Map.put("object", object)
:error = Transmogrifier.handle_incoming(data)
assert Repo.get(Activity, activity.id)
end
test "it works for incoming unannounces with an existing notice" do
user = insert(:user)
{:ok, activity} = CommonAPI.post(user, %{"status" => "hey"})
@ -918,4 +938,61 @@ test "it rejects activities which reference objects that have an incorrect attri
:error = Transmogrifier.handle_incoming(data)
end
end
describe "general origin containment" do
test "contain_origin_from_id() catches obvious spoofing attempts" do
data = %{
"id" => "http://example.com/~alyssa/activities/1234.json"
}
:error =
Transmogrifier.contain_origin_from_id(
"http://example.org/~alyssa/activities/1234.json",
data
)
end
test "contain_origin_from_id() allows alternate IDs within the same origin domain" do
data = %{
"id" => "http://example.com/~alyssa/activities/1234.json"
}
:ok =
Transmogrifier.contain_origin_from_id(
"http://example.com/~alyssa/activities/1234",
data
)
end
test "contain_origin_from_id() allows matching IDs" do
data = %{
"id" => "http://example.com/~alyssa/activities/1234.json"
}
:ok =
Transmogrifier.contain_origin_from_id(
"http://example.com/~alyssa/activities/1234.json",
data
)
end
test "users cannot be collided through fake direction spoofing attempts" do
user =
insert(:user, %{
nickname: "rye@niu.moe",
local: false,
ap_id: "https://niu.moe/users/rye",
follower_address: User.ap_followers(%User{nickname: "rye@niu.moe"})
})
{:error, _} = User.get_or_fetch_by_ap_id("https://n1u.moe/users/rye")
end
test "all objects with fake directions are rejected by the object fetcher" do
{:error, _} =
ActivityPub.fetch_and_contain_remote_object_from_id(
"https://info.pleroma.site/activity4.json"
)
end
end
end

38
test/web/federator_test.exs
View File

@ -61,4 +61,42 @@ test "with relays deactivated, it does not publish to the relay", %{
Pleroma.Config.put([:instance, :allow_relay], true)
end
end
describe "Receive an activity" do
test "successfully processes incoming AP docs with correct origin" do
params = %{
"@context" => "https://www.w3.org/ns/activitystreams",
"actor" => "http://mastodon.example.org/users/admin",
"type" => "Create",
"id" => "http://mastodon.example.org/users/admin/activities/1",
"object" => %{
"type" => "Note",
"content" => "hi world!",
"id" => "http://mastodon.example.org/users/admin/objects/1",
"attributedTo" => "http://mastodon.example.org/users/admin"
},
"to" => ["https://www.w3.org/ns/activitystreams#Public"]
}
{:ok, _activity} = Federator.handle(:incoming_ap_doc, params)
end
test "rejects incoming AP docs with incorrect origin" do
params = %{
"@context" => "https://www.w3.org/ns/activitystreams",
"actor" => "https://niu.moe/users/rye",
"type" => "Create",
"id" => "http://mastodon.example.org/users/admin/activities/1",
"object" => %{
"type" => "Note",
"content" => "hi world!",
"id" => "http://mastodon.example.org/users/admin/objects/1",
"attributedTo" => "http://mastodon.example.org/users/admin"
},
"to" => ["https://www.w3.org/ns/activitystreams#Public"]
}
:error = Federator.handle(:incoming_ap_doc, params)
end
end
end