From 9cd27b412d75ab9cb26054aa85d0eca82d78552e Mon Sep 17 00:00:00 2001
From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com>
Date: Tue, 13 Aug 2024 15:37:09 +0000
Subject: [security] harden account update logic (#3198)

* on account update, ensure that public key has not changed

* change expected error message

* also support the case of changing account keys when expired (not waiting for handshake)

* tweak account update hardening logic, add tests for updating account with pubkey expired

* add check for whether incoming data was via federator, accepting keys if so

* use freshest window for federated account updates + comment about it
---
 internal/federation/dereferencing/account.go | 32 +++++++++++++++++-----------
 1 file changed, 19 insertions(+), 13 deletions(-)

(limited to 'internal/federation/dereferencing/account.go')

diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go
index b0118380d..a47284c34 100644
--- a/internal/federation/dereferencing/account.go
+++ b/internal/federation/dereferencing/account.go
@@ -341,7 +341,7 @@ func (d *Dereferencer) RefreshAccount(
 	)
 	if err != nil {
 		log.Errorf(ctx, "error enriching remote account: %v", err)
-		return nil, nil, gtserror.Newf("error enriching remote account: %w", err)
+		return nil, nil, gtserror.Newf("%w", err)
 	}
 
 	if accountable != nil {
@@ -600,7 +600,9 @@ func (d *Dereferencer) enrichAccount(
 	d.startHandshake(requestUser, uri)
 	defer d.stopHandshake(requestUser, uri)
 
-	if apubAcc == nil {
+	var resolve bool
+
+	if resolve = (apubAcc == nil); resolve {
 		// We were not given any (partial) ActivityPub
 		// version of this account as a parameter.
 		// Dereference latest version of the account.
@@ -732,16 +734,25 @@ func (d *Dereferencer) enrichAccount(
 		)...,
 	); err != nil {
 		return nil, nil, gtserror.Newf(
-			"error checking dereferenced account uri %s: %w",
+			"error checking account uri %s: %w",
 			latestAcc.URI, err,
 		)
 	} else if !matches {
 		return nil, nil, gtserror.Newf(
-			"dereferenced account uri %s does not match %s",
+			"account uri %s does not match %s",
 			latestAcc.URI, uri.String(),
 		)
 	}
 
+	// Get current time.
+	now := time.Now()
+
+	// Before expending any further serious compute, we need
+	// to ensure account keys haven't unexpectedly been changed.
+	if !verifyAccountKeysOnUpdate(account, latestAcc, now, !resolve) {
+		return nil, nil, gtserror.Newf("account %s pubkey has changed (key rotation required?)", uri)
+	}
+
 	/*
 		BY THIS POINT we have more or less a fullly-formed
 		representation of the target account, derived from
@@ -753,7 +764,8 @@ func (d *Dereferencer) enrichAccount(
 	// Ensure internal db ID is
 	// set and update fetch time.
 	latestAcc.ID = account.ID
-	latestAcc.FetchedAt = time.Now()
+	latestAcc.FetchedAt = now
+	latestAcc.UpdatedAt = now
 
 	// Ensure the account's avatar media is populated, passing in existing to check for chages.
 	if err := d.fetchAccountAvatar(ctx, requestUser, account, latestAcc); err != nil {
@@ -772,14 +784,11 @@ func (d *Dereferencer) enrichAccount(
 
 	if account.IsNew() {
 		// Prefer published/created time from
-		// apubAcc, fall back to FetchedAt value.
+		// apubAcc, fall back to current time.
 		if latestAcc.CreatedAt.IsZero() {
-			latestAcc.CreatedAt = latestAcc.FetchedAt
+			latestAcc.CreatedAt = now
 		}
 
-		// Set time of update from the last-fetched date.
-		latestAcc.UpdatedAt = latestAcc.FetchedAt
-
 		// This is new, put it in the database.
 		err := d.state.DB.PutAccount(ctx, latestAcc)
 		if err != nil {
@@ -792,9 +801,6 @@ func (d *Dereferencer) enrichAccount(
 			latestAcc.CreatedAt = account.CreatedAt
 		}
 
-		// Set time of update from the last-fetched date.
-		latestAcc.UpdatedAt = latestAcc.FetchedAt
-
 		// This is an existing account, update the model in the database.
 		if err := d.state.DB.UpdateAccount(ctx, latestAcc); err != nil {
 			return nil, nil, gtserror.Newf("error updating database: %w", err)
-- 
cgit v1.2.3