element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.2k stars 2k forks source link

Handle cross-signing key changes when old key is cached #12904

Open dbkr opened 4 years ago

dbkr commented 4 years ago

Ensure we handle the user resetting their cross-signing keys and notice that the ones we have cached are no longer useful. We can do this by ensuring that we always revalidate the that the private key we use matches the public key stored on the account.

Presumably if we don't have the key cached, we should prompt for the SSSS (secure secret storage and sharing) passphrase?

foldleft commented 4 years ago

we do this already for getCrossSigningKey, when we prompt the user:

        function validateKey(key) {
            if (!key) return;
            const signing = new global.Olm.PkSigning();
            const gotPubkey = signing.init_with_seed(key);
            if (gotPubkey === expectedPubkey) {
                return [gotPubkey, signing];
            }
            signing.free();
        }

        let privkey;
        if (this._cacheCallbacks.getCrossSigningKeyCache && shouldCache) {
            privkey = await this._cacheCallbacks
              .getCrossSigningKeyCache(type, expectedPubkey);
        }

        const cacheresult = validateKey(privkey);

however, it doesn't happen for key sharing. for that we just hand over whatever we have in the cache.

foldleft commented 4 years ago

and then for requests, it looks like this:

const crossSigning = new CrossSigningInfo(
                    original.userId,
                    { getCrossSigningKey: async (type) => {
                        console.debug("VerificationBase.done: requesting secret",
                                      type, this.deviceId);
                        const { promise } = client.requestSecret(
                            `m.cross_signing.${type}`, [this.deviceId],
                        );
                        const result = await promise;
                        const decoded = decodeBase64(result);
                        return Uint8Array.from(decoded);
                    } },
                    original._cacheCallbacks,
                );

which is to say, the requesting side validates that it didn't get a bad key. the responding side, however, does happily send a bad key.

So I think there's no case where you might end up getting your keys poisoned: all the paths in question do check.

However, there's still one edge-case we should probably consider: if a bad key is sent as a response, pre-empting a correct response, then key sharing won't work properly

dbkr commented 4 years ago

OK yeah - as long as we then fall back to getting the keys out of SSSS if our cached key is wrong then we are probably fine here, as even in the worst case if we do get stale keys from a keyshare request, it will just mean the user has to enter their passphrase. We can maybe worry about validating keys either before sharing or upon receiving at a later date if we want to.

foldleft commented 4 years ago

should we close this?

jryans commented 4 years ago

Sounds like we can at the very least defer, I'll leave to @dbkr on whether to close or not.