From 5ff1fead669ae97fe73075ca03f6c0d96aa2733b Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Wed, 28 Oct 2026 10:14:59 +0100 Subject: [PATCH 1/4] Only accept activities signed with the actor's own key --- .agents/skills/code-style/SKILL.md | 8 +- .claude/agents/security-audit.md | 3 +- .github/changelog/fix-signing-key-owner | 4 + AGENTS.md | 2 + includes/class-signature.php | 10 ++- includes/collection/class-remote-actors.php | 10 ++- includes/functions-request.php | 10 +++ includes/rest/trait-verification.php | 46 ++++++----- .../class-http-message-signature.php | 25 +++--- .../signature/class-http-signature-draft.php | 15 +++- .../signature/interface-http-signature.php | 2 +- .../tests/includes/class-test-signature.php | 6 +- .../collection/class-test-remote-actors.php | 79 +++++++++++++++++++ .../rest/class-test-trait-verification.php | 60 ++++++-------- 14 files changed, 199 insertions(+), 81 deletions(-) create mode 100644 .github/changelog/fix-signing-key-owner diff --git a/.agents/skills/code-style/SKILL.md b/.agents/skills/code-style/SKILL.md index 5aeb9dcb4d..b46671c84e 100644 --- a/.agents/skills/code-style/SKILL.md +++ b/.agents/skills/code-style/SKILL.md @@ -154,8 +154,12 @@ Expose ActivityPub endpoints. ## Plugin-Specific Helper Functions ```php -// Get remote actor metadata. -$metadata = get_remote_metadata_by_actor( $actor_url ); +// Fetch a remote actor or object document from its own host (cached; pass array( 'cached' => false ) for a fresh copy). +$actor = Proxy::get( $actor_url ); + +// Not get_remote_metadata_by_actor(): it returns the stored ap_actor record, which can be stale. +// An incoming Update writes its embedded actor into that record; it only stays the actor's own +// data because the signing key's owner and the object's id both have to be the actor. // Convert ActivityPub object to URI string. $uri = object_to_uri( $object ); diff --git a/.claude/agents/security-audit.md b/.claude/agents/security-audit.md index 7eb8014daa..a2f9c24351 100644 --- a/.claude/agents/security-audit.md +++ b/.claude/agents/security-audit.md @@ -40,6 +40,7 @@ Past CVEs and security fixes inform what patterns to watch for. The full list is 17. **Canonicalization decides access: host folding, Unicode-class regexes and percent-decoding** (2026) — patterns to enforce, stated generally so the audit finds the whole class. When two identifiers are compared, or one is decoded before a lookup, the canonical form *is* the gate. (a) **The folded host must be the host that is contacted.** `fold_host()`/`fold_authority()` run IDNA UTS #46, which also *maps* code points: soft hyphen and zero-width characters vanish, fullwidth and compatibility letters become ASCII (`example.com` folds to `example.com`). WordPress' HTTP layer punycodes the unmapped string instead (`WpOrg\Requests\IdnaEncoder`), so a folded value can name a different DNS host than a fetch of the same string. Today core's `wp_http_validate_url()` refuses non-ASCII hosts (`gethostbyname()` fails), so these URLs are never fetched; any path that A-labels before fetching, or fetches outside the WP HTTP API, changes that. Fold both sides of a gate, never one, and flag a neighbouring gate that still compares raw hosts. (b) **IDNA output can contain characters the input did not:** fullwidth `/`, `@`, `:`, `?`, `#` map to their ASCII forms, so a folded host is not safe to put into a URL until it is checked to be a bare host. (c) **A regex used as a validator is only as strict as its modifiers.** `\p{L}`/`\p{M}`/`\p{N}` without `u` match single *bytes*, which is laxer than the ASCII class they replace; with `u`, `preg_*` returns `false`/`NULL` on invalid UTF-8, so callers need a fail-closed branch, and a `NULL` replacement must never be appended back into content. A documented pattern constant (`ACTIVITYPUB_USERNAME_REGEXP`) must state its modifiers, and every in-repo caller must use them. (d) **A decode widens the input of a lookup.** The REST layer already decodes query parameters once, so after a second `rawurldecode()` magic values (`*`, `_`, the empty string) and encoded spellings of stored identifiers are reachable. Decode the identifier, never a host a gate already compared, and keep an empty host an explicit rejection: it must not match an unset stored host. See Audit Scope §4, §6 and §8. 18. **An ownership check is only as strong as its fallback and its lookup** (2026) — patterns to enforce, stated generally so the audit finds the whole class. History #7 added actor↔owner checks to the Update and Undo paths, and their fallbacks left records open. (a) **A backward-compatibility branch is a gate of its own.** "No owner recorded, so let it through" for rows written before the owner was stored must still bind the actor to something, at least `is_same_host( actor, )`. A missing owner never means "anyone". (b) **A recorded reference that no longer resolves fails closed.** An owner ID whose post was deleted or merged must not switch the comparison off: a guard shaped `if ( $owner instanceof WP_Post && … )` skips the check exactly when the owner is gone. And every path that deletes or merges a record an authorization depends on (actor Delete, Move, scheduled cleanup) has to repoint or remove the references to it in the same step. (c) **Check the value you look up by.** `object_to_uri()` dispatches by type (it returns `url` for `Image`, `Audio`, `Video` and `Document`), so a gate on `object_to_uri( $object )` and a lookup on `$object['id']` can be handed two different identifiers by the sender. Derive the identifier once and use it for the lookup, the host check and the stored key, as `add_comment()` does. See Audit Scope §4 and §8. 19. **Two signature verifiers, one rule** (2026) — the requester chooses the verifier: a `Signature-Input` header selects RFC 9421, otherwise draft-cavage. A binding is only in place once both verifiers enforce it. In both, the signer also picks what is signed, so a signature over `(request-target) date` or `("@method")` verifies on its own terms. Each verifier must require a component that names this site (draft: `host`; RFC 9421: `@target-uri`, `@authority` or `host`; each checked by `is_signature_host()`) and, with a body, a signed digest (`digest` / `content-digest`) whose header is present. A signed but absent header enters the signed data empty and vouches for any body. See Audit Scope §3. +20. **A same-host key is not the actor's key, and the stored actor is not the actor's document** (2026) — `verify_key_id()` bound the signing key to the actor by host only. On a server where users hold their own keys (Friendica lets every user export theirs), user A could sign an Update that claims to be user B on the same host. `Update::update_actor()` then wrote A's key and inbox into B's stored `ap_actor` record, and from then on A's signatures verified as B while B's own failed. Two patterns: (a) the key's owner must be the activity actor, not just share its host (the same rule as Mastodon's `different_actor?`). Take the owner from where the key was resolved: the stored actor the keyId points to, else the key as its host serves it (`Proxy::get()`). Compare the owner, not the keyId string, so same-host key aliases keep working. (b) The stored `ap_actor` record is written from the actor embedded in incoming Updates. Like Mastodon's stored keys, it is only the actor's own data because of (a), and (a) reads from it. So any path that writes a remote actor, its key or its inbox must be bound to the signer the same way, and a check that relaxes (a) poisons the store. See Audit Scope §4. ## Audit Scope @@ -131,7 +132,7 @@ Files: `includes/class-signature.php`, `includes/signature/class-http-signature- Files: `includes/rest/class-inbox-controller.php`, `includes/rest/class-actors-inbox-controller.php`, `includes/handler/` -- Check that `actor` field is validated against the signature's `keyId` origin +- Check that the signature's `keyId` belongs to the `actor`, not only to its host: the key's owner (the stored actor the keyId resolves to, or the key as its host serves it through `Proxy::get()`) has to be the actor. On a server where users hold their own keys, a same-host check lets one user sign for another (see history #20). - Every host-equality check on this surface (keyId ↔ actor, and any `host(ref) === host(actor)` binding) must fail **closed** when a side has no parsable host — non-`http(s)` identifiers parse to a `null` host. Use `is_same_host()` (fails closed); flag any bespoke `wp_parse_url` comparison that passes when a side is empty. Bind **every** remote-supplied reference a handler acts on to the actor's host, including activity-specific reference fields, not only `object` / `attributedTo`. Beware a binding made meaningless because the compared value was auto-derived from the signing key (see history #13). - Every ownership check on an Update/Undo/Delete path: read the branch for a missing or unresolvable owner, and confirm the lookup, the host check and the stored key use one identifier (see history #18). - Every host-equality gate folds **both** sides with `fold_host()`/`fold_authority()`, and the folded value must name the host the plugin would connect to (see history #17): a fold that maps a non-ASCII host onto an unrelated ASCII name can make an attacker's spelling compare equal to a victim host. diff --git a/.github/changelog/fix-signing-key-owner b/.github/changelog/fix-signing-key-owner new file mode 100644 index 0000000000..7da24527ad --- /dev/null +++ b/.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. diff --git a/AGENTS.md b/AGENTS.md index c07148a637..7c089a8783 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,6 +28,8 @@ Prefer reading project files and `docs/` over relying on training data for WordP **ALWAYS reuse existing functionality instead of hand-rolling it.** Before writing any parsing, validation, escaping, or formatting logic, check for a WordPress core function that does the job (e.g. `wp_kses_bad_protocol()`, `wp_parse_url()`, `wp_http_validate_url()`), then for an existing plugin helper (`includes/functions*.php`, `Sanitize`, `src/shared/`). Core primitives are battle-tested against adversarial input in ways a fresh implementation is not, and reviewers can trust them on sight. A custom implementation needs a stated reason why no existing function fits — "shorter" is not one. This applies to refactors too: never replace a core-function-based check with bespoke logic just to save a line. +**Fetch remote documents with `Proxy::get()`.** Do not use `get_remote_metadata_by_actor()` in new code. It returns the stored `ap_actor` record first and only fetches when none is stored, so it can be stale, and it ignores `$cached`. 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 (`verify_key_id()`), and `Update::update_actor()` requires the object's id to be the actor. Never relax either, and give any new writer that stores remote-supplied data the same two checks. `Proxy::get()` is only filled from the actor's own host; pass `array( 'cached' => false )` for a fresh copy. + **MUST** backslash-prefix all WordPress functions in namespaced code: `\get_option()`, `\add_action()`, `\apply_filters()`, `\__()`, `\_e()`, etc. PHP falls back to global scope, but backslashes are a project standard for consistency and to avoid accidentally shadowing globals. **No inline namespaces.** Use `use` statements at the top of the file instead of inline fully-qualified class names (e.g., `use Activitypub\Options;` then `Options::method()`, not `\Activitypub\Options::method()`). diff --git a/includes/class-signature.php b/includes/class-signature.php index 0d9b58e377..531cf6e176 100644 --- a/includes/class-signature.php +++ b/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. diff --git a/includes/collection/class-remote-actors.php b/includes/collection/class-remote-actors.php index 8e482c0167..28c238c3e2 100644 --- a/includes/collection/class-remote-actors.php +++ b/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; } diff --git a/includes/functions-request.php b/includes/functions-request.php index ba82738b23..4e782a6dcd 100644 --- a/includes/functions-request.php +++ b/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. * diff --git a/includes/rest/trait-verification.php b/includes/rest/trait-verification.php index 32a0b770d1..b891a3640e 100644 --- a/includes/rest/trait-verification.php +++ b/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 ) ); } diff --git a/includes/signature/class-http-message-signature.php b/includes/signature/class-http-message-signature.php index b8b8032c5b..aa2a6394ab 100644 --- a/includes/signature/class-http-message-signature.php +++ b/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; } /** diff --git a/includes/signature/class-http-signature-draft.php b/includes/signature/class-http-signature-draft.php index 3bdb62a804..2e9be8d841 100644 --- a/includes/signature/class-http-signature-draft.php +++ b/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; } /** diff --git a/includes/signature/interface-http-signature.php b/includes/signature/interface-http-signature.php index 760ff2e69b..7dcf3f9483 100644 --- a/includes/signature/interface-http-signature.php +++ b/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 ); diff --git a/tests/phpunit/tests/includes/class-test-signature.php b/tests/phpunit/tests/includes/class-test-signature.php index fe10cafd1c..eab4c675f4 100644 --- a/tests/phpunit/tests/includes/class-test-signature.php +++ b/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 ); } diff --git a/tests/phpunit/tests/includes/collection/class-test-remote-actors.php b/tests/phpunit/tests/includes/collection/class-test-remote-actors.php index c7cb1504ba..64bdead366 100644 --- a/tests/phpunit/tests/includes/collection/class-test-remote-actors.php +++ b/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. * diff --git a/tests/phpunit/tests/includes/rest/class-test-trait-verification.php b/tests/phpunit/tests/includes/rest/class-test-trait-verification.php index 835a8fcc90..deffaf5569 100644 --- a/tests/phpunit/tests/includes/rest/class-test-trait-verification.php +++ b/tests/phpunit/tests/includes/rest/class-test-trait-verification.php @@ -659,85 +659,77 @@ public function test_verify_owner_blog_actor_filter_revokes_admin_access() { /** * Data provider for verify_key_id tests. * - * @return array[] Test cases: [ verified_key_id, actor, expected_pass ]. + * @return array[] Test cases: [ verified key owner, actor, expected_pass ]. */ public function data_verify_key_id() { return array( - 'matching hosts' => array( - 'https://remote.example/users/alice#main-key', + 'own key' => array( + 'https://remote.example/users/alice', 'https://remote.example/users/alice', true, ), - 'mismatched hosts' => array( - 'https://evil.example/users/alice#main-key', + // A user on the same server who holds their own key must not sign for another user. + 'key of another actor' => array( + 'https://remote.example/users/mallory', + 'https://remote.example/users/alice', + false, + ), + 'key on another host' => array( + 'https://evil.example/users/alice', 'https://remote.example/users/alice', false, ), 'no actor in body' => array( - 'https://remote.example/users/alice#main-key', + 'https://remote.example/users/alice', null, true, ), - // A missing keyId cannot be bound to the actor, so it must fail closed. - 'no verified key id' => array( + // No owner cannot be bound to the actor, so it must fail closed. + 'no key owner' => array( null, 'https://remote.example/users/alice', false, ), - // A non-URL `acct:` keyId resolves to a real key but yields no host: the bypass this guards. - 'acct key id cannot bind' => array( - 'acct:mallory@attacker.example', - 'https://victim.example/users/alice', + 'empty key owner' => array( + '', + 'https://remote.example/users/alice', false, ), - // An `acct:` actor likewise has no host and cannot be bound. 'acct actor cannot bind' => array( - 'https://remote.example/users/alice#main-key', + 'https://remote.example/users/alice', 'acct:alice@remote.example', false, ), - // The FeatureRequest fill sets actor = strip_fragment( keyId ); a hostless `acct:` - // keyId makes both sides hostless, which must still fail closed rather than self-match. - 'acct key id and actor' => array( + // A hostless owner must not bind by matching a hostless actor, as the FeatureRequest fill could produce. + 'acct owner and actor' => array( 'acct:mallory@attacker.example', 'acct:mallory@attacker.example', false, ), - // An empty keyId string has no host and cannot be bound. - 'empty key id' => array( - '', - 'https://remote.example/users/alice', - false, - ), 'actor as object with id' => array( - 'https://remote.example/users/alice#main-key', + 'https://remote.example/users/alice', array( 'id' => 'https://remote.example/users/alice' ), true, ), 'actor object mismatch' => array( - 'https://evil.example/users/alice#main-key', + 'https://remote.example/users/mallory', array( 'id' => 'https://remote.example/users/alice' ), false, ), - 'case-insensitive hosts' => array( - 'https://Remote.Example/users/alice#main-key', - 'https://remote.example/users/alice', - true, - ), ); } /** - * Test that verify_key_id checks the verified keyId's host against the actor host. + * Test that verify_key_id requires the verified key's owner to be the actor. * * @dataProvider data_verify_key_id * @covers ::verify_key_id * - * @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. * @param string|array|null $actor The actor value in the JSON body. * @param bool $should_pass Whether the check should pass. */ - public function test_verify_key_id( $key_id, $actor, $should_pass ) { + public function test_verify_key_id( $owner, $actor, $should_pass ) { $request = new \WP_REST_Request( 'POST', '/activitypub/1.0/inbox' ); $body = array( @@ -758,7 +750,7 @@ public function test_verify_key_id( $key_id, $actor, $should_pass ) { if ( \PHP_VERSION_ID < 80100 ) { $method->setAccessible( true ); } - $result = $method->invoke( $this->instance, $request, $key_id ); + $result = $method->invoke( $this->instance, $request, $owner ); if ( $should_pass ) { $this->assertTrue( $result ); From 7453dfc3765bb5e90518abcd62cb76b2e1016051 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Wed, 28 Oct 2026 10:15:54 +0100 Subject: [PATCH 2/4] Revert the documentation changes --- .agents/skills/code-style/SKILL.md | 8 ++------ .claude/agents/security-audit.md | 3 +-- AGENTS.md | 2 -- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/.agents/skills/code-style/SKILL.md b/.agents/skills/code-style/SKILL.md index b46671c84e..5aeb9dcb4d 100644 --- a/.agents/skills/code-style/SKILL.md +++ b/.agents/skills/code-style/SKILL.md @@ -154,12 +154,8 @@ Expose ActivityPub endpoints. ## Plugin-Specific Helper Functions ```php -// Fetch a remote actor or object document from its own host (cached; pass array( 'cached' => false ) for a fresh copy). -$actor = Proxy::get( $actor_url ); - -// Not get_remote_metadata_by_actor(): it returns the stored ap_actor record, which can be stale. -// An incoming Update writes its embedded actor into that record; it only stays the actor's own -// data because the signing key's owner and the object's id both have to be the actor. +// Get remote actor metadata. +$metadata = get_remote_metadata_by_actor( $actor_url ); // Convert ActivityPub object to URI string. $uri = object_to_uri( $object ); diff --git a/.claude/agents/security-audit.md b/.claude/agents/security-audit.md index a2f9c24351..7eb8014daa 100644 --- a/.claude/agents/security-audit.md +++ b/.claude/agents/security-audit.md @@ -40,7 +40,6 @@ Past CVEs and security fixes inform what patterns to watch for. The full list is 17. **Canonicalization decides access: host folding, Unicode-class regexes and percent-decoding** (2026) — patterns to enforce, stated generally so the audit finds the whole class. When two identifiers are compared, or one is decoded before a lookup, the canonical form *is* the gate. (a) **The folded host must be the host that is contacted.** `fold_host()`/`fold_authority()` run IDNA UTS #46, which also *maps* code points: soft hyphen and zero-width characters vanish, fullwidth and compatibility letters become ASCII (`example.com` folds to `example.com`). WordPress' HTTP layer punycodes the unmapped string instead (`WpOrg\Requests\IdnaEncoder`), so a folded value can name a different DNS host than a fetch of the same string. Today core's `wp_http_validate_url()` refuses non-ASCII hosts (`gethostbyname()` fails), so these URLs are never fetched; any path that A-labels before fetching, or fetches outside the WP HTTP API, changes that. Fold both sides of a gate, never one, and flag a neighbouring gate that still compares raw hosts. (b) **IDNA output can contain characters the input did not:** fullwidth `/`, `@`, `:`, `?`, `#` map to their ASCII forms, so a folded host is not safe to put into a URL until it is checked to be a bare host. (c) **A regex used as a validator is only as strict as its modifiers.** `\p{L}`/`\p{M}`/`\p{N}` without `u` match single *bytes*, which is laxer than the ASCII class they replace; with `u`, `preg_*` returns `false`/`NULL` on invalid UTF-8, so callers need a fail-closed branch, and a `NULL` replacement must never be appended back into content. A documented pattern constant (`ACTIVITYPUB_USERNAME_REGEXP`) must state its modifiers, and every in-repo caller must use them. (d) **A decode widens the input of a lookup.** The REST layer already decodes query parameters once, so after a second `rawurldecode()` magic values (`*`, `_`, the empty string) and encoded spellings of stored identifiers are reachable. Decode the identifier, never a host a gate already compared, and keep an empty host an explicit rejection: it must not match an unset stored host. See Audit Scope §4, §6 and §8. 18. **An ownership check is only as strong as its fallback and its lookup** (2026) — patterns to enforce, stated generally so the audit finds the whole class. History #7 added actor↔owner checks to the Update and Undo paths, and their fallbacks left records open. (a) **A backward-compatibility branch is a gate of its own.** "No owner recorded, so let it through" for rows written before the owner was stored must still bind the actor to something, at least `is_same_host( actor, )`. A missing owner never means "anyone". (b) **A recorded reference that no longer resolves fails closed.** An owner ID whose post was deleted or merged must not switch the comparison off: a guard shaped `if ( $owner instanceof WP_Post && … )` skips the check exactly when the owner is gone. And every path that deletes or merges a record an authorization depends on (actor Delete, Move, scheduled cleanup) has to repoint or remove the references to it in the same step. (c) **Check the value you look up by.** `object_to_uri()` dispatches by type (it returns `url` for `Image`, `Audio`, `Video` and `Document`), so a gate on `object_to_uri( $object )` and a lookup on `$object['id']` can be handed two different identifiers by the sender. Derive the identifier once and use it for the lookup, the host check and the stored key, as `add_comment()` does. See Audit Scope §4 and §8. 19. **Two signature verifiers, one rule** (2026) — the requester chooses the verifier: a `Signature-Input` header selects RFC 9421, otherwise draft-cavage. A binding is only in place once both verifiers enforce it. In both, the signer also picks what is signed, so a signature over `(request-target) date` or `("@method")` verifies on its own terms. Each verifier must require a component that names this site (draft: `host`; RFC 9421: `@target-uri`, `@authority` or `host`; each checked by `is_signature_host()`) and, with a body, a signed digest (`digest` / `content-digest`) whose header is present. A signed but absent header enters the signed data empty and vouches for any body. See Audit Scope §3. -20. **A same-host key is not the actor's key, and the stored actor is not the actor's document** (2026) — `verify_key_id()` bound the signing key to the actor by host only. On a server where users hold their own keys (Friendica lets every user export theirs), user A could sign an Update that claims to be user B on the same host. `Update::update_actor()` then wrote A's key and inbox into B's stored `ap_actor` record, and from then on A's signatures verified as B while B's own failed. Two patterns: (a) the key's owner must be the activity actor, not just share its host (the same rule as Mastodon's `different_actor?`). Take the owner from where the key was resolved: the stored actor the keyId points to, else the key as its host serves it (`Proxy::get()`). Compare the owner, not the keyId string, so same-host key aliases keep working. (b) The stored `ap_actor` record is written from the actor embedded in incoming Updates. Like Mastodon's stored keys, it is only the actor's own data because of (a), and (a) reads from it. So any path that writes a remote actor, its key or its inbox must be bound to the signer the same way, and a check that relaxes (a) poisons the store. See Audit Scope §4. ## Audit Scope @@ -132,7 +131,7 @@ Files: `includes/class-signature.php`, `includes/signature/class-http-signature- Files: `includes/rest/class-inbox-controller.php`, `includes/rest/class-actors-inbox-controller.php`, `includes/handler/` -- Check that the signature's `keyId` belongs to the `actor`, not only to its host: the key's owner (the stored actor the keyId resolves to, or the key as its host serves it through `Proxy::get()`) has to be the actor. On a server where users hold their own keys, a same-host check lets one user sign for another (see history #20). +- Check that `actor` field is validated against the signature's `keyId` origin - Every host-equality check on this surface (keyId ↔ actor, and any `host(ref) === host(actor)` binding) must fail **closed** when a side has no parsable host — non-`http(s)` identifiers parse to a `null` host. Use `is_same_host()` (fails closed); flag any bespoke `wp_parse_url` comparison that passes when a side is empty. Bind **every** remote-supplied reference a handler acts on to the actor's host, including activity-specific reference fields, not only `object` / `attributedTo`. Beware a binding made meaningless because the compared value was auto-derived from the signing key (see history #13). - Every ownership check on an Update/Undo/Delete path: read the branch for a missing or unresolvable owner, and confirm the lookup, the host check and the stored key use one identifier (see history #18). - Every host-equality gate folds **both** sides with `fold_host()`/`fold_authority()`, and the folded value must name the host the plugin would connect to (see history #17): a fold that maps a non-ASCII host onto an unrelated ASCII name can make an attacker's spelling compare equal to a victim host. diff --git a/AGENTS.md b/AGENTS.md index 7c089a8783..c07148a637 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,8 +28,6 @@ Prefer reading project files and `docs/` over relying on training data for WordP **ALWAYS reuse existing functionality instead of hand-rolling it.** Before writing any parsing, validation, escaping, or formatting logic, check for a WordPress core function that does the job (e.g. `wp_kses_bad_protocol()`, `wp_parse_url()`, `wp_http_validate_url()`), then for an existing plugin helper (`includes/functions*.php`, `Sanitize`, `src/shared/`). Core primitives are battle-tested against adversarial input in ways a fresh implementation is not, and reviewers can trust them on sight. A custom implementation needs a stated reason why no existing function fits — "shorter" is not one. This applies to refactors too: never replace a core-function-based check with bespoke logic just to save a line. -**Fetch remote documents with `Proxy::get()`.** Do not use `get_remote_metadata_by_actor()` in new code. It returns the stored `ap_actor` record first and only fetches when none is stored, so it can be stale, and it ignores `$cached`. 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 (`verify_key_id()`), and `Update::update_actor()` requires the object's id to be the actor. Never relax either, and give any new writer that stores remote-supplied data the same two checks. `Proxy::get()` is only filled from the actor's own host; pass `array( 'cached' => false )` for a fresh copy. - **MUST** backslash-prefix all WordPress functions in namespaced code: `\get_option()`, `\add_action()`, `\apply_filters()`, `\__()`, `\_e()`, etc. PHP falls back to global scope, but backslashes are a project standard for consistency and to avoid accidentally shadowing globals. **No inline namespaces.** Use `use` statements at the top of the file instead of inline fully-qualified class names (e.g., `use Activitypub\Options;` then `Options::method()`, not `\Activitypub\Options::method()`). From e2ca4c2bfb6016c94c2275eef11e2364ddfc25d9 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Wed, 28 Oct 2026 10:25:47 +0100 Subject: [PATCH 3/4] Test that a key signs only for its own actor --- .../rest/class-test-trait-verification.php | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/phpunit/tests/includes/rest/class-test-trait-verification.php b/tests/phpunit/tests/includes/rest/class-test-trait-verification.php index deffaf5569..1f0b96f08d 100644 --- a/tests/phpunit/tests/includes/rest/class-test-trait-verification.php +++ b/tests/phpunit/tests/includes/rest/class-test-trait-verification.php @@ -228,6 +228,98 @@ public function redirected_signing_key_provider() { ); } + /** + * A key signs only for its own actor, not for another actor on the same host. + * + * @dataProvider signing_key_owner_provider + * @covers ::verify_signature + * + * @param string $format Signature format. + * @param string $actor The actor the activity claims, on the signer's host. + * @param bool $passes Whether the signature has to be accepted. + */ + public function test_verify_signature_binds_the_key_to_its_actor( $format, $actor, $passes ) { + $this->stub_remote_requests(); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + $fixtures = \json_decode( \file_get_contents( AP_TESTS_DIR . '/data/fixtures/http-signature-keys.json' ), true )['rsa']; + $keys = $fixtures['2048']; + $signer = 'https://example.com/users/mallory'; + $this->responses[ $signer ] = array( + 'id' => $signer, + 'type' => 'Person', + 'publicKey' => array( + 'id' => $signer . '#main-key', + 'owner' => $signer, + 'publicKeyPem' => $keys['public_key'], + ), + ); + $this->responses['https://example.com/users/alice'] = array( + 'id' => 'https://example.com/users/alice', + 'type' => 'Person', + 'publicKey' => array( + 'id' => 'https://example.com/users/alice#main-key', + 'owner' => 'https://example.com/users/alice', + 'publicKeyPem' => $fixtures['4096']['public_key'], + ), + ); + + $route = '/' . ACTIVITYPUB_REST_NAMESPACE . '/inbox'; + $url = \set_url_scheme( 'http://' . \wp_parse_url( \home_url(), PHP_URL_HOST ) . \wp_parse_url( \home_url( '/' . \rest_get_url_prefix() . $route ), PHP_URL_PATH ) ); + $method = 'draft' === $format ? new Http_Signature_Draft() : new Http_Message_Signature(); + $args = $method->sign( + array( + 'method' => 'POST', + 'body' => \wp_json_encode( + array( + 'id' => 'https://example.com/activities/1', + 'type' => 'Update', + 'actor' => $actor, + 'object' => array( 'id' => $actor ), + ) + ), + 'headers' => array( + 'Host' => \wp_parse_url( $url, PHP_URL_HOST ), + 'Date' => \gmdate( 'D, d M Y H:i:s T' ), + 'Content-Type' => 'application/activity+json', + ), + 'key_id' => $signer . '#main-key', + 'private_key' => \openssl_pkey_get_private( $keys['private_key'] ), + ), + $url + ); + + $_SERVER['REQUEST_URI'] = \wp_parse_url( $url, PHP_URL_PATH ); + $_SERVER['REQUEST_METHOD'] = 'POST'; + $_SERVER['HTTP_HOST'] = \wp_parse_url( $url, PHP_URL_HOST ); + $request = new \WP_REST_Request( 'POST', $route ); + $request->set_headers( $args['headers'] ); + $request->set_body( $args['body'] ); + + $result = $this->instance->verify_signature( $request ); + + if ( $passes ) { + $this->assertTrue( $result ); + } else { + $this->assertWPError( $result ); + $this->assertSame( 'activitypub_key_actor_mismatch', $result->get_error_code() ); + $this->assertSame( 403, $result->get_error_data()['status'] ); + } + } + + /** + * Signer and claimed actor on the same host, in both signature formats. + * + * @return array Test cases. + */ + public function signing_key_owner_provider() { + return array( + 'draft own actor' => array( 'draft', 'https://example.com/users/mallory', true ), + 'draft other actor' => array( 'draft', 'https://example.com/users/alice', false ), + 'rfc9421 own actor' => array( 'rfc9421', 'https://example.com/users/mallory', true ), + 'rfc9421 other actor' => array( 'rfc9421', 'https://example.com/users/alice', false ), + ); + } + /** * Test defer filter bypasses signature verification. * From 1dc22350af1a23e6229c3fc9378028212e7d575c Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Wed, 28 Oct 2026 10:33:19 +0100 Subject: [PATCH 4/4] Serve the actor without the key fragment in the inbox test --- .../tests/includes/rest/class-test-actors-inbox-controller.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php b/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php index eff15927a6..8794f13bcf 100644 --- a/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php +++ b/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,