diff --git a/changelog.d/fetch-id-containment.security b/changelog.d/fetch-id-containment.security new file mode 100644 index 000000000..aee5eca95 --- /dev/null +++ b/changelog.d/fetch-id-containment.security @@ -0,0 +1 @@ +Only accept fetched objects whose id is the URL they were fetched from; for other ids, refetch once from the canonical id on the same host diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex index b50d560d1..fe42bfa69 100644 --- a/lib/pleroma/object/containment.ex +++ b/lib/pleroma/object/containment.ex @@ -108,6 +108,34 @@ defmodule Pleroma.Object.Containment do def contain_origin_from_id(_id, _data), do: :error + @doc """ + Checks that a fetched document's id is the URL it was fetched from + (after redirects). + + Fragments, a trailing slash, the case of the host and differences + that our URL encoding removes before a request are ignored. + Documents without an id never match. + + Based on Akkoma's `contain_id_to_fetch/2`. + """ + def contain_id_to_fetch(url, %{"id" => id}) when is_binary(url) and is_binary(id) do + if normalize_uri(url) == normalize_uri(id), do: :ok, else: :error + end + + def contain_id_to_fetch(_url, _data), do: :error + + defp normalize_uri(uri) do + uri = uri |> Pleroma.Utils.URIEncoding.encode_url() |> URI.parse() + + %URI{ + uri + | fragment: nil, + host: uri.host && String.downcase(uri.host, :ascii), + path: uri.path && String.replace_suffix(uri.path, "/", "") + } + |> URI.to_string() + end + def contain_child(%{"object" => %{"id" => id, "attributedTo" => _} = object}), do: contain_origin(id, object) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 084d44d5e..8a6f5561f 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -140,17 +140,39 @@ defmodule Pleroma.Object.Fetcher do def fetch_and_contain_remote_object_from_id(%{"id" => id}), do: fetch_and_contain_remote_object_from_id(id) - def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do + def fetch_and_contain_remote_object_from_id(id) when is_binary(id), + do: fetch_and_contain(id, false) + + def fetch_and_contain_remote_object_from_id(_id), + do: {:error, "id must be a string"} + + # The fetched document's id must be the URL it came from. Display URLs + # (e.g. Mastodon's /@user/123) serve the object under another id, so + # if the id differs we refetch once from that id, which has to be on the + # same host, must not redirect, and must then match exactly. + # Based on Akkoma 8684964c5 and 940792f8b. + defp fetch_and_contain(id, canonical?) do Logger.debug("Fetching object #{id} via AP") with {:scheme, true} <- {:scheme, String.starts_with?(id, "http")}, {_, true} <- {:mrf, MRF.id_filter(id)}, {_, :ok} <- {:local_fetch, Containment.contain_local_fetch(id)}, - {:ok, body} <- get_object(id), + {:ok, final_url, body} <- get_object(id), + {_, true} <- {:redirect, not canonical? or same_url?(final_url, id)}, {:ok, data} <- safe_json_decode(body), - :ok <- Containment.contain_origin_from_id(id, data) do + :ok <- Containment.contain_origin_from_id(id, data), + {_, _, :ok} <- {:id, data, Containment.contain_id_to_fetch(final_url, data)} do {:ok, data} else + {:id, %{"id" => canonical_id}, :error} when is_binary(canonical_id) and not canonical? -> + fetch_and_contain(canonical_id, true) + + {:id, _, :error} -> + {:error, :id_mismatch} + + {:redirect, false} -> + {:error, :canonical_id_redirect} + {:scheme, _} -> {:error, "Unsupported URI scheme"} @@ -168,8 +190,7 @@ defmodule Pleroma.Object.Fetcher do end end - def fetch_and_contain_remote_object_from_id(_id), - do: {:error, "id must be a string"} + defp same_url?(url, other), do: Containment.contain_id_to_fetch(url, %{"id" => other}) == :ok defp check_crossdomain_redirect(final_host, _original_url) when is_nil(final_host) do {:cross_domain_redirect, false} @@ -195,7 +216,7 @@ defmodule Pleroma.Object.Fetcher do with {:cross_domain_redirect, false} <- check_crossdomain_redirect(remote_host, id), {_, content_type} <- List.keyfind(headers, "content-type", 0), {:ok, _media_type} <- verify_content_type(content_type) do - {:ok, body} + {:ok, if(final_url in [nil, ""], do: id, else: final_url), body} else {:cross_domain_redirect, true} -> {:error, {:cross_domain_redirect, true}} @@ -209,7 +230,7 @@ defmodule Pleroma.Object.Fetcher do case List.keyfind(headers, "content-type", 0) do {_, content_type} -> case verify_content_type(content_type) do - {:ok, _} -> {:ok, body} + {:ok, _} -> {:ok, id, body} error -> error end 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/object/containment_test.exs b/test/pleroma/object/containment_test.exs index 5560dfce5..1319064f2 100644 --- a/test/pleroma/object/containment_test.exs +++ b/test/pleroma/object/containment_test.exs @@ -127,4 +127,38 @@ defmodule Pleroma.Object.ContainmentTest do :ok = Containment.contain_child(data) end end + + describe "contain_id_to_fetch/2" do + test "matches the exact id" do + assert :ok = + Containment.contain_id_to_fetch("https://a.test/o/1", %{ + "id" => "https://a.test/o/1" + }) + end + + test "ignores fragments, a trailing slash and the host case" do + assert :ok = + Containment.contain_id_to_fetch("https://A.test/o/1/", %{ + "id" => "https://a.test/o/1#x" + }) + end + + test "rejects another id on the same host" do + assert :error = + Containment.contain_id_to_fetch("https://a.test/o/1", %{ + "id" => "https://a.test/o/2" + }) + end + + test "rejects a different query" do + assert :error = + Containment.contain_id_to_fetch("https://a.test/o?page=1", %{ + "id" => "https://a.test/o?page=2" + }) + end + + test "rejects documents without an id" do + assert :error = Containment.contain_id_to_fetch("https://a.test/o/1", %{}) + end + end end diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 9afa34fa2..411f69bea 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -48,6 +48,8 @@ defmodule Pleroma.Object.FetcherTest do %{method: :get, url: "https://social.sakamoto.gq/notice/9wTkLEnuq47B25EehM"} -> %Tesla.Env{ status: 200, + # Pleroma redirects /notice/:id to the object id + 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() } @@ -642,7 +644,23 @@ defmodule Pleroma.Object.FetcherTest do }) } - # No redirect - same domain + # Same-domain redirect to the object id + %{method: :get, url: "https://original.test/@actor/redirect"} -> + %Tesla.Env{ + status: 200, + url: "https://original.test/objects/redirect-target", + headers: [{"content-type", "application/activity+json"}], + body: + Jason.encode!(%{ + "id" => "https://original.test/objects/redirect-target", + "type" => "Note", + "content" => "This has a same-domain redirect", + "actor" => "https://original.test/users/actor", + "attributedTo" => "https://original.test/users/actor" + }) + } + + # Same-domain redirect to a document claiming the original URL as id %{method: :get, url: "https://original.test/objects/same-domain-redirect"} -> %Tesla.Env{ status: 200, @@ -695,12 +713,19 @@ defmodule Pleroma.Object.FetcherTest do test "it accepts objects with same-domain redirects" do assert {:ok, data} = Fetcher.fetch_and_contain_remote_object_from_id( - "https://original.test/objects/same-domain-redirect" + "https://original.test/@actor/redirect" ) assert data["content"] == "This has a same-domain redirect" end + test "it rejects redirect targets that claim the original URL as id" do + assert {:error, _} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://original.test/objects/same-domain-redirect" + ) + end + test "it handles responses without URL field (common in tests)" do assert {:ok, data} = Fetcher.fetch_and_contain_remote_object_from_id( @@ -711,6 +736,131 @@ defmodule Pleroma.Object.FetcherTest do end end + describe "id containment" do + setup do + note = fn id, content -> + %Tesla.Env{ + status: 200, + headers: [{"content-type", "application/activity+json"}], + body: + Jason.encode!(%{ + "id" => id, + "type" => "Note", + "content" => content, + "actor" => "https://host.test/users/actor", + "attributedTo" => "https://host.test/users/actor" + }) + } + end + + mock(fn + # display URL serving the canonical object + %{method: :get, url: "https://host.test/@actor/1"} -> + note.("https://host.test/objects/1", "display") + + %{method: :get, url: "https://host.test/objects/1"} -> + note.("https://host.test/objects/1", "canonical") + + # uploaded file claiming to be another object on the same host + %{method: :get, url: "https://host.test/media/evil.json"} -> + note.("https://host.test/objects/2", "spoofed") + + %{method: :get, url: "https://host.test/objects/2"} -> + note.("https://host.test/objects/2", "real") + + # canonical id does not serve itself + %{method: :get, url: "https://host.test/media/loop.json"} -> + note.("https://host.test/objects/3", "loop") + + %{method: :get, url: "https://host.test/objects/3"} -> + note.("https://host.test/objects/4", "other") + + # canonical id redirects + %{method: :get, url: "https://host.test/media/redirect.json"} -> + note.("https://host.test/objects/5", "redirect") + + %{method: :get, url: "https://host.test/objects/5"} -> + %{note.("https://host.test/objects/5", "moved") | url: "https://host.test/media/x"} + + %{method: :get, url: "https://host.test/objects/slash"} -> + note.("https://host.test/objects/slash/", "slash") + + %{method: :get, url: "https://host.test/objects/no-id"} -> + note.(nil, "no id") + end) + + :ok + end + + test "it refetches display URLs from the canonical id" do + assert {:ok, %{"id" => "https://host.test/objects/1", "content" => "canonical"}} = + Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/@actor/1") + end + + test "it does not trust a document claiming another id on the same host" do + assert {:ok, %{"id" => "https://host.test/objects/2", "content" => "real"}} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://host.test/media/evil.json" + ) + end + + test "it refetches at most once" do + assert {:error, :id_mismatch} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://host.test/media/loop.json" + ) + end + + test "it does not follow redirects when refetching from the canonical id" do + assert {:error, _} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://host.test/media/redirect.json" + ) + end + + test "it ignores a trailing slash" do + assert {:ok, %{"content" => "slash"}} = + Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/objects/slash") + end + + test "it accepts ids that only differ by URL encoding" do + clear_config(:test_url_encoding, true) + + mock(fn %{method: :get, url: url} -> + # the encoder sends uppercase escapes + assert url in [ + "https://host.test/users/%E3%81%82", + "https://host.test/notes/%E3%81%82" + ] + + id = + if url =~ "users", + do: "https://host.test/users/あ", + else: "https://host.test/notes/%e3%81%82" + + %Tesla.Env{ + status: 200, + url: url, + headers: [{"content-type", "application/activity+json"}], + body: Jason.encode!(%{"id" => id, "type" => "Note"}) + } + end) + + assert {:ok, _} = + Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/users/あ") + + assert {:ok, _} = + Fetcher.fetch_and_contain_remote_object_from_id( + "https://host.test/notes/%e3%81%82" + ) + end + + test "it rejects documents without an id" do + assert {:error, _} = + Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/objects/no-id") + end + end + describe "fetch with history" do setup do object2 = %{ diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index 125a2b4f9..7294119b8 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -541,7 +541,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do end test "fetches user birthday information from misskey" do - user_id = "https://misskey.io/@mkljczk" + user_id = "https://misskey.io/users/8dhi2ne167" Tesla.Mock.mock(fn %{ @@ -2058,7 +2058,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do json( %{ "@context" => "https://www.w3.org/ns/activitystreams", - "id" => "https://remote.org/users/masto_hidden_counters/followers" + "id" => "https://remote.org/users/masto_hidden_counters/following" }, headers: HttpRequestMock.activitypub_object_headers() ) @@ -2070,7 +2070,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do json( %{ "@context" => "https://www.w3.org/ns/activitystreams", - "id" => "https://remote.org/users/masto_hidden_counters/following" + "id" => "https://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 82affe7ac..94be59536 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -577,6 +577,16 @@ defmodule HttpRequestMock do }} end + # the same status via its canonical id + def get("http://mastodon.example.org/users/admin/statuses/99541947525187367", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/mastodon-note-object.json"), + headers: activitypub_object_headers() + }} + end + def get("http://mastodon.example.org/@admin/99541947525187368", _, _, _) do {:ok, %Tesla.Env{