diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex index a312f69e8..37bc20e4d 100644 --- a/lib/pleroma/object/containment.ex +++ b/lib/pleroma/object/containment.ex @@ -11,6 +11,9 @@ defmodule Pleroma.Object.Containment do Object containment is an important step in validating remote objects to prevent spoofing, therefore removal of object containment functions is NOT recommended. """ + + alias Pleroma.Web.ActivityPub.Transmogrifier + def get_actor(%{"actor" => actor}) when is_binary(actor) do actor end @@ -47,6 +50,18 @@ defmodule Pleroma.Object.Containment do defp compare_uris(%URI{host: host} = _id_uri, %URI{host: host} = _other_uri), do: :ok defp compare_uris(_id_uri, _other_uri), do: :error + defp compare_uris_exact(uri, uri), do: :ok + + defp compare_uris_exact(%URI{} = id, %URI{} = other), + do: compare_uris_exact(URI.to_string(id), URI.to_string(other)) + + defp compare_uris_exact(id_uri, other_uri) + when is_binary(id_uri) and is_binary(other_uri) do + norm_id = String.replace_suffix(id_uri, "/", "") + norm_other = String.replace_suffix(other_uri, "/", "") + if norm_id == norm_other, do: :ok, else: :error + end + @doc """ Checks whether an URL to fetch from is from the local server. @@ -77,6 +92,26 @@ defmodule Pleroma.Object.Containment do def contain_origin(_id, _data), do: :ok + @doc """ + Check whether the fetch URL (after redirects) exactly (sans tralining slash) matches either + the canonical ActivityPub id or the objects url field (for display URLs from *key and Mastodon) + + Since this is meant to be used for fetches, anonymous or transient objects are not accepted here. + """ + def contain_id_to_fetch(url, %{"id" => id} = data) when is_binary(id) do + with {:id, :error} <- {:id, compare_uris_exact(id, url)}, + # "url" can be a "Link" object and this is checked before full normalisation + display_url <- Transmogrifier.fix_url(data)["url"], + true <- display_url != nil do + compare_uris_exact(display_url, url) + else + {:id, :ok} -> :ok + _ -> :error + end + end + + def contain_id_to_fetch(_url, _data), do: :error + @doc """ Check whether the object id is from the same host as another id """ diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index e51262de4..618fb278e 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -263,7 +263,7 @@ defmodule Pleroma.Object.Fetcher do {_, :ok} <- {:local_fetch, Containment.contain_local_fetch(id)}, {:ok, final_id, body} <- get_object(id), {:ok, data} <- safe_json_decode(body), - {_, :ok} <- {:containment, Containment.contain_origin_from_id(final_id, data)}, + {_, :ok} <- {:strict_id, Containment.contain_id_to_fetch(final_id, data)}, {_, :ok} <- {:containment, Containment.contain_origin(final_id, data)} do unless Instances.reachable?(final_id) do Instances.set_reachable(final_id) @@ -271,6 +271,9 @@ defmodule Pleroma.Object.Fetcher do {:ok, data} else + {:strict_id, _} -> + {:error, "Object's ActivityPub id/url does not match final fetch URL"} + {:scheme, _} -> {:error, "Unsupported URI scheme"} diff --git a/test/fixtures/tesla_mock/https__info.pleroma.site_activity3.json b/test/fixtures/tesla_mock/https__info.pleroma.site_activity3.json index 1df73f2c5..dbf74dfe1 100644 --- a/test/fixtures/tesla_mock/https__info.pleroma.site_activity3.json +++ b/test/fixtures/tesla_mock/https__info.pleroma.site_activity3.json @@ -3,7 +3,7 @@ "attributedTo": "http://mastodon.example.org/users/admin", "attachment": [], "content": "

this post was not actually written by Haelwenn

", - "id": "https://info.pleroma.site/activity2.json", + "id": "https://info.pleroma.site/activity3.json", "published": "2018-09-01T22:15:00Z", "tag": [], "to": [ diff --git a/test/fixtures/tesla_mock/relay@mastdon.example.org.json b/test/fixtures/tesla_mock/relay@mastdon.example.org.json index c1fab7d3b..21dd405c8 100644 --- a/test/fixtures/tesla_mock/relay@mastdon.example.org.json +++ b/test/fixtures/tesla_mock/relay@mastdon.example.org.json @@ -11,7 +11,7 @@ "toot": "http://joinmastodon.org/ns#", "Emoji": "toot:Emoji" }], - "id": "http://mastodon.example.org/users/admin", + "id": "http://mastodon.example.org/users/relay", "type": "Application", "invisible": true, "following": "http://mastodon.example.org/users/admin/following", @@ -24,8 +24,8 @@ "url": "http://mastodon.example.org/@admin", "manuallyApprovesFollowers": false, "publicKey": { - "id": "http://mastodon.example.org/users/admin#main-key", - "owner": "http://mastodon.example.org/users/admin", + "id": "http://mastodon.example.org/users/relay#main-key", + "owner": "http://mastodon.example.org/users/relay", "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAtc4Tir+3ADhSNF6VKrtW\nOU32T01w7V0yshmQei38YyiVwVvFu8XOP6ACchkdxbJ+C9mZud8qWaRJKVbFTMUG\nNX4+6Q+FobyuKrwN7CEwhDALZtaN2IPbaPd6uG1B7QhWorrY+yFa8f2TBM3BxnUy\nI4T+bMIZIEYG7KtljCBoQXuTQmGtuffO0UwJksidg2ffCF5Q+K//JfQagJ3UzrR+\nZXbKMJdAw4bCVJYs4Z5EhHYBwQWiXCyMGTd7BGlmMkY6Av7ZqHKC/owp3/0EWDNz\nNqF09Wcpr3y3e8nA10X40MJqp/wR+1xtxp+YGbq/Cj5hZGBG7etFOmIpVBrDOhry\nBwIDAQAB\n-----END PUBLIC KEY-----\n" }, "attachment": [{ diff --git a/test/pleroma/collections/collections_fetcher_test.exs b/test/pleroma/collections/collections_fetcher_test.exs index 7a582a3d7..ff1aa84db 100644 --- a/test/pleroma/collections/collections_fetcher_test.exs +++ b/test/pleroma/collections/collections_fetcher_test.exs @@ -12,11 +12,14 @@ defmodule Akkoma.Collections.FetcherTest do end test "it should extract items from an embedded array in a Collection" do + ap_id = "https://example.com/collection/ordered_array" + unordered_collection = "test/fixtures/collections/unordered_array.json" |> File.read!() - - ap_id = "https://example.com/collection/ordered_array" + |> Jason.decode!() + |> Map.put("id", ap_id) + |> Jason.encode!(pretty: true) Tesla.Mock.mock(fn %{ diff --git a/test/pleroma/object/containment_test.exs b/test/pleroma/object/containment_test.exs index ad1660069..f8f40a3ac 100644 --- a/test/pleroma/object/containment_test.exs +++ b/test/pleroma/object/containment_test.exs @@ -105,6 +105,56 @@ defmodule Pleroma.Object.ContainmentTest do ) end + test "contain_id_to_fetch() refuses alternate IDs within the same origin domain" do + data = %{ + "id" => "http://example.com/~alyssa/activities/1234.json", + "url" => "http://example.com/@alyssa/status/1234" + } + + :error = + Containment.contain_id_to_fetch( + "http://example.com/~alyssa/activities/1234", + data + ) + end + + test "contain_id_to_fetch() allows matching IDs" do + data = %{ + "id" => "http://example.com/~alyssa/activities/1234.json/" + } + + :ok = + Containment.contain_id_to_fetch( + "http://example.com/~alyssa/activities/1234.json/", + data + ) + + :ok = + Containment.contain_id_to_fetch( + "http://example.com/~alyssa/activities/1234.json", + data + ) + end + + test "contain_id_to_fetch() allows display URLs" do + data = %{ + "id" => "http://example.com/~alyssa/activities/1234.json", + "url" => "http://example.com/@alyssa/status/1234" + } + + :ok = + Containment.contain_id_to_fetch( + "http://example.com/@alyssa/status/1234", + data + ) + + :ok = + Containment.contain_id_to_fetch( + "http://example.com/@alyssa/status/1234/", + data + ) + end + test "users cannot be collided through fake direction spoofing attempts" do _user = insert(:user, %{ diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index a761578f9..c59d77f0f 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -57,6 +57,46 @@ defmodule Pleroma.Object.FetcherTest do body: spoofed_object_with_ids("https://patch.cx/objects/spoof_content_type") } + # Spoof: mismatching ids + # Variant 1: Non-exisitng fake id + %{ + method: :get, + url: + "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json" + } -> + %Tesla.Env{ + status: 200, + url: + "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json", + headers: [{"content-type", "application/activity+json"}], + body: spoofed_object_with_ids() + } + + %{method: :get, url: "https://patch.cx/objects/spoof"} -> + %Tesla.Env{ + status: 404, + url: "https://patch.cx/objects/spoof", + headers: [], + body: "Not found" + } + + # Varaint 2: two-stage payload + %{method: :get, url: "https://patch.cx/media/spoof_stage1.json"} -> + %Tesla.Env{ + status: 200, + url: "https://patch.cx/media/spoof_stage1.json", + headers: [{"content-type", "application/activity+json"}], + body: spoofed_object_with_ids("https://patch.cx/media/spoof_stage2.json") + } + + %{method: :get, url: "https://patch.cx/media/spoof_stage2.json"} -> + %Tesla.Env{ + status: 200, + url: "https://patch.cx/media/spoof_stage2.json", + headers: [{"content-type", "application/activity+json"}], + body: spoofed_object_with_ids("https://patch.cx/media/unpredictable.json") + } + # Spoof: cross-domain redirect with original domain id %{method: :get, url: "https://patch.cx/objects/spoof_media_redirect1"} -> %Tesla.Env{ @@ -79,7 +119,7 @@ defmodule Pleroma.Object.FetcherTest do %{method: :get, url: "https://patch.cx/objects/spoof_redirect"} -> %Tesla.Env{ status: 200, - url: "https://patch.cx/objects/spoof", + url: "https://patch.cx/objects/spoof_redirect", headers: [{"content-type", "application/activity+json"}], body: spoofed_object_with_ids("https://patch.cx/objects/spoof_redirect") } @@ -110,6 +150,7 @@ defmodule Pleroma.Object.FetcherTest do %{method: :get, url: "https://social.sakamoto.gq/notice/9wTkLEnuq47B25EehM"} -> %Tesla.Env{ status: 200, + url: "https://social.sakamoto.gq/objects/f20f2497-66d9-4a52-a2e1-1be2a39c32c1", body: File.read!("test/fixtures/fetch_mocks/9wTkLEnuq47B25EehM.json"), headers: HttpRequestMock.activitypub_object_headers() } @@ -208,6 +249,18 @@ defmodule Pleroma.Object.FetcherTest do ) end + test "it does not fetch a spoofed object with id different from URL" do + assert {:error, "Object's ActivityPub id/url does not match final fetch URL"} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json" + ) + + assert {:error, "Object's ActivityPub id/url does not match final fetch URL"} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://patch.cx/media/spoof_stage1.json" + ) + end + test "it does not fetch an object via cross-domain redirects (initial id)" do assert {:error, {:cross_domain_redirect, true}} = Fetcher.fetch_and_contain_remote_object_from_id( diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index c6543ec83..5ad6d4716 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -312,7 +312,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do end test "fetches user featured collection using the first property" do - featured_url = "https://friendica.example.com/raha/collections/featured" + featured_url = "https://friendica.example.com/featured/raha" first_url = "https://friendica.example.com/featured/raha?page=1" featured_data = @@ -350,7 +350,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do end test "fetches user featured when it has string IDs" do - featured_url = "https://example.com/alisaie/collections/featured" + featured_url = "https://example.com/users/alisaie/collections/featured" dead_url = "https://example.com/users/alisaie/statuses/108311386746229284" featured_data = @@ -1720,7 +1720,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do json( %{ "@context" => "https://www.w3.org/ns/activitystreams", - "id" => "http://remote.org/users/masto_hidden_counters/followers" + "id" => "http://remote.org/users/masto_hidden_counters/following" }, headers: HttpRequestMock.activitypub_object_headers() ) @@ -1732,7 +1732,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do json( %{ "@context" => "https://www.w3.org/ns/activitystreams", - "id" => "http://remote.org/users/masto_hidden_counters/following" + "id" => "http://remote.org/users/masto_hidden_counters/followers" }, headers: HttpRequestMock.activitypub_object_headers() ) diff --git a/test/support/http_request_mock.ex b/test/support/http_request_mock.ex index e831f43f7..cc0e22af1 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -572,6 +572,7 @@ defmodule HttpRequestMock do }} end + # Mastodon status via display URL def get( "http://mastodon.example.org/@admin/99541947525187367", _, @@ -581,6 +582,23 @@ defmodule HttpRequestMock do {:ok, %Tesla.Env{ status: 200, + url: "http://mastodon.example.org/@admin/99541947525187367", + body: File.read!("test/fixtures/mastodon-note-object.json"), + headers: activitypub_object_headers() + }} + end + + # same status via its canonical ActivityPub id + def get( + "http://mastodon.example.org/users/admin/statuses/99541947525187367", + _, + _, + _ + ) do + {:ok, + %Tesla.Env{ + status: 200, + url: "http://mastodon.example.org/users/admin/statuses/99541947525187367", body: File.read!("test/fixtures/mastodon-note-object.json"), headers: activitypub_object_headers() }}