From 3a8005872939e7620bcd9bd6770421e33ddb4b95 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 12 Aug 2021 14:42:25 +0100 Subject: [PATCH] Don't bother verifying signatures, validate key lengths if we can, notifier fixes --- clientapi/jsonerror/jsonerror.go | 6 + clientapi/routing/key_crosssigning.go | 10 ++ keyserver/api/api.go | 1 + keyserver/internal/cross_signing.go | 246 +++++++++++--------------- keyserver/producers/keychange.go | 6 +- sytest-whitelist | 2 + 6 files changed, 125 insertions(+), 146 deletions(-) diff --git a/clientapi/jsonerror/jsonerror.go b/clientapi/jsonerror/jsonerror.go index c42b25bea..7accde5f5 100644 --- a/clientapi/jsonerror/jsonerror.go +++ b/clientapi/jsonerror/jsonerror.go @@ -131,6 +131,12 @@ func InvalidSignature(msg string) *MatrixError { return &MatrixError{"M_INVALID_SIGNATURE", msg} } +// InvalidParam is an error that is returned when a parameter was invalid, +// traditionally with cross-signing. +func InvalidParam(msg string) *MatrixError { + return &MatrixError{"M_INVALID_PARAM", msg} +} + // MissingParam is an error that is returned when a parameter was incorrect, // traditionally with cross-signing. func MissingParam(msg string) *MatrixError { diff --git a/clientapi/routing/key_crosssigning.go b/clientapi/routing/key_crosssigning.go index 3c103fd72..756598dbc 100644 --- a/clientapi/routing/key_crosssigning.go +++ b/clientapi/routing/key_crosssigning.go @@ -73,6 +73,11 @@ func UploadCrossSigningDeviceKeys( Code: http.StatusBadRequest, JSON: jsonerror.MissingParam(err.Error()), } + case err.IsInvalidParam: + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.InvalidParam(err.Error()), + } default: return util.JSONResponse{ Code: http.StatusBadRequest, @@ -110,6 +115,11 @@ func UploadCrossSigningDeviceSignatures(req *http.Request, keyserverAPI api.KeyI Code: http.StatusBadRequest, JSON: jsonerror.MissingParam(err.Error()), } + case err.IsInvalidParam: + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.InvalidParam(err.Error()), + } default: return util.JSONResponse{ Code: http.StatusBadRequest, diff --git a/keyserver/api/api.go b/keyserver/api/api.go index 449c42969..40120236f 100644 --- a/keyserver/api/api.go +++ b/keyserver/api/api.go @@ -48,6 +48,7 @@ type KeyError struct { Err string `json:"error"` IsInvalidSignature bool `json:"is_invalid_signature,omitempty"` // M_INVALID_SIGNATURE IsMissingParam bool `json:"is_missing_param,omitempty"` // M_MISSING_PARAM + IsInvalidParam bool `json:"is_invalid_param,omitempty"` // M_INVALID_PARAM } func (k *KeyError) Error() string { diff --git a/keyserver/internal/cross_signing.go b/keyserver/internal/cross_signing.go index 216cda186..bb5d99418 100644 --- a/keyserver/internal/cross_signing.go +++ b/keyserver/internal/cross_signing.go @@ -19,7 +19,6 @@ import ( "context" "crypto/ed25519" "database/sql" - "encoding/json" "fmt" "strings" @@ -28,6 +27,7 @@ import ( "github.com/matrix-org/dendrite/keyserver/types" "github.com/matrix-org/gomatrixserverlib" "github.com/sirupsen/logrus" + "golang.org/x/crypto/curve25519" ) func sanityCheckKey(key gomatrixserverlib.CrossSigningKey, userID string, purpose gomatrixserverlib.CrossSigningKeyPurpose) error { @@ -46,6 +46,41 @@ func sanityCheckKey(key gomatrixserverlib.CrossSigningKey, userID string, purpos if tokens[1] != b64 { return fmt.Errorf("key ID isn't correct") } + switch tokens[0] { + case "ed25519": + if len(keyData) != ed25519.PublicKeySize { + return fmt.Errorf("ed25519 key is not the correct length") + } + case "curve25519": + if len(keyData) != curve25519.PointSize { + return fmt.Errorf("curve25519 key is not the correct length") + } + default: + // We can't enforce the key length to be correct for an + // algorithm that we don't recognise, so instead we'll + // just make sure that it isn't incredibly excessive. + if len(keyData) > 4096 { + return fmt.Errorf("unknown key type is too long") + } + } + } + + // Check to see if the signatures make sense + for _, forOriginUser := range key.Signatures { + for originKeyID, originSignature := range forOriginUser { + switch strings.SplitN(string(originKeyID), ":", 1)[0] { + case "ed25519": + if len(originSignature) != ed25519.SignatureSize { + return fmt.Errorf("ed25519 signature is not the correct length") + } + case "curve25519": + return fmt.Errorf("curve25519 signatures are impossible") + default: + if len(originSignature) > 4096 { + return fmt.Errorf("unknown signature type is too long") + } + } + } } // Does the key claim to be from the right user? @@ -70,42 +105,68 @@ func sanityCheckKey(key gomatrixserverlib.CrossSigningKey, userID string, purpos // nolint:gocyclo func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.PerformUploadDeviceKeysRequest, res *api.PerformUploadDeviceKeysResponse) { - var masterKey gomatrixserverlib.Base64Bytes + // Find the keys to store. + byPurpose := map[gomatrixserverlib.CrossSigningKeyPurpose]gomatrixserverlib.CrossSigningKey{} + toStore := types.CrossSigningKeyMap{} hasMasterKey := false if len(req.MasterKey.Keys) > 0 { if err := sanityCheckKey(req.MasterKey, req.UserID, gomatrixserverlib.CrossSigningKeyPurposeMaster); err != nil { res.Error = &api.KeyError{ - Err: "Master key sanity check failed: " + err.Error(), + Err: "Master key sanity check failed: " + err.Error(), + IsInvalidParam: true, } return } - for _, keyData := range req.MasterKey.Keys { // iterates once, because sanityCheckKey requires one key - hasMasterKey = true - masterKey = keyData + + byPurpose[gomatrixserverlib.CrossSigningKeyPurposeMaster] = req.MasterKey + for _, key := range req.MasterKey.Keys { // iterates once, see sanityCheckKey + toStore[gomatrixserverlib.CrossSigningKeyPurposeMaster] = key } + hasMasterKey = true } if len(req.SelfSigningKey.Keys) > 0 { if err := sanityCheckKey(req.SelfSigningKey, req.UserID, gomatrixserverlib.CrossSigningKeyPurposeSelfSigning); err != nil { res.Error = &api.KeyError{ - Err: "Self-signing key sanity check failed: " + err.Error(), + Err: "Self-signing key sanity check failed: " + err.Error(), + IsInvalidParam: true, } return } + + byPurpose[gomatrixserverlib.CrossSigningKeyPurposeSelfSigning] = req.SelfSigningKey + for _, key := range req.SelfSigningKey.Keys { // iterates once, see sanityCheckKey + toStore[gomatrixserverlib.CrossSigningKeyPurposeSelfSigning] = key + } } if len(req.UserSigningKey.Keys) > 0 { if err := sanityCheckKey(req.UserSigningKey, req.UserID, gomatrixserverlib.CrossSigningKeyPurposeUserSigning); err != nil { res.Error = &api.KeyError{ - Err: "User-signing key sanity check failed: " + err.Error(), + Err: "User-signing key sanity check failed: " + err.Error(), + IsInvalidParam: true, } return } + + byPurpose[gomatrixserverlib.CrossSigningKeyPurposeUserSigning] = req.UserSigningKey + for _, key := range req.UserSigningKey.Keys { // iterates once, see sanityCheckKey + toStore[gomatrixserverlib.CrossSigningKeyPurposeUserSigning] = key + } } - // If the user hasn't given a new master key, then let's go and get their - // existing keys from the database. + // If there's nothing to do then stop here. + if len(toStore) == 0 { + res.Error = &api.KeyError{ + Err: "No keys were supplied in the request", + IsMissingParam: true, + } + return + } + + // We can't have a self-signing or user-signing key without a master + // key, so make sure we have one of those. if !hasMasterKey { existingKeys, err := a.DB.CrossSigningKeysDataForUser(ctx, req.UserID) if err != nil { @@ -115,87 +176,20 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P return } - masterKey, hasMasterKey = existingKeys[gomatrixserverlib.CrossSigningKeyPurposeMaster] + _, hasMasterKey = existingKeys[gomatrixserverlib.CrossSigningKeyPurposeMaster] } - // If we still don't have a master key at this point then there's nothing else - // we can do - we've checked both the request and the database. + // If we still can't find a master key for the user then stop the upload. + // This satisfies the "Fails to upload self-signing key without master key" test. if !hasMasterKey { res.Error = &api.KeyError{ - Err: "No master key was found either in the database or in the request!", - IsMissingParam: true, - } - return - } - - // The key ID is basically the key itself. - masterKeyID := gomatrixserverlib.KeyID(fmt.Sprintf("ed25519:%s", masterKey.Encode())) - - // Work out which things we need to verify the signatures for. - toVerify := make(map[gomatrixserverlib.CrossSigningKeyPurpose]gomatrixserverlib.CrossSigningKey, 3) - toStore := types.CrossSigningKeyMap{} - if len(req.MasterKey.Keys) > 0 { - toVerify[gomatrixserverlib.CrossSigningKeyPurposeMaster] = req.MasterKey - } - if len(req.SelfSigningKey.Keys) > 0 { - toVerify[gomatrixserverlib.CrossSigningKeyPurposeSelfSigning] = req.SelfSigningKey - } - if len(req.UserSigningKey.Keys) > 0 { - toVerify[gomatrixserverlib.CrossSigningKeyPurposeUserSigning] = req.UserSigningKey - } - - if len(toVerify) == 0 { - res.Error = &api.KeyError{ - Err: "No supplied keys available for verification", - IsMissingParam: true, - } - return - } - - for purpose, key := range toVerify { - // Collect together the key IDs we need to verify with. This will include - // all of the key IDs specified in the signatures. - keyJSON, err := json.Marshal(key) - if err != nil { - res.Error = &api.KeyError{ - Err: fmt.Sprintf("The JSON of the key section is invalid: %s", err.Error()), - } - return - } - - switch purpose { - case gomatrixserverlib.CrossSigningKeyPurposeMaster: - // The master key might have a signature attached to it from the - // previous key, or from a device key, but there's no real need - // to verify it. Clients will perform key checks when the master - // key changes. - - default: - // Sub-keys should be signed by the master key. - if err := gomatrixserverlib.VerifyJSON(req.UserID, masterKeyID, ed25519.PublicKey(masterKey), keyJSON); err != nil { - res.Error = &api.KeyError{ - Err: fmt.Sprintf("The %q sub-key failed master key signature verification: %s", purpose, err.Error()), - IsInvalidSignature: true, - } - return - } - } - - // If we've reached this point then all the signatures are valid so - // add the key to the list of keys to store. - for _, keyData := range key.Keys { // iterates once, see sanityCheckKey - toStore[purpose] = keyData - } - } - - if len(toStore) == 0 { - res.Error = &api.KeyError{ - Err: "No supplied keys passed verification", + Err: "No master key was found", IsMissingParam: true, } return } + // Store the keys. if err := a.DB.StoreCrossSigningKeysForUser(ctx, req.UserID, toStore); err != nil { res.Error = &api.KeyError{ Err: fmt.Sprintf("a.DB.StoreCrossSigningKeysForUser: %s", err), @@ -204,7 +198,7 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P } // Now upload any signatures that were included with the keys. - for _, key := range toVerify { + for _, key := range byPurpose { var targetKeyID gomatrixserverlib.KeyID for targetKey := range key.Keys { // iterates once, see sanityCheckKey targetKeyID = targetKey @@ -229,11 +223,14 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P update := eduserverAPI.CrossSigningKeyUpdate{ UserID: req.UserID, } - if _, ok := toStore[gomatrixserverlib.CrossSigningKeyPurposeMaster]; ok { - update.MasterKey = &req.MasterKey + if mk, ok := byPurpose[gomatrixserverlib.CrossSigningKeyPurposeMaster]; ok { + update.MasterKey = &mk } - if _, ok := toStore[gomatrixserverlib.CrossSigningKeyPurposeSelfSigning]; ok { - update.SelfSigningKey = &req.SelfSigningKey + if ssk, ok := byPurpose[gomatrixserverlib.CrossSigningKeyPurposeSelfSigning]; ok { + update.SelfSigningKey = &ssk + } + if update.MasterKey == nil && update.SelfSigningKey == nil { + return } if err := a.Producer.ProduceSigningKeyUpdate(update); err != nil { res.Error = &api.KeyError{ @@ -297,7 +294,7 @@ func (a *KeyInternalAPI) PerformUploadDeviceSignatures(ctx context.Context, req } } - if err := a.processSelfSignatures(ctx, req.UserID, queryRes, selfSignatures); err != nil { + if err := a.processSelfSignatures(ctx, req.UserID, selfSignatures); err != nil { res.Error = &api.KeyError{ Err: fmt.Sprintf("a.processSelfSignatures: %s", err), } @@ -310,10 +307,25 @@ func (a *KeyInternalAPI) PerformUploadDeviceSignatures(ctx context.Context, req } return } + + // Finally, generate a notification that we updated the signatures. + for userID := range req.Signatures { + if _, host, err := gomatrixserverlib.SplitID('@', userID); err == nil && host == a.ThisServer { + update := eduserverAPI.CrossSigningKeyUpdate{ + UserID: userID, + } + if err := a.Producer.ProduceSigningKeyUpdate(update); err != nil { + res.Error = &api.KeyError{ + Err: fmt.Sprintf("a.Producer.ProduceSigningKeyUpdate: %s", err), + } + return + } + } + } } func (a *KeyInternalAPI) processSelfSignatures( - ctx context.Context, _ string, queryRes *api.QueryKeysResponse, + ctx context.Context, _ string, signatures map[string]map[gomatrixserverlib.KeyID]gomatrixserverlib.CrossSigningForKeyOrDevice, ) error { // Here we will process: @@ -324,37 +336,8 @@ func (a *KeyInternalAPI) processSelfSignatures( for targetKeyID, signature := range forTargetUserID { switch sig := signature.CrossSigningBody.(type) { case *gomatrixserverlib.CrossSigningKey: - // The user is signing their master key with one of their devices - // The QueryKeys response should contain the device key hopefully. - // First we need to marshal the blob back into JSON so we can verify - // it. - j, err := json.Marshal(sig) - if err != nil { - return fmt.Errorf("json.Marshal: %w", err) - } - for originUserID, forOriginUserID := range sig.Signatures { - originDeviceKeys, ok := queryRes.DeviceKeys[originUserID] - if !ok { - return fmt.Errorf("missing device keys for user %q", originUserID) - } - for originKeyID, originSig := range forOriginUserID { - var originKey gomatrixserverlib.DeviceKeys - if err := json.Unmarshal(originDeviceKeys[string(originKeyID)], &originKey); err != nil { - return fmt.Errorf("json.Unmarshal: %w", err) - } - - originSigningKey, ok := originKey.Keys[originKeyID] - if !ok { - return fmt.Errorf("missing origin signing key %q", originKeyID) - } - originSigningKeyPublic := ed25519.PublicKey(originSigningKey) - - if err := gomatrixserverlib.VerifyJSON(originUserID, originKeyID, originSigningKeyPublic, j); err != nil { - return fmt.Errorf("gomatrixserverlib.VerifyJSON: %w", err) - } - if err := a.DB.StoreCrossSigningSigsForTarget( ctx, originUserID, originKeyID, targetUserID, targetKeyID, originSig, ); err != nil { @@ -364,35 +347,8 @@ func (a *KeyInternalAPI) processSelfSignatures( } case *gomatrixserverlib.DeviceKeys: - // The user is signing one of their devices with their self-signing key - // The QueryKeys response should contain the master key hopefully. - // First we need to marshal the blob back into JSON so we can verify - // it. - j, err := json.Marshal(sig) - if err != nil { - return fmt.Errorf("json.Marshal: %w", err) - } - for originUserID, forOriginUserID := range sig.Signatures { for originKeyID, originSig := range forOriginUserID { - originSelfSigningKeys, ok := queryRes.SelfSigningKeys[originUserID] - if !ok { - return fmt.Errorf("missing self-signing key for user %q", originUserID) - } - - var originSelfSigningKeyID gomatrixserverlib.KeyID - var originSelfSigningKey gomatrixserverlib.Base64Bytes - for keyID, key := range originSelfSigningKeys.Keys { - originSelfSigningKeyID, originSelfSigningKey = keyID, key - break - } - - originSelfSigningKeyPublic := ed25519.PublicKey(originSelfSigningKey) - - if err := gomatrixserverlib.VerifyJSON(originUserID, originSelfSigningKeyID, originSelfSigningKeyPublic, j); err != nil { - return fmt.Errorf("gomatrixserverlib.VerifyJSON: %w", err) - } - if err := a.DB.StoreCrossSigningSigsForTarget( ctx, originUserID, originKeyID, targetUserID, targetKeyID, originSig, ); err != nil { diff --git a/keyserver/producers/keychange.go b/keyserver/producers/keychange.go index 34cd49422..782675c2a 100644 --- a/keyserver/producers/keychange.go +++ b/keyserver/producers/keychange.go @@ -93,7 +93,11 @@ func (p *KeyChange) ProduceSigningKeyUpdate(key eduapi.CrossSigningKeyUpdate) er m.Key = sarama.StringEncoder(key.UserID) m.Value = sarama.ByteEncoder(value) - _, _, err = p.Producer.SendMessage(&m) + partition, offset, err := p.Producer.SendMessage(&m) + if err != nil { + return err + } + err = p.DB.StoreKeyChange(context.Background(), partition, offset, key.UserID) if err != nil { return err } diff --git a/sytest-whitelist b/sytest-whitelist index d2f2a1c7d..9f3eb893a 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -554,3 +554,5 @@ Can upload self-signing keys Fails to upload self-signing keys with no auth Fails to upload self-signing key without master key can fetch self-signing keys over federation +Changing master key notifies local users +Changing user-signing key notifies local users