Only accept activities signed with the actor's own key

.github/changelog/fix-signing-key-owner

@@ -0,0 +1,4 @@
+Significance: patch
+Type: security
+
+Incoming activities are now only accepted when they are signed with the sending account's own key.

includes/class-signature.php

@@ -129,15 +129,17 @@ public static function sign_request( $args, $url ) {
 	/**
 	 * Verifies the http signatures
 	 *
-	 * On success the verified keyId is returned (a truthy string), so callers can bind it to
-	 * the activity actor without re-parsing headers, which cannot tell which signature label
-	 * actually validated. Pass/fail callers should branch on {@see is_wp_error()} as before.
+	 * On success the id of the actor that owns the verified key is returned (a truthy string), so
+	 * callers can bind the signer to the activity actor without re-parsing headers, which cannot
+	 * tell which signature label actually validated. Pass/fail callers should branch on
+	 * {@see is_wp_error()} as before.
 	 *
 	 * @since 9.0.0 Returns the verified keyId on success instead of `true`.
+	 * @since unreleased Returns the id of the actor that owns the verified key instead of the keyId.
 	 *
 	 * @param \WP_REST_Request|array $request The request object or $_SERVER array.
 	 *
-	 * @return string|\WP_Error The verified keyId on success, WP_Error on failure.
+	 * @return string|\WP_Error The id of the actor that owns the verified key on success, WP_Error on failure.
 	 */
 	public static function verify_http_signature( $request ) {
 		if ( \is_object( $request ) ) { // REST Request object.

includes/collection/class-remote-actors.php

@@ -809,11 +809,14 @@ public static function normalize_identifier( $actor ) {
 	/**
 	 * Get public key from key_id.
 	 *
-	 * @param string $key_id The URL to the public key.
+	 * @since unreleased Added the `$owner` parameter.
+	 *
+	 * @param string      $key_id The URL to the public key.
+	 * @param string|null $owner  Optional. Set to the id of the actor the key was taken from.
 	 *
 	 * @return resource|\WP_Error The public key resource or WP_Error.
 	 */
-	public static function get_public_key( $key_id ) {
+	public static function get_public_key( $key_id, &$owner = null ) {
 		$no_profile_error = new \WP_Error( 'activitypub_no_remote_profile_found', 'No Profile found or Profile not accessible', array( 'status' => 401 ) );
 		$no_key_error     = new \WP_Error( 'activitypub_no_remote_key_found', 'No Public-Key found', array( 'status' => 401 ) );
 
@@ -862,6 +865,9 @@ public static function get_public_key( $key_id ) {
 			return $no_key_error;
 		}
 
+		// The actor this key belongs to, so the caller can bind it to the activity's actor.
+		$owner = $actor['id'];
+
 		return $key_resource;
 	}
 

includes/functions-request.php

@@ -185,6 +185,16 @@ function maybe_set_no_store( $response ) {
 /**
  * Requests the Meta-Data from the Actors profile.
  *
+ * Avoid this function in new code and use {@see \Activitypub\Proxy::get()} instead. It returns
+ * the stored remote actor first (the `ap_actor` post, via {@see Remote_Actors::fetch_by_various()},
+ * which resolves an `acct:` handle through WebFinger first) and only fetches when none is stored,
+ * so it can be stale, and `$cached` is ignored.
+ *
+ * Every writer of that record stores what `Proxy::get()` fetched from the actor's own host, except
+ * an incoming Update, which stores the actor it embeds. Two checks keep that the actor's own data:
+ * the inbox requires the signing key's owner to be the actor, and `Update::update_actor()` requires
+ * the object's id to be the actor. Neither may be relaxed.
+ *
  * @param array|string $actor  The Actor array or URL.
  * @param bool         $cached Optional. Whether the result should be cached. Default true.
  *

includes/rest/trait-verification.php

@@ -92,17 +92,17 @@ public function verify_signature( $request, $force_signature = false ) {
 		 * URL hands out, so the two must be answered alike.
 		 */
 		if ( ! \in_array( $request->get_method(), array( 'GET', 'HEAD' ), true ) || use_authorized_fetch() || $force_signature ) {
-			$verified_key_id = Signature::verify_http_signature( $request );
-			if ( \is_wp_error( $verified_key_id ) ) {
+			$signer = Signature::verify_http_signature( $request );
+			if ( \is_wp_error( $signer ) ) {
 				return new \WP_Error(
 					'activitypub_signature_verification',
-					$verified_key_id->get_error_message(),
+					$signer->get_error_message(),
 					array( 'status' => 401 )
 				);
 			}
 
-			// Verify the signing key's host matches the activity actor's host.
-			$key_id_check = $this->verify_key_id( $request, $verified_key_id );
+			// Verify the signing key belongs to the activity actor.
+			$key_id_check = $this->verify_key_id( $request, $signer );
 			if ( \is_wp_error( $key_id_check ) ) {
 				return $key_id_check;
 			}
@@ -161,27 +161,28 @@ private function verify_activity_id( $request ) {
 	}
 
 	/**
-	 * Check that the signature keyId and activity actor share the same host.
+	 * Check that the verified signing key belongs to the activity actor.
 	 *
-	 * Binds against the keyId that {@see Signature::verify_http_signature()} actually
-	 * verified, passed in by the caller. Re-parsing the headers here would be unsafe: a
-	 * request can present several signature labels (or a draft and an RFC 9421 header) with
-	 * different keyIds, and only the verifier knows which one validated.
+	 * Binds against the key owner that {@see Signature::verify_http_signature()} returns for the
+	 * signature that actually verified. Re-parsing the headers here would be unsafe: a request can
+	 * present several signature labels (or a draft and an RFC 9421 header) with different keyIds,
+	 * and only the verifier knows which one validated.
 	 *
-	 * Fails closed when either side has no parsable host. A non-URL keyId such as
-	 * `acct:mallory@attacker.example` resolves to a real key through WebFinger yet yields no host,
-	 * so treating "no host" as "nothing to check" would drop the only tie between the signing key
-	 * and the claimed actor. Both hosts must be present and equal.
+	 * The owner is the actor {@see \Activitypub\Collection\Remote_Actors::get_public_key()} took the key from, the stored
+	 * actor or the fetched one, and that lookup already requires it on the keyId's host. A keyId
+	 * with no host (an `acct:` keyId) gets no key there. So comparing the owner with the actor
+	 * also binds the hosts.
 	 *
 	 * @since 8.1.0
 	 * @since 9.0.0 Added the `$key_id` parameter; binds against the verified keyId.
 	 * @since 9.2.1 Reject a keyId or actor with no parsable host instead of allowing it.
+	 * @since unreleased The second parameter is the verified key's owner; it has to be the actor.
 	 *
 	 * @param \WP_REST_Request $request The request object.
-	 * @param string|null      $key_id  The keyId that verified the signature.
+	 * @param string|null      $owner   The id of the actor that owns the verified key.
 	 * @return true|\WP_Error True if valid, WP_Error on mismatch.
 	 */
-	private function verify_key_id( $request, $key_id ) {
+	private function verify_key_id( $request, $owner ) {
 		$json  = $request->get_json_params();
 		$actor = isset( $json['actor'] ) ? object_to_uri( $json['actor'] ) : null;
 
@@ -190,12 +191,17 @@ private function verify_key_id( $request, $key_id ) {
 			return true;
 		}
 
-		// The keyId and the actor must resolve to the same host; an identifier with no parsable
-		// host (e.g. an `acct:` keyId) is unbindable and therefore not an authorized one.
-		if ( ! is_same_host( $key_id, $actor ) ) {
+		/*
+		 * The same host is not enough: on a server where users hold their own keys, one user's
+		 * key would pass for another user on that server, so the key's owner has to be the actor.
+		 * The stored actor the owner may come from can be trusted because this check is what keeps
+		 * an Update from writing another actor's key into it. The host check fails closed on an
+		 * identifier without a host, which could otherwise match itself.
+		 */
+		if ( ! is_same_host( $owner, $actor ) || $owner !== $actor ) {
 			return new \WP_Error(
 				'activitypub_key_actor_mismatch',
-				\__( 'Signing key and activity actor must be on the same host.', 'activitypub' ),
+				\__( 'Signing key does not belong to the activity actor.', 'activitypub' ),
 				array( 'status' => 403 )
 			);
 		}

includes/signature/class-http-message-signature.php

@@ -137,10 +137,11 @@ public function sign( $args, $url ) {
 	 * Verify the HTTP Signature against a request.
 	 *
 	 * @since 9.0.0 Returns the verified keyId on success instead of `true`.
+	 * @since unreleased Returns the id of the actor that owns the verified key instead of the keyId.
 	 *
 	 * @param array       $headers The HTTP headers.
 	 * @param string|null $body    The request body, if applicable.
-	 * @return string|\WP_Error The verified keyId on success, WP_Error on failure.
+	 * @return string|\WP_Error The id of the actor that owns the verified key on success, WP_Error on failure.
 	 */
 	public function verify( array $headers, $body = null ) {
 		$parsed = $this->parse_signature_labels( $headers );
@@ -151,17 +152,15 @@ public function verify( array $headers, $body = null ) {
 		$errors = new \WP_Error();
 		foreach ( $parsed as $data ) {
 			$result = $this->verify_signature_label( $data, $headers, $body );
-			if ( true === $result ) {
+			if ( ! \is_wp_error( $result ) ) {
 				/*
-				 * Return the keyId of the label that actually verified, so the caller binds
+				 * Return the key owner of the label that actually verified, so the caller binds
 				 * against the real signer instead of guessing among several labels.
 				 */
-				return $data['params']['keyid'];
+				return $result;
 			}
 
-			if ( \is_wp_error( $result ) ) {
-				$errors->add( $result->get_error_code(), $result->get_error_message() );
-			}
+			$errors->add( $result->get_error_code(), $result->get_error_message() );
 		}
 
 		// No valid signature found.
@@ -225,7 +224,7 @@ private function parse_signature_labels( array $headers ) {
 	 * @param array       $data     Parsed signature data.
 	 * @param array       $headers  HTTP headers.
 	 * @param string|null $body     Request body, if applicable.
-	 * @return bool|\WP_Error True, if the signature is valid, WP_Error on failure.
+	 * @return string|\WP_Error The id of the actor that owns the label's key, if the signature is valid, WP_Error on failure.
 	 */
 	private function verify_signature_label( $data, $headers, $body ) {
 		$params = $data['params'];
@@ -299,11 +298,17 @@ static function ( $component ) {
 			return new \WP_Error( 'missing_keyid', 'Missing keyId in signature parameters.' );
 		}
 
-		$public_key = Remote_Actors::get_public_key( $params['keyid'] );
+		$owner      = null;
+		$public_key = Remote_Actors::get_public_key( $params['keyid'], $owner );
 		if ( \is_wp_error( $public_key ) ) {
 			return $public_key;
 		}
 
+		// The owner is what callers bind to the actor, so a key without one does not verify anything.
+		if ( ! \is_string( $owner ) || '' === $owner ) {
+			return new \WP_Error( 'activitypub_no_key_owner', 'The signing key has no owner.' );
+		}
+
 		// Algorithm verification.
 		$algorithm = $this->verify_algorithm( $params['alg'] ?? '', $public_key );
 		if ( \is_wp_error( $algorithm ) ) {
@@ -323,7 +328,7 @@ static function ( $component ) {
 			return new \WP_Error( 'activitypub_signature', 'Invalid signature' );
 		}
 
-		return true;
+		return $owner;
 	}
 
 	/**

includes/signature/class-http-signature-draft.php

@@ -91,10 +91,11 @@ public function sign( $args, $url ) {
 	 * Verify the HTTP Signature against a request.
 	 *
 	 * @since 9.0.0 Returns the verified keyId on success instead of `true`.
+	 * @since unreleased Returns the id of the actor that owns the verified key instead of the keyId.
 	 *
 	 * @param array       $headers The HTTP headers.
 	 * @param string|null $body    The request body, if applicable.
-	 * @return string|\WP_Error The verified keyId on success, WP_Error on failure.
+	 * @return string|\WP_Error The id of the actor that owns the verified key on success, WP_Error on failure.
 	 */
 	public function verify( array $headers, $body = null ) {
 		if ( ! isset( $headers['signature'] ) && ! isset( $headers['authorization'] ) ) {
@@ -127,11 +128,17 @@ public function verify( array $headers, $body = null ) {
 			return new \WP_Error( 'invalid_signed_data', 'Signed data is invalid or expired.' );
 		}
 
-		$public_key = Remote_Actors::get_public_key( $parsed['keyId'] );
+		$owner      = null;
+		$public_key = Remote_Actors::get_public_key( $parsed['keyId'], $owner );
 		if ( \is_wp_error( $public_key ) ) {
 			return $public_key;
 		}
 
+		// The owner is what callers bind to the actor, so a key without one does not verify anything.
+		if ( ! \is_string( $owner ) || '' === $owner ) {
+			return new \WP_Error( 'activitypub_no_key_owner', 'The signing key has no owner.', array( 'status' => 401 ) );
+		}
+
 		$algorithm = $this->get_signature_algorithm( $parsed, $public_key );
 		if ( \is_wp_error( $algorithm ) ) {
 			return $algorithm;
@@ -148,8 +155,8 @@ public function verify( array $headers, $body = null ) {
 			return new \WP_Error( 'activitypub_signature', 'Invalid signature', array( 'status' => 401 ) );
 		}
 
-		// Return the verified keyId so the caller can bind it to the activity actor.
-		return $parsed['keyId'];
+		// Return the key's owner so the caller can bind it to the activity actor.
+		return $owner;
 	}
 
 	/**

includes/signature/interface-http-signature.php

@@ -32,7 +32,7 @@ public function sign( $args, $url );
 	 *
 	 * @param array       $headers The HTTP headers.
 	 * @param string|null $body    The request body, if applicable.
-	 * @return string|\WP_Error The verified keyId on success, WP_Error on failure.
+	 * @return string|\WP_Error The id of the actor that owns the verified key on success, WP_Error on failure.
 	 */
 	public function verify( array $headers, $body = null );
 

tests/phpunit/tests/includes/class-test-signature.php

@@ -1157,9 +1157,9 @@ public function test_verify_http_signature_rfc9421_returns_verifying_label_key_i
 		$result = Signature::verify_http_signature( $request );
 
 		$this->assertSame(
-			'https://example.org/author/admin#main-key',
+			'https://example.org/author/admin',
 			$result,
-			'The keyId of the label that actually verified must be returned, not the first label.'
+			'The key owner of the label that actually verified must be returned, not the first label.'
 		);
 
 		\remove_filter( 'activitypub_pre_http_get_remote_object', $mock_remote_key_retrieval );
@@ -1823,7 +1823,7 @@ public function test_verify_http_signature_returns_verified_key_id() {
 
 		$result = Signature::verify_http_signature( $request );
 
-		$this->assertSame( 'https://example.org/author/admin#main-key', $result, 'The verified keyId is returned on success.' );
+		$this->assertSame( 'https://example.org/author/admin', $result, 'The verified key\'s owner is returned on success.' );
 
 		\remove_filter( 'activitypub_pre_http_get_remote_object', $mock_remote_key_retrieval );
 	}

tests/phpunit/tests/includes/collection/class-test-remote-actors.php

@@ -120,6 +120,85 @@ public function test_get_public_key_redirected_reference( $owner_redirect, $same
 		}
 	}
 
+	/**
+	 * The key's owner is the actor the key was taken from: stored, fetched, or a standalone key's owner.
+	 *
+	 * @covers ::get_public_key
+	 */
+	public function test_get_public_key_sets_owner() {
+		$stored = 'https://example.com/users/stored';
+		Remote_Actors::upsert(
+			array(
+				'id'                => $stored,
+				'type'              => 'Person',
+				'inbox'             => $stored . '/inbox',
+				'preferredUsername' => 'stored',
+				'publicKey'         => array(
+					'id'           => $stored . '#main-key',
+					'owner'        => $stored,
+					'publicKeyPem' => $this->x509_key,
+				),
+			)
+		);
+
+		$this->stub_remote_requests();
+		$fetched                                       = 'https://example.com/users/fetched';
+		$this->responses[ $fetched ]                   = array(
+			'id'        => $fetched,
+			'type'      => 'Person',
+			'publicKey' => array(
+				'id'           => $fetched . '#main-key',
+				'owner'        => $fetched,
+				'publicKeyPem' => $this->x509_key,
+			),
+		);
+		$this->responses['https://example.com/keys/1'] = array(
+			'id'           => 'https://example.com/keys/1',
+			'owner'        => $fetched,
+			'publicKeyPem' => $this->x509_key,
+		);
+
+		$owners = array();
+		foreach ( array( $stored . '#main-key', $fetched . '#main-key', 'https://example.com/keys/1' ) as $key_id ) {
+			$owner = null;
+			$this->assertNotWPError( Remote_Actors::get_public_key( $key_id, $owner ) );
+			$owners[] = $owner;
+		}
+		$this->unstub_remote_requests();
+
+		$this->assertSame( array( $stored, $fetched, $fetched ), $owners );
+	}
+
+	/**
+	 * A keyId without a host cannot name the actor it belongs to, so it gets no key and no owner.
+	 *
+	 * @covers ::get_public_key
+	 */
+	public function test_get_public_key_rejects_a_key_id_without_host() {
+		$actor = array(
+			'id'        => 'https://attacker.example/users/mallory',
+			'type'      => 'Person',
+			'publicKey' => array(
+				'id'           => 'https://attacker.example/users/mallory#main-key',
+				'owner'        => 'https://attacker.example/users/mallory',
+				'publicKeyPem' => $this->x509_key,
+			),
+		);
+		$mock  = static function () use ( $actor ) {
+			return $actor;
+		};
+		\add_filter( 'activitypub_pre_http_get_remote_object', $mock );
+
+		$owner  = null;
+		$result = Remote_Actors::get_public_key( 'acct:mallory@attacker.example', $owner );
+
+		\remove_filter( 'activitypub_pre_http_get_remote_object', $mock );
+
+		$this->assertWPError( $result );
+		$this->assertSame( 'activitypub_no_remote_key_found', $result->get_error_code() );
+		$this->assertNull( $owner );
+	}
+
 	/**
 	 * Key and owner reference redirects.
 	 *

tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php

@@ -494,6 +494,9 @@ public function test_user_inbox_post_verification() {
 		$remote_object_filter = function ( $json, $actor ) {
 			$public_key = Actors::get_public_key( self::$user_id );
 
+			// The key is fetched by its keyId; like a real server, answer with the actor's id without the fragment.
+			$actor = \strip_fragment_from_url( $actor );
+
 			// Return ActivityPub Profile with signature.
 			return array(
 				'id'        => $actor,