Prevent actor URL rebinding

packages/tests/src/api/activitypub/actor-identity.ts

@@ -0,0 +1,213 @@
+/* oxlint-disable @typescript-eslint/no-unused-expressions,@typescript-eslint/require-await */
+
+import { buildAbsoluteFixturePath } from '@peertube/peertube-node-utils'
+import {
+  PeerTubeServer,
+  cleanupTests,
+  createMultipleServers,
+  makeActivityPubGetRequest,
+  setAccessTokensToServers,
+  waitJobs
+} from '@peertube/peertube-server-commands'
+import {
+  activityPubContextify,
+  buildGlobalHTTPHeaders,
+  getAPPublicValue
+} from '@peertube/peertube-server/core/helpers/activity-pub-utils.js'
+import { buildDigest } from '@peertube/peertube-server/core/helpers/peertube-crypto.js'
+import { ACTIVITY_PUB, HTTP_SIGNATURE } from '@peertube/peertube-server/core/initializers/constants.js'
+import { makePOSTAPRequest } from '@tests/shared/requests.js'
+import { SQLCommand } from '@tests/shared/sql-command.js'
+import { expect } from 'chai'
+import { readJsonSync } from 'fs-extra/esm'
+
+function fakeFilter () {
+  return (data: any) => Promise.resolve(data)
+}
+
+function setKeysOfServer (onServer: SQLCommand, ofServerUrl: string, publicKey: string, privateKey: string) {
+  const url = ofServerUrl + '/accounts/peertube'
+
+  return Promise.all([
+    onServer.setActorField(url, 'publicKey', publicKey),
+    onServer.setActorField(url, 'privateKey', privateKey)
+  ])
+}
+
+describe('Test ActivityPub actor identity binding', function () {
+  let servers: PeerTubeServer[]
+  let sqlCommands: SQLCommand[] = []
+
+  let inboxUrl: string
+  let remoteActorUrl: string
+  let remoteActorObject: any
+
+  const keys = readJsonSync(buildAbsoluteFixturePath('./ap-json/peertube/keys.json'))
+
+  function buildHttpSignature () {
+    return {
+      keyId: remoteActorUrl,
+      key: keys.privateKey,
+      headers: HTTP_SIGNATURE.HEADERS_TO_SIGN_WITH_PAYLOAD
+    }
+  }
+
+  // Send an "Update" of the remote actor, signed by that same remote actor
+  async function sendActorUpdate (actorObject: any) {
+    const activity = {
+      type: 'Update',
+      id: remoteActorUrl + '/updates/' + new Date().toISOString(),
+      actor: remoteActorUrl,
+      to: [ getAPPublicValue() ],
+      object: actorObject
+    }
+
+    const body = await activityPubContextify(activity, 'Actor', fakeFilter())
+    const headers = {
+      ...buildGlobalHTTPHeaders(body, buildDigest),
+
+      'content-type': 'application/activity+json',
+      'accept': ACTIVITY_PUB.ACCEPT_HEADER
+    }
+
+    try {
+      const { statusCode } = await makePOSTAPRequest(inboxUrl, body, buildHttpSignature(), headers)
+
+      return { rejected: false, statusCode }
+    } catch (err) {
+      return { rejected: true, statusCode: err.statusCode as number }
+    }
+  }
+
+  // Load the cached actor of servers[1] on servers[0] using its server association, so we detect a rebound URL
+  async function getCachedRemoteActor () {
+    const query = 'SELECT a."url", a."publicKey", a."serverId" FROM "actor" a ' +
+      'INNER JOIN "server" s ON s."id" = a."serverId" ' +
+      'WHERE s."host" = :host AND a."preferredUsername" = :preferredUsername'
+
+    const [ row ] = await sqlCommands[0].selectQuery<{ url: string, publicKey: string, serverId: number }>(query, {
+      host: servers[1].host,
+      preferredUsername: 'peertube'
+    })
+
+    return row
+  }
+
+  before(async function () {
+    this.timeout(120000)
+
+    servers = await createMultipleServers(2)
+    await setAccessTokensToServers(servers)
+
+    sqlCommands = servers.map(s => new SQLCommand(s))
+
+    inboxUrl = servers[0].url + '/inbox'
+    remoteActorUrl = servers[1].url + '/accounts/peertube'
+
+    // Use a known key pair so we can sign activities on behalf of the servers[1] instance actor.
+    // It must be done before servers[0] fetches and caches that actor
+    await setKeysOfServer(sqlCommands[1], servers[1].url, keys.publicKey, keys.privateKey)
+
+    await servers[0].follows.follow({ hosts: [ servers[1].url ] })
+    await waitJobs(servers)
+
+    const { body } = await makeActivityPubGetRequest(servers[1].url, '/accounts/peertube')
+    remoteActorObject = body
+  })
+
+  it('Should have cached the remote actor', async function () {
+    const actor = await getCachedRemoteActor()
+
+    expect(actor).to.exist
+    expect(actor.url).to.equal(remoteActorUrl)
+    expect(actor.publicKey).to.equal(keys.publicKey)
+    expect(actor.serverId).to.not.be.null
+  })
+
+  it('Should not rebind the actor URL/public key to our own host', async function () {
+    this.timeout(30000)
+
+    const forgedUrl = servers[0].url + '/accounts/hijacked'
+    const otherKeys = readJsonSync(buildAbsoluteFixturePath('./ap-json/peertube/invalid-keys.json'))
+
+    await sendActorUpdate({
+      ...remoteActorObject,
+
+      id: forgedUrl,
+      publicKey: { id: forgedUrl + '#main-key', owner: forgedUrl, publicKeyPem: otherKeys.publicKey }
+    })
+
+    await waitJobs(servers)
+
+    const actor = await getCachedRemoteActor()
+    expect(actor.url).to.equal(remoteActorUrl)
+    expect(actor.publicKey).to.equal(keys.publicKey)
+    expect(actor.serverId).to.not.be.null
+  })
+
+  it('Should not update another actor of the same host', async function () {
+    this.timeout(30000)
+
+    const otherUrl = servers[1].url + '/accounts/root'
+
+    await sendActorUpdate({
+      ...remoteActorObject,
+
+      id: otherUrl,
+      publicKey: { id: otherUrl + '#main-key', owner: otherUrl, publicKeyPem: keys.publicKey }
+    })
+
+    await waitJobs(servers)
+
+    const actor = await getCachedRemoteActor()
+    expect(actor.url).to.equal(remoteActorUrl)
+  })
+
+  it('Should not accept an actor whose public key is owned by another identity', async function () {
+    this.timeout(30000)
+
+    const otherKeys = readJsonSync(buildAbsoluteFixturePath('./ap-json/peertube/invalid-keys.json'))
+
+    await sendActorUpdate({
+      ...remoteActorObject,
+
+      publicKey: {
+        id: remoteActorUrl + '#main-key',
+        owner: servers[0].url + '/accounts/peertube',
+        publicKeyPem: otherKeys.publicKey
+      }
+    })
+
+    await waitJobs(servers)
+
+    const actor = await getCachedRemoteActor()
+    expect(actor.url).to.equal(remoteActorUrl)
+    expect(actor.publicKey).to.equal(keys.publicKey)
+  })
+
+  it('Should still accept a valid actor update', async function () {
+    this.timeout(30000)
+
+    await sendActorUpdate({
+      ...remoteActorObject,
+
+      name: 'updated display name'
+    })
+
+    await waitJobs(servers)
+
+    const account = await servers[0].accounts.get({ accountName: 'peertube@' + servers[1].host })
+    expect(account.displayName).to.equal('updated display name')
+
+    const actor = await getCachedRemoteActor()
+    expect(actor.url).to.equal(remoteActorUrl)
+  })
+
+  after(async function () {
+    for (const sqlCommand of sqlCommands) {
+      await sqlCommand.cleanup()
+    }
+
+    await cleanupTests(servers)
+  })
+})

server/core/helpers/custom-validators/activitypub/actor.ts

@@ -75,6 +75,7 @@ export function sanitizeAndCheckActorObject (actor: ActivityPubActor) {
     isActivityPubUrlValid(actor.inbox) &&
     isActorPreferredUsernameValid(actor.preferredUsername) &&
     isActorPublicKeyObjectValid(actor.publicKey) &&
+    actor.publicKey.owner === actor.id &&
     isActorEndpointsObjectValid(actor.endpoints) &&
     (!actor.outbox || isActivityPubUrlValid(actor.outbox)) &&
     (!actor.following || isActivityPubUrlValid(actor.following)) &&

server/core/lib/activitypub/actors/shared/creator.ts

@@ -9,6 +9,7 @@ import { VideoChannelModel } from '@server/models/video/video-channel.js'
 import { MAccount, MActor, MActorFull, MActorFullActor, MActorImages, MChannel, MServer } from '@server/types/models/index.js'
 import { Transaction } from 'sequelize'
 import { upsertAPPlayerSettings } from '../../player-settings.js'
+import { isLocalUrl } from '../../url.js'
 import { updateActorImages } from '../image.js'
 import { getActorAttributesFromObject, getActorDisplayNameFromObject, getImagesInfoFromObject } from './object-to-model-attributes.js'
 import { fetchActorFollowsCount } from './url-to-object.js'
@@ -26,6 +27,11 @@ export class APActorCreator {
   async create (): Promise<MActorFull> {
     logger.debug('Creating remote actor from object', { actorObject: this.actorObject, ...this.lTags() })
 
+    // A remote actor must never be registered with our own host
+    if (isLocalUrl(this.actorObject.id)) {
+      throw new Error(`Cannot create remote actor ${this.actorObject.id} with a local URL`)
+    }
+
     const { followersCount, followingCount } = await fetchActorFollowsCount(this.actorObject)
 
     const actor = await sequelizeTypescript.transaction(async t => {

server/core/lib/activitypub/actors/updater.ts

@@ -6,6 +6,7 @@ import { VideoChannelModel } from '@server/models/video/video-channel.js'
 import { VideoPlaylistModel } from '@server/models/video/video-playlist.js'
 import { MAccount, MActor, MActorFull, MChannel } from '@server/types/models/index.js'
 import { upsertAPPlayerSettings } from '../player-settings.js'
+import { checkUrlsSameHost, isLocalUrl } from '../url.js'
 import { getOrCreateAPOwner } from './get.js'
 import { updateActorImages } from './image.js'
 import { fetchActorFollowsCount } from './shared/index.js'
@@ -23,6 +24,8 @@ export class APActorUpdater {
   }
 
   async update () {
+    this.checkActorIdentityBindingOrThrow()
+
     const avatarsInfo = getImagesInfoFromObject(this.actorObject, ActorImageType.AVATAR)
     const bannersInfo = getImagesInfoFromObject(this.actorObject, ActorImageType.BANNER)
 
@@ -101,6 +104,25 @@ export class APActorUpdater {
     }
   }
 
+  // An actor can only update itself: its new AP id must stay on the host it is already associated to
+  private checkActorIdentityBindingOrThrow () {
+    const { id, publicKey } = this.actorObject
+    const currentUrl = this.actor.url
+
+    if (!checkUrlsSameHost(currentUrl, id)) {
+      throw new Error(`Actor ${currentUrl} cannot be updated with object id ${id} that is not on the same host`)
+    }
+
+    // A remote actor must never claim our own host
+    if (!this.actor.isLocal() && isLocalUrl(id)) {
+      throw new Error(`Remote actor ${currentUrl} cannot be updated with local URL ${id}`)
+    }
+
+    if (publicKey.owner !== id) {
+      throw new Error(`Public key owner ${publicKey.owner} of actor ${id} does not match the actor id`)
+    }
+  }
+
   private async updateActorInstance (actorInstance: MActor, actorObject: ActivityPubActor) {
     const { followersCount, followingCount } = await fetchActorFollowsCount(actorObject)
 

server/core/lib/activitypub/player-settings.ts

@@ -3,7 +3,7 @@ import { sanitizeAndCheckPlayerSettingsObject } from '@server/helpers/custom-val
 import { MChannelDefault, MVideoIdUrl } from '../../types/models/index.js'
 import { upsertPlayerSettings } from '../player-settings.js'
 import { fetchAPObjectIfNeeded } from './activity.js'
-import { checkUrlsSameHost } from './url.js'
+import { checkUrlsSameHost, isLocalUrl } from './url.js'
 
 export async function upsertAPPlayerSettings (options: {
   video: MVideoIdUrl
@@ -21,6 +21,11 @@ export async function upsertAPPlayerSettings (options: {
     throw new Error(`Player settings ${settingsObject.id} object is not valid`)
   }
 
+  // Federation must never update the player settings of a video/channel we own
+  if (isLocalUrl(settingsObject.id)) {
+    throw new Error(`Cannot update local player settings ${settingsObject.id} from a remote actor`)
+  }
+
   if (!checkUrlsSameHost(settingsObject.id, contextUrl)) {
     throw new Error(`Player settings ${settingsObject.id} object is not on the same host as context URL ${contextUrl}`)
   }

server/core/lib/activitypub/playlists/create-update.ts

@@ -15,7 +15,7 @@ import Bluebird from 'bluebird'
 import { getAPId } from '../activity.js'
 import { getOrCreateAPActor } from '../actors/index.js'
 import { crawlCollectionPage } from '../crawl.js'
-import { checkUrlsSameHost } from '../url.js'
+import { checkUrlsSameHost, isLocalUrl } from '../url.js'
 import { getOrCreateAPVideo } from '../videos/index.js'
 import {
   fetchRemotePlaylistElement,
@@ -64,6 +64,11 @@ export async function createOrUpdateVideoPlaylist (options: {
 }) {
   const { playlistObject, contextUrl, to } = options
 
+  // Federation must never create or update a playlist we own
+  if (isLocalUrl(playlistObject.id)) {
+    throw new Error(`Cannot create or update local playlist ${playlistObject.id} from a remote actor`)
+  }
+
   if (!checkUrlsSameHost(playlistObject.id, contextUrl)) {
     throw new Error(`Playlist ${playlistObject.id} is not on the same host as context URL ${contextUrl}`)
   }

server/core/lib/activitypub/process/process-update.ts

@@ -19,7 +19,7 @@ import { sequelizeTypescript } from '../../../initializers/database.js'
 import { ActorModel } from '../../../models/actor/actor.js'
 import { APProcessorOptions } from '../../../types/activitypub-processor.model.js'
 import { MActorFull, MActorSignature } from '../../../types/models/index.js'
-import { fetchAPObjectIfNeeded } from '../activity.js'
+import { fetchAPObjectIfNeeded, getAPId } from '../activity.js'
 import { getOrCreateAPActor } from '../actors/get.js'
 import { APActorUpdater } from '../actors/updater.js'
 import { createOrUpdateCacheFile } from '../cache-file.js'
@@ -27,7 +27,7 @@ import { upsertAPPlayerSettings } from '../player-settings.js'
 import { createOrUpdateVideoPlaylist } from '../playlists/index.js'
 import { forwardVideoRelatedActivity } from '../send/shared/send-utils.js'
 import { APVideoUpdater, canVideoBeFederated, getOrCreateAPVideo, maybeGetOrCreateAPVideo } from '../videos/index.js'
-import { checkUrlsSameHost } from '../url.js'
+import { checkUrlsSameHost, isLocalUrl } from '../url.js'
 
 async function processUpdateActivity (options: APProcessorOptions<ActivityUpdate<ActivityUpdateObject>>) {
   const { activity, byActor } = options
@@ -40,6 +40,13 @@ async function processUpdateActivity (options: APProcessorOptions<ActivityUpdate
   }
 
   if (isActorTypeValid(objectType as ActivityPubActorType)) {
+    // An actor can only update itself: the object id must be the actor that signed the activity
+    const actorObjectId = getAPId(object as ActivityPubActor)
+    if (actorObjectId !== byActor.url) {
+      logger.error(`Actor ${byActor.url} cannot update the actor ${actorObjectId}.`, { actorObjectId, byActor: byActor.url })
+      return undefined
+    }
+
     // We need more attributes
     const byActorFull = await ActorModel.loadByUrlAndPopulateAccountAndChannel(byActor.url)
     return retryTransactionWrapper(processUpdateActor, byActorFull, object)
@@ -73,6 +80,12 @@ export {
 async function processUpdateVideo (byActor: MActorSignature, activity: ActivityUpdate<VideoObject | string>) {
   const videoObject = activity.object as VideoObject
 
+  // Federation must never update a video we own
+  if (isLocalUrl(videoObject.id)) {
+    logger.warn('Video sent by update is a local video.', { videoObject, byActor })
+    return undefined
+  }
+
   if (!checkUrlsSameHost(byActor.url, videoObject.id)) {
     logger.warn('Video sent by update is not from the same host as the actor.', { videoObject, byActor })
     return undefined

server/core/lib/activitypub/url.ts

@@ -163,3 +163,11 @@ export function checkUrlsSameHost (url1: string, url2: string) {
 
   return idHost?.toLowerCase() === actorHost?.toLowerCase()
 }
+
+export function isLocalUrl (url: string) {
+  try {
+    return new URL(url).host?.toLowerCase() === WEBSERVER.HOST.toLowerCase()
+  } catch {
+    return false
+  }
+}

server/core/middlewares/validators/activitypub/activity.ts

@@ -1,5 +1,6 @@
 import express from 'express'
 import { HttpStatusCode } from '@peertube/peertube-models'
+import { isLocalUrl } from '@server/lib/activitypub/url.js'
 import { getServerActor } from '@server/models/application/application.js'
 import { isRootActivityValid } from '../../../helpers/custom-validators/activitypub/activity.js'
 import { logger } from '../../../helpers/logger.js'
@@ -14,7 +15,10 @@ async function activityPubValidator (req: express.Request, res: express.Response
 
   const serverActor = await getServerActor()
   const remoteActor = res.locals.signature.actor
-  if (serverActor.id === remoteActor.id || remoteActor.serverId === null) {
+
+  // Also check the actor URL and not only the server association
+  // A remote actor row that carries our own host is inconsistent
+  if (serverActor.id === remoteActor.id || remoteActor.serverId === null || isLocalUrl(remoteActor.url)) {
     logger.error('Receiving request in INBOX by ourselves!', req.body)
     return res.status(HttpStatusCode.CONFLICT_409).end()
   }