[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

@@ -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)

internal/federation/dereferencing/account_test.go

@@ -19,11 +19,17 @@ package dereferencing_test
 
 import (
 	"context"
+	"crypto/rsa"
+	"crypto/x509"
+	"encoding/pem"
 	"fmt"
+	"net/url"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/suite"
+	"github.com/superseriousbusiness/activity/streams"
+	"github.com/superseriousbusiness/activity/streams/vocab"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/config"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
@@ -226,10 +232,172 @@ func (suite *AccountTestSuite) TestDereferenceRemoteAccountWithNonMatchingURI()
 		fetchingAccount.Username,
 		testrig.URLMustParse(remoteAltURI),
 	)
-	suite.Equal(err.Error(), fmt.Sprintf("enrichAccount: dereferenced account uri %s does not match %s", remoteURI, remoteAltURI))
+	suite.Equal(err.Error(), fmt.Sprintf("enrichAccount: account uri %s does not match %s", remoteURI, remoteAltURI))
 	suite.Nil(fetchedAccount)
 }
 
+func (suite *AccountTestSuite) TestDereferenceRemoteAccountWithUnexpectedKeyChange() {
+	ctx, cncl := context.WithCancel(context.Background())
+	defer cncl()
+
+	fetchingAcc := suite.testAccounts["local_account_1"]
+	remoteURI := "https://turnip.farm/users/turniplover6969"
+
+	// Fetch the remote account to load into the database.
+	remoteAcc, _, err := suite.dereferencer.GetAccountByURI(ctx,
+		fetchingAcc.Username,
+		testrig.URLMustParse(remoteURI),
+	)
+	suite.NoError(err)
+	suite.NotNil(remoteAcc)
+
+	// Mark account as requiring a refetch.
+	remoteAcc.FetchedAt = time.Time{}
+	err = suite.state.DB.UpdateAccount(ctx, remoteAcc, "fetched_at")
+	suite.NoError(err)
+
+	// Update remote to have an unexpected different key.
+	remotePerson := suite.client.TestRemotePeople[remoteURI]
+	setPublicKey(remotePerson,
+		remoteURI,
+		fetchingAcc.PublicKeyURI+".unique",
+		fetchingAcc.PublicKey,
+	)
+
+	// Force refresh account expecting key change error.
+	_, _, err = suite.dereferencer.RefreshAccount(ctx,
+		fetchingAcc.Username,
+		remoteAcc,
+		nil,
+		nil,
+	)
+	suite.Equal(err.Error(), fmt.Sprintf("RefreshAccount: enrichAccount: account %s pubkey has changed (key rotation required?)", remoteURI))
+}
+
+func (suite *AccountTestSuite) TestDereferenceRemoteAccountWithExpectedKeyChange() {
+	ctx, cncl := context.WithCancel(context.Background())
+	defer cncl()
+
+	fetchingAcc := suite.testAccounts["local_account_1"]
+	remoteURI := "https://turnip.farm/users/turniplover6969"
+
+	// Fetch the remote account to load into the database.
+	remoteAcc, _, err := suite.dereferencer.GetAccountByURI(ctx,
+		fetchingAcc.Username,
+		testrig.URLMustParse(remoteURI),
+	)
+	suite.NoError(err)
+	suite.NotNil(remoteAcc)
+
+	// Expire the remote account's public key.
+	remoteAcc.PublicKeyExpiresAt = time.Now()
+	remoteAcc.FetchedAt = time.Time{} // force fetch
+	err = suite.state.DB.UpdateAccount(ctx, remoteAcc, "fetched_at", "public_key_expires_at")
+	suite.NoError(err)
+
+	// Update remote to have a different stored public key.
+	remotePerson := suite.client.TestRemotePeople[remoteURI]
+	setPublicKey(remotePerson,
+		remoteURI,
+		fetchingAcc.PublicKeyURI+".unique",
+		fetchingAcc.PublicKey,
+	)
+
+	// Refresh account expecting a succesful refresh with changed keys!
+	updatedAcc, apAcc, err := suite.dereferencer.RefreshAccount(ctx,
+		fetchingAcc.Username,
+		remoteAcc,
+		nil,
+		nil,
+	)
+	suite.NoError(err)
+	suite.NotNil(apAcc)
+	suite.True(updatedAcc.PublicKey.Equal(fetchingAcc.PublicKey))
+}
+
+func (suite *AccountTestSuite) TestRefreshFederatedRemoteAccountWithKeyChange() {
+	ctx, cncl := context.WithCancel(context.Background())
+	defer cncl()
+
+	fetchingAcc := suite.testAccounts["local_account_1"]
+	remoteURI := "https://turnip.farm/users/turniplover6969"
+
+	// Fetch the remote account to load into the database.
+	remoteAcc, _, err := suite.dereferencer.GetAccountByURI(ctx,
+		fetchingAcc.Username,
+		testrig.URLMustParse(remoteURI),
+	)
+	suite.NoError(err)
+	suite.NotNil(remoteAcc)
+
+	// Update remote to have a different stored public key.
+	remotePerson := suite.client.TestRemotePeople[remoteURI]
+	setPublicKey(remotePerson,
+		remoteURI,
+		fetchingAcc.PublicKeyURI+".unique",
+		fetchingAcc.PublicKey,
+	)
+
+	// Refresh account expecting a succesful refresh with changed keys!
+	// By passing in the remote person model this indicates that the data
+	// was received via the federator, which should trust any key change.
+	updatedAcc, apAcc, err := suite.dereferencer.RefreshAccount(ctx,
+		fetchingAcc.Username,
+		remoteAcc,
+		remotePerson,
+		nil,
+	)
+	suite.NoError(err)
+	suite.NotNil(apAcc)
+	suite.True(updatedAcc.PublicKey.Equal(fetchingAcc.PublicKey))
+}
+
 func TestAccountTestSuite(t *testing.T) {
 	suite.Run(t, new(AccountTestSuite))
 }
+
+func setPublicKey(person vocab.ActivityStreamsPerson, ownerURI, keyURI string, key *rsa.PublicKey) {
+	profileIDURI, err := url.Parse(ownerURI)
+	if err != nil {
+		panic(err)
+	}
+
+	publicKeyURI, err := url.Parse(keyURI)
+	if err != nil {
+		panic(err)
+	}
+
+	publicKeyProp := streams.NewW3IDSecurityV1PublicKeyProperty()
+
+	// create the public key
+	publicKey := streams.NewW3IDSecurityV1PublicKey()
+
+	// set ID for the public key
+	publicKeyIDProp := streams.NewJSONLDIdProperty()
+	publicKeyIDProp.SetIRI(publicKeyURI)
+	publicKey.SetJSONLDId(publicKeyIDProp)
+
+	// set owner for the public key
+	publicKeyOwnerProp := streams.NewW3IDSecurityV1OwnerProperty()
+	publicKeyOwnerProp.SetIRI(profileIDURI)
+	publicKey.SetW3IDSecurityV1Owner(publicKeyOwnerProp)
+
+	// set the pem key itself
+	encodedPublicKey, err := x509.MarshalPKIXPublicKey(key)
+	if err != nil {
+		panic(err)
+	}
+	publicKeyBytes := pem.EncodeToMemory(&pem.Block{
+		Type:  "PUBLIC KEY",
+		Bytes: encodedPublicKey,
+	})
+	publicKeyPEMProp := streams.NewW3IDSecurityV1PublicKeyPemProperty()
+	publicKeyPEMProp.Set(string(publicKeyBytes))
+	publicKey.SetW3IDSecurityV1PublicKeyPem(publicKeyPEMProp)
+
+	// append the public key to the public key property
+	publicKeyProp.AppendW3IDSecurityV1PublicKey(publicKey)
+
+	// set the public key property on the Person
+	person.SetW3IDSecurityV1PublicKey(publicKeyProp)
+}

internal/federation/dereferencing/authenticate.go

@@ -0,0 +1,54 @@
+// GoToSocial
+// Copyright (C) GoToSocial Authors admin@gotosocial.org
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package dereferencing
+
+import (
+	"time"
+
+	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+)
+
+// verifyAccountKeysOnUpdate verifies that account's public key hasn't changed on update from
+// our existing stored representation, UNLESS the key has been explicitly expired (i.e. key rotation).
+func verifyAccountKeysOnUpdate(existing, latest *gtsmodel.Account, now time.Time, federated bool) bool {
+	if federated {
+		// If this data was federated
+		// to us then we implicitly trust
+		// it on the grounds that it
+		// passed any signature checks.
+		return true
+	}
+
+	if existing.PublicKey == nil {
+		// New account which has been
+		// passed as a placeholder.
+		// This is always permitted.
+		return true
+	}
+
+	// Ensure that public keys have not changed.
+	if existing.PublicKey.Equal(latest.PublicKey) &&
+		existing.PublicKeyURI == latest.PublicKeyURI {
+		return true
+	}
+
+	// The only time that an account key change is
+	// permitted is when it is marked as expired.
+	return !existing.PublicKeyExpiresAt.IsZero() &&
+		existing.PublicKeyExpiresAt.Before(now)
+}

internal/processing/workers/fromfediapi.go

@@ -674,8 +674,13 @@ func (p *fediAPI) UpdateAccount(ctx context.Context, fMsg *messages.FromFediAPI)
 		fMsg.Receiving.Username,
 		account,
 		apubAcc,
-		// Force refresh within 5min window.
-		dereferencing.Fresh,
+
+		// Force refresh within 10s window.
+		//
+		// Missing account updates could be
+		// detrimental to federation if they
+		// include public key changes.
+		dereferencing.Freshest,
 	)
 	if err != nil {
 		log.Errorf(ctx, "error refreshing account: %v", err)