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: [PATCH] [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 ++-- .../federation/dereferencing/account_test.go | 170 +++++++++++++++++- .../federation/dereferencing/authenticate.go | 54 ++++++ internal/processing/workers/fromfediapi.go | 9 +- 4 files changed, 249 insertions(+), 16 deletions(-) create mode 100644 internal/federation/dereferencing/authenticate.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) diff --git a/internal/federation/dereferencing/account_test.go b/internal/federation/dereferencing/account_test.go index f99012904..309771758 100644 --- a/internal/federation/dereferencing/account_test.go +++ b/internal/federation/dereferencing/account_test.go @@ -19,11 +19,17 @@ 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) +} diff --git a/internal/federation/dereferencing/authenticate.go b/internal/federation/dereferencing/authenticate.go new file mode 100644 index 000000000..7c5946202 --- /dev/null +++ b/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 . + +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) +} diff --git a/internal/processing/workers/fromfediapi.go b/internal/processing/workers/fromfediapi.go index 31df9d284..ce7c53388 100644 --- a/internal/processing/workers/fromfediapi.go +++ b/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)