diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex index 7b1cc37bd..b0944e782 100644 --- a/lib/pleroma/object/containment.ex +++ b/lib/pleroma/object/containment.ex @@ -157,4 +157,16 @@ def same_origin(id1, id2) do compare_uris(uri1, uri2) end + + @doc """ + Checks whether a key_id - owner_id pair are acceptable. + + While in theory keys and actors on different domain could be verified + by fetching both and checking the links on both ends (if different at all), + this requires extra fetches and there are no known implementations with split + actor and key domains, thus atm this simply requires same domain. + """ + def contain_key_user(key_id, user_id) do + same_origin(key_id, user_id) + end end diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 0f129f318..c76974adc 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -275,6 +275,42 @@ def fetch_and_contain_remote_object_from_id(id, is_ap_id) when is_binary(id) do def fetch_and_contain_remote_object_from_id(_id, _is_ap_id), do: {:error, :invalid_id} + def fetch_and_contain_remote_key(id) do + Logger.debug("Fetching remote actor key #{id}") + + fetch_and_contain_remote_ap_doc( + id, + # key IDs are alwas AP IDs which should resolve directly and exactly + true, + fn + final_uri, %{"id" => user_id, "publicKey" => %{"id" => key_id}} -> + # For non-fragment keys as used e.g. by GTS, the "id" won't match the fetch URI, + # but the key ID will. Thus do NOT strict check the top-lelve id, but byte-exact + # check the key ID (since for later lookups we need byte-exact matches). + # This relies on fetching already enforcing a domain match for toplevel id and host + with :ok <- Containment.contain_key_user(key_id, user_id), + true <- key_id == final_uri do + {:ok, key_id} + else + _ -> {:error, key_id} + end + + final_uri, + %{"type" => "CryptographicKey", "id" => key_id, "owner" => user_id, "publicKeyPem" => _} -> + # XXX: refactor into separate function isntead of duplicating + with :ok <- Containment.contain_key_user(key_id, user_id), + true <- key_id == final_uri do + {:ok, key_id} + else + _ -> {:error, nil} + end + + _, _ -> + {:error, nil} + end + ) + end + # Fetches an AP document and performing variable security checks on it. # # Note that the received documents "id" matching the final host domain diff --git a/lib/pleroma/user/signing_key.ex b/lib/pleroma/user/signing_key.ex index 87149aa58..04cdced59 100644 --- a/lib/pleroma/user/signing_key.ex +++ b/lib/pleroma/user/signing_key.ex @@ -196,7 +196,7 @@ def fetch_remote_key(key_id) do Logger.debug("Fetching remote key: #{key_id}") with {:ok, _body} = resp <- - Pleroma.Object.Fetcher.fetch_and_contain_remote_object_from_id(key_id), + Pleroma.Object.Fetcher.fetch_and_contain_remote_key(key_id), {:ok, ap_id, public_key_pem} <- handle_signature_response(resp) do Logger.debug("Fetched remote key: #{ap_id}") # fetch the user diff --git a/lib/pleroma/web/activity_pub/object_validators/user_validator.ex b/lib/pleroma/web/activity_pub/object_validators/user_validator.ex index b80068e37..f75e2f87e 100644 --- a/lib/pleroma/web/activity_pub/object_validators/user_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/user_validator.ex @@ -22,7 +22,8 @@ def validate(object, meta) def validate(%{"type" => type, "id" => _id} = data, meta) when type in Pleroma.Constants.actor_types() do - with :ok <- validate_inbox(data), + with :ok <- validate_pubkey(data), + :ok <- validate_inbox(data), :ok <- contain_collection_origin(data) do {:ok, data, meta} else @@ -33,6 +34,21 @@ def validate(%{"type" => type, "id" => _id} = data, meta) def validate(_, _), do: {:error, "Not a user object"} + defp validate_pubkey(%{"id" => user_id, "publicKey" => %{"id" => pk_id, "publicKeyPem" => _key}}) do + with {_, true} <- {:user, is_binary(user_id)}, + {_, true} <- {:key, is_binary(pk_id)}, + :ok <- Containment.contain_key_user(pk_id, user_id) do + :ok + else + {:user, _} -> {:error, "Invalid user id: #{inspect(user_id)}"} + {:key, _} -> {:error, "Invalid key id: #{inspect(pk_id)}"} + :error -> {:error, "Problematic actor-key pairing: #{user_id} - #{pk_id}"} + end + end + + # pubkey is optional atm + defp validate_pubkey(_data), do: :ok + defp validate_inbox(%{"id" => id, "inbox" => inbox}) do case Containment.same_origin(id, inbox) do :ok -> :ok diff --git a/test/fixtures/tesla_mock/evil@remote.example_with_keyid_from_admin@mastdon.example.org.json b/test/fixtures/tesla_mock/evil@remote.example_with_keyid_from_admin@mastdon.example.org.json new file mode 100644 index 000000000..f6c801096 --- /dev/null +++ b/test/fixtures/tesla_mock/evil@remote.example_with_keyid_from_admin@mastdon.example.org.json @@ -0,0 +1,54 @@ +{ + "@context": [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + { + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", + "sensitive": "as:sensitive", + "movedTo": "as:movedTo", + "Hashtag": "as:Hashtag", + "ostatus": "http://ostatus.org#", + "atomUri": "ostatus:atomUri", + "inReplyToAtomUri": "ostatus:inReplyToAtomUri", + "conversation": "ostatus:conversation", + "toot": "http://joinmastodon.org/ns#", + "Emoji": "toot:Emoji", + "alsoKnownAs": { + "@id": "as:alsoKnownAs", + "@type": "@id" + } + } + ], + "id": "http://remote.example/users/with_key_id_of_admin-mastodon.example.org", + "type": "Person", + "following": "http://remote.example/users/evil/following", + "followers": "http://remote.example/users/evil/followers", + "inbox": "http://remote.example/users/evil/inbox", + "outbox": "http://remote.example/users/evil/outbox", + "preferredUsername": "evil", + "name": null, + "discoverable": "true", + "summary": "hii", + "url": "http://remote.example/@evil", + "manuallyApprovesFollowers": false, + "publicKey": { + "id": "http://mastodon.example.org/users/admin#main-key", + "owner": "http://remote.example/users/evil", + "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": [], + "endpoints": { + "sharedInbox": "http://remote.example/inbox" + }, + "icon": { + "type": "Image", + "mediaType": "image/jpeg", + "url": "https://cdn.niu.moe/accounts/avatars/000/033/323/original/fd7f8ae0b3ffedc9.jpeg" + }, + "image": { + "type": "Image", + "mediaType": "image/png", + "url": "https://cdn.niu.moe/accounts/headers/000/033/323/original/850b3448fa5fd477.png" + }, + "alsoKnownAs": [] +} diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index 0d5c9faec..b280a269c 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -10,6 +10,7 @@ defmodule Pleroma.UserTest do alias Pleroma.Repo alias Pleroma.Tests.ObanHelpers alias Pleroma.User + alias Pleroma.User.SigningKey alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.CommonAPI @@ -908,6 +909,35 @@ test "it doesn't fail on invalid alsoKnownAs entries" do assert {:ok, %User{also_known_as: []}} = User.get_or_fetch_by_ap_id("https://mbp.example.com/") end + + test "doesn't allow key_id poisoning" do + {:error, {:validate, {:error, "Problematic actor-key pairing:" <> _}}} = + User.fetch_by_ap_id( + "http://remote.example/users/with_key_id_of_admin-mastodon.example.org" + ) + + used_key_id = "http://mastodon.example.org/users/admin#main-key" + refute Repo.get_by(SigningKey, key_id: used_key_id) + + {:ok, user} = User.fetch_by_ap_id("http://mastodon.example.org/users/admin") + user = SigningKey.load_key(user) + + # ensure we checked for the right key before + assert user.signing_key.key_id == used_key_id + end + + test "doesn't allow key_id takeovers" do + {:ok, user} = User.fetch_by_ap_id("http://mastodon.example.org/users/admin") + user = SigningKey.load_key(user) + + {:error, {:validate, {:error, "Problematic actor-key pairing:" <> _}}} = + User.fetch_by_ap_id( + "http://remote.example/users/with_key_id_of_admin-mastodon.example.org" + ) + + refreshed_sk = Repo.get_by(SigningKey, key_id: user.signing_key.key_id) + assert refreshed_sk.user_id == user.id + end end test "returns an ap_id for a user" do diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 7cf280a17..49bbb9d63 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -561,7 +561,7 @@ test "it inserts an incoming activity into the database", %{conn: conn} do |> assign(:valid_signature, true) |> put_req_header( "signature", - "keyId=\"http://mastodon.example.org/users/admin/main-key\"" + "keyId=\"http://mastodon.example.org/users/admin#main-key\"" ) |> put_req_header("content-type", "application/activity+json") |> post("/inbox", data) @@ -679,7 +679,7 @@ test "accepts Add/Remove activities", %{conn: conn} do |> String.replace("{{nickname}}", "lain") actor = "https://example.com/users/lain" - key_id = "#{actor}/main-key" + key_id = "#{actor}#main-key" insert(:user, ap_id: actor, @@ -741,7 +741,7 @@ test "accepts Add/Remove activities", %{conn: conn} do assert "ok" == conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{actor}/main-key\"") + |> put_req_header("signature", "keyId=\"#{actor}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/inbox", data) |> json_response(200) @@ -764,7 +764,7 @@ test "accepts Add/Remove activities", %{conn: conn} do assert "ok" == conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{actor}/main-key\"") + |> put_req_header("signature", "keyId=\"#{actor}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/inbox", data) |> json_response(200) @@ -790,7 +790,7 @@ test "mastodon pin/unpin", %{conn: conn} do |> String.replace("{{nickname}}", "lain") actor = "https://example.com/users/lain" - key_id = "#{actor}/main-key" + key_id = "#{actor}#main-key" sender = insert(:user, @@ -883,7 +883,7 @@ test "mastodon pin/unpin", %{conn: conn} do assert "ok" == conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{actor}/main-key\"") + |> put_req_header("signature", "keyId=\"#{actor}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/inbox", data) |> json_response(200) @@ -915,7 +915,7 @@ test "it inserts an incoming activity into the database", %{conn: conn, data: da conn = conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{data["actor"]}/main-key\"") + |> put_req_header("signature", "keyId=\"#{data["actor"]}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/users/#{user.nickname}/inbox", data) @@ -939,7 +939,7 @@ test "it accepts messages with to as string instead of array", %{conn: conn, dat conn = conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{data["actor"]}/main-key\"") + |> put_req_header("signature", "keyId=\"#{data["actor"]}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/users/#{user.nickname}/inbox", data) @@ -961,7 +961,7 @@ test "it accepts messages with cc as string instead of array", %{conn: conn, dat conn = conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{data["actor"]}/main-key\"") + |> put_req_header("signature", "keyId=\"#{data["actor"]}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/users/#{user.nickname}/inbox", data) @@ -988,7 +988,7 @@ test "it accepts messages with bcc as string instead of array", %{conn: conn, da conn = conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{data["actor"]}/main-key\"") + |> put_req_header("signature", "keyId=\"#{data["actor"]}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/users/#{user.nickname}/inbox", data) diff --git a/test/support/http_request_mock.ex b/test/support/http_request_mock.ex index d14434333..4d2ad559d 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -424,7 +424,7 @@ def get("http://mastodon.example.org/users/admin", _, _, _) do }} end - def get("http://mastodon.example.org/users/admin/main-key", _, _, _) do + def get("http://mastodon.example.org/users/admin#main-key", _, _, _) do {:ok, %Tesla.Env{ status: 200, @@ -433,6 +433,19 @@ def get("http://mastodon.example.org/users/admin/main-key", _, _, _) do }} end + def get("http://remote.example/users/with_key_id_of_admin-mastodon.example.org" = url, _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + url: url, + body: + File.read!( + "test/fixtures/tesla_mock/evil@remote.example_with_keyid_from_admin@mastdon.example.org.json" + ), + headers: activitypub_object_headers() + }} + end + def get( "http://mastodon.example.org/users/admin/statuses/99512778738411822/replies?min_id=99512778738411824&page=true", _,