TryQuiet / quiet

A private, p2p alternative to Slack and Discord built on Tor & IPFS
https://www.tryquiet.org
GNU General Public License v3.0
1.93k stars 82 forks source link

Add certificates store validation #1899

Closed leblowl closed 8 months ago

leblowl commented 11 months ago

Currently anyone can write to the certificates log. This allows anyone to forge certificates. certificates store should only be writable to by the owner and we should validate certs and verify they are signed by the owner.

With the additions of #1843, when certificates are replicated: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/storage/storage.service.ts#L361-L367

They are then sent to the client, where they are parsed and added to the Redux store: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/state-manager/src/sagas/users/users.slice.ts#L20-L30

parseCertificate doesn't check the signature.

Then certs are used for usernames via allUsers selector: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/state-manager/src/sagas/users/users.selectors.ts#L76

If we don't verify that a cert is signed by the owner, we cannot guarantee username uniqueness and we cannot guarantee the public key in the cert is connected to the username in the cert... assuming that we trust the owner to check uniqueness and verify CSR signatures.

holmesworcester commented 11 months ago

Right now lucas believes this means that anyone can change another user's name.

holmesworcester commented 11 months ago

We should address #1891 first and then address this.

leblowl commented 11 months ago

We can assume some of the the infrastructure necessary for this task will be implemented in #1891. Specifically that there will exist functions putOwnerOrbitDbIdentityId and getOwnerOrbitDbIdentityId in src/nest/storage/identity.ts. If implementing this in parallel to #1891, we can mock these functions and use them to verify that only the owner can append entries to the certificates store. We can use OrbitDB's IPFSAccessController as an example of how to do that: https://github.com/orbitdb/orbit-db-access-controllers/blob/3741eb318e9c7efea5af15a54110cf6de8ae1fe3/src/access-controllers/ipfs.js#L20

We should also verify:

Since it looks like access controllers only validate heads, I think we should simply validate each entry ourselves. We can create a new validation function validateCertificateEntry that, given an entry, validates the entry according the specs above (and calls Entry.verify) and returns true/false. Then we can create a new function, getCertificates that iterates over the certificates log entries, and filters the logs with validateCertificateEntry such that it only returns valid entries. It should also filter and return only the latest certificate per public key such that there is a 1-to-1 mapping of public key to cert. We can then use getCertificates to send valid certs to other parts of the application for processing. See also https://github.com/TryQuiet/quiet/issues/1893. We can optimize this later.

It might be nice to encapsulate everything in a CertificateStore class. See also: https://github.com/TryQuiet/quiet/pull/1923

kingalg commented 9 months ago

@leblowl any way I can check this doing manual checks?

kingalg commented 8 months ago

Desktop: quiet@2.0.3-alpha.15 Mobile: mobile@2.0.3-alpha.17, ios 344