palant / pfp

A simple and secure browser extension to be used with KeePass databases.
https://pfp.works/
Mozilla Public License 2.0
113 stars 14 forks source link

Make sync protocol resistant to manipulations #86

Closed palant closed 5 years ago

palant commented 6 years ago

The sync protocol as implemented in #24 still has some issues. While the storage provider cannot decrypt the data stored by PfP, they might still be able to tamper with it:

How this can be addressed:

There is still one legitimate scenario where the revision number might decrease: if the user removes the data from the storage and syncs an unrelated instance to it then. An existing client should refuse to sync then, there is no way to tell that it isn't an old version of the data. The user can disable sync and reconnect to repair this.

palant commented 5 years ago

Some additional considerations:

palant commented 5 years ago

It might be a good idea to derive HMAC secrets from the master password,

Never mind, this is a bad idea, as there is no salt which is random enough and could be used here. Then it just needs a way to resolve the conflict if it happens. Sync needs to be disabled on the failing client and the client reconnected then. With identical master passwords this will just work and bring the clients in sync. With different master passwords or otherwise broken data, connecting should error out and offer to remove the data.

palant commented 5 years ago

I'm not sure whether we can rely on the JSON serializer to be consistent across browsers.

The spec actually defines exactly which characters should be escaped and how. I tested this in Firefox and Chrome and it seems that we can rely on the results to be consistent. However, sorting keys is not possible.

So I think that in order to sign the data we should convert our data object to an array of key/value pairs, sorted by key (signature key excluded). So {foo: 1, bar: 2} becomes [["bar", 2], ["foo", 1]]. This array should then be stringified to JSON without any whitespace and signed.

palant commented 5 years ago

Initial upload requires the master password, which will become problematic if autolock delay is set to 0. As I realized now, this is already an issue due to required rekeying.

palant commented 5 years ago

A bulk of this has been implemented, documentation has been updated as well. Better handling of edge conditions is still necessary.

palant commented 5 years ago

This can be considered resolved now, further UI enhancement is #103 now.