Merge commit from fork

Fix critical authentication bypass vulnerability in inbox handler

Author: dahlia

CHANGES.md

@@ -8,6 +8,15 @@ Version 1.3.20
 
 To be released.
 
+ -  Fixed a critical authentication bypass vulnerability in the inbox handler
+    that allowed unauthenticated attackers to impersonate any ActivityPub actor.
+    The vulnerability occurred because activities were processed before
+    verifying that the HTTP Signatures key belonged to the claimed actor.
+    Now authentication verification is performed before activity processing to
+    prevent actor impersonation attacks.  [[CVE-2025-54888]]
+
+[CVE-2025-54888]: https://github.com/fedify-dev/fedify/security/advisories/GHSA-6jcc-xgcr-q3h4
+
 
 Version 1.3.19
 --------------

src/federation/handler.test.ts

@@ -35,6 +35,7 @@ import {
   respondWithObject,
   respondWithObjectIfAcceptable,
 } from "./handler.ts";
+import { InboxListenerSet } from "./inbox.ts";
 import { MemoryKvStore } from "./kv.ts";
 
 test("acceptsJsonLd()", () => {
@@ -1279,6 +1280,86 @@ test("respondWithObject()", async () => {
   });
 });
 
+test("handleInbox() - authentication bypass vulnerability", async () => {
+  // This test reproduces the authentication bypass vulnerability where
+  // activities are processed before verifying the signing key belongs
+  // to the claimed actor
+
+  let processedActivity: Create | undefined;
+  const inboxListeners = new InboxListenerSet<void>();
+  inboxListeners.add(Create, (_ctx, activity) => {
+    // Track that the malicious activity was processed
+    processedActivity = activity;
+  });
+
+  // Create malicious activity claiming to be from victim actor
+  const maliciousActivity = new Create({
+    id: new URL("https://attacker.example.com/activities/malicious"),
+    actor: new URL("https://victim.example.com/users/alice"), // Impersonating victim
+    object: new Note({
+      id: new URL("https://attacker.example.com/notes/forged"),
+      attribution: new URL("https://victim.example.com/users/alice"),
+      content: "This is a forged message from the victim!",
+    }),
+  });
+
+  // Sign request with attacker's key (not victim's key)
+  const maliciousRequest = await signRequest(
+    new Request("https://example.com/", {
+      method: "POST",
+      body: JSON.stringify(await maliciousActivity.toJsonLd()),
+    }),
+    rsaPrivateKey3, // Attacker's private key
+    rsaPublicKey3.id!, // Attacker's public key ID
+  );
+
+  const maliciousContext = createRequestContext({
+    request: maliciousRequest,
+    url: new URL(maliciousRequest.url),
+    data: undefined,
+    documentLoader: mockDocumentLoader,
+  });
+
+  const actorDispatcher: ActorDispatcher<void> = (_ctx, identifier) => {
+    if (identifier !== "someone") return null;
+    return new Person({ name: "Someone" });
+  };
+
+  const response = await handleInbox(maliciousRequest, {
+    recipient: "someone",
+    context: maliciousContext,
+    inboxContextFactory(_activity) {
+      return createInboxContext({ ...maliciousContext, recipient: "someone" });
+    },
+    kv: new MemoryKvStore(),
+    kvPrefixes: {
+      activityIdempotence: ["_fedify", "activityIdempotence"],
+      publicKey: ["_fedify", "publicKey"],
+    },
+    actorDispatcher,
+    inboxListeners,
+    onNotFound: () => new Response("Not found", { status: 404 }),
+    signatureTimeWindow: { minutes: 5 },
+    skipSignatureVerification: false,
+  });
+
+  // The vulnerability: Even though the response is 401 (unauthorized),
+  // the malicious activity was already processed by routeActivity()
+  assertEquals(response.status, 401);
+  assertEquals(await response.text(), "The signer and the actor do not match.");
+
+  assertEquals(
+    processedActivity,
+    undefined,
+    `SECURITY VULNERABILITY: Malicious activity with mismatched signature was processed! ` +
+      `Activity ID: ${processedActivity?.id?.href}, ` +
+      `Claimed actor: ${processedActivity?.actorId?.href}`,
+  );
+
+  // If we reach here, the vulnerability is fixed - activities with mismatched
+  // signatures are properly rejected before processing
+});
+
 test("respondWithObjectIfAcceptable", async () => {
   let request = new Request("https://example.com/", {
     headers: { Accept: "application/activity+json" },

src/federation/handler.ts

@@ -649,20 +649,6 @@ async function handleInboxInternal<TContextData>(
     span.setAttribute("activitypub.activity.id", activity.id.href);
   }
   span.setAttribute("activitypub.activity.type", getTypeId(activity).href);
-  const routeResult = await routeActivity({
-    context: ctx,
-    json,
-    activity,
-    recipient,
-    inboxListeners,
-    inboxContextFactory,
-    inboxErrorHandler,
-    kv,
-    kvPrefixes,
-    queue,
-    span,
-    tracerProvider,
-  });
   if (
     httpSigKey != null && !await doesActorOwnKey(activity, httpSigKey, ctx)
   ) {
@@ -685,6 +671,20 @@ async function handleInboxInternal<TContextData>(
       headers: { "Content-Type": "text/plain; charset=utf-8" },
     });
   }
+  const routeResult = await routeActivity({
+    context: ctx,
+    json,
+    activity,
+    recipient,
+    inboxListeners,
+    inboxContextFactory,
+    inboxErrorHandler,
+    kv,
+    kvPrefixes,
+    queue,
+    span,
+    tracerProvider,
+  });
   if (routeResult === "alreadyProcessed") {
     return new Response(
       `Activity <${activity.id}> has already been processed.`,