Prevent key-actor mapping poisoning and key take overs

Previously there were mainly two attack vectors:
 - for raw keys the owner <-> key mapping wasn't verified at all
 - keys were retrieved with refetching allowed
   and only the top-level ID was sanitised while
   usually keys are but a subobject

This reintroduces public key checks in the user actor,
previously removed in 9728e2f8f7
but now adapted to account for the new mapping mechanism.

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

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

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

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

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": []
+}

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

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)
 

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",
         _,