From 8684964c5d03f6c70f73730b3f1ad26784ffb004 Mon Sep 17 00:00:00 2001 From: Oneric Date: Fri, 15 Mar 2024 23:00:19 -0100 Subject: [PATCH] Only allow exact id matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This protects us from falling for obvious spoofs as from the current upload exploit (unfortunately we can’t reasonably do anything about spoofs with exact matches as was possible via emoji and proxy). Such objects being invalid is supported by the spec, sepcifically sections 3.1 and 3.2: https://www.w3.org/TR/activitypub/#obj-id Anonymous objects are not relevant here (they can only exists within parent objects iiuc) and neither is client-to-server or transient objects (as those cannot be fetched in the first place). This leaves us with the requirement for `id` to (a) exist and (b) be a publicly dereferencable URI from the originating server. This alone does not yet demand strict equivalence, but the spec then further explains objects ought to be fetchable _via their ID_. Meaning an object not retrievable via its ID, is invalid. This reading is supported by the fact, e.g. GoToSocial (recently) and Mastodon (for 6+ years) do already implement such strict ID checks, additionally proving this doesn’t cause federation issues in practice. However, apart from canonical IDs there can also be additional display URLs. *omas first redirect those to their canonical location, but *keys and Mastodon directly serve the AP representation without redirects. Mastodon and GTS deal with this in two different ways, but both constitute an effective countermeasure: - Mastodon: Unless it already is a known AP id, two fetches occur. The first fetch just reads the `id` property and then refetches from the id. The last fetch requires the returned id to exactly match the URL the content was fetched from. (This can be optimised by skipping the second fetch if it already matches) https://github.com/mastodon/mastodon/blob/05eda8d19330a9c27c0cf07de19a87edff269057/app/helpers/jsonld_helper.rb#L168 https://github.com/mastodon/mastodon/commit/63f097979990bf5ba9db848b8a253056bad781af - GTS: Only does a single fetch and then checks if _either_ the id _or_ url property (which can be an object) match the original fetch URL. This relies on implementations always including their display URL as "url" if differing from the id. For actors this is true for all investigated implementations, for posts only Mastodon includes an "url", but it is also the only one with a differing display URL. https://github.com/superseriousbusiness/gotosocial/commit/2bafd7daf542d985ee76d9079a30a602cb7be827#diff-943bbb02c8ac74ac5dc5d20807e561dcdfaebdc3b62b10730f643a20ac23c24fR222 Albeit Mastodon’s refetch offers higher compatibility with theoretical implmentations using either multiple different display URL or not denoting any of them as "url" at all, for now we chose to adopt a GTS-like refetch-free approach to avoid additional implementation concerns wrt to whether redirects should be allowed when fetching a canonical AP id and potential for accidentally loosening some checks (e.g. cross-domain refetches) for one of the fetches. This may be reconsidered in the future. --- lib/pleroma/object/containment.ex | 35 ++++++++++++ lib/pleroma/object/fetcher.ex | 5 +- .../https__info.pleroma.site_activity3.json | 2 +- .../tesla_mock/relay@mastdon.example.org.json | 6 +- .../collections/collections_fetcher_test.exs | 7 ++- test/pleroma/object/containment_test.exs | 50 +++++++++++++++++ test/pleroma/object/fetcher_test.exs | 55 ++++++++++++++++++- .../web/activity_pub/activity_pub_test.exs | 8 +-- test/support/http_request_mock.ex | 18 ++++++ 9 files changed, 174 insertions(+), 12 deletions(-) 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 @@ def get_object(_) 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 @@ def contain_origin(id, %{"attributedTo" => actor} = params), 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 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) 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 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) 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 @@ test "contain_origin_from_id() allows matching IDs" 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 @@ defp spoofed_object_with_ids( 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 @@ defp spoofed_object_with_ids( %{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 @@ defp spoofed_object_with_ids( %{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 @@ test "it does not fetch a spoofed object with wrong content type" 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 @@ test "fetches user featured collection" 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 @@ test "fetches user featured collection using the first property" 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 @@ test "doesn't crash when follower and following counters are hidden" 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 @@ test "doesn't crash when follower and following counters are hidden" 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 @@ def get("https://social.stopwatchingus-heidelberg.de/.well-known/host-meta", _, }} end + # Mastodon status via display URL def get( "http://mastodon.example.org/@admin/99541947525187367", _, @@ -581,6 +582,23 @@ def get( {: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() }}