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 certificatesRequests store validation #1893

Closed leblowl closed 8 months ago

leblowl commented 11 months ago

We should verify that each CSR is signed by pubKey (see: https://github.com/TryQuiet/quiet/issues/1892) and validate usernames, etc.

This prevents users from forging other users' CSRs, overwriting other users' username requests and from adding invalid usernames.

Currently, with the additions of #1843, CSRs are replicated: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/storage/storage.service.ts#L404

And via RESPONSE_GET_CSRS, these CSRs are sent to the frontend: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/connections-manager/connections-manager.service.ts#L562-L569

CSRs are indexed by public key: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/state-manager/src/sagas/users/users.selectors.ts#L48

Where the public key is extracted from the decoded cert: https://github.com/TryQuiet/quiet/blob/277f96631805071654ad15f2bdf7f03567d3fadc/packages/identity/src/extractPubKey.ts#L14 https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/identity/src/extractPubKey.ts#L22

These CSRs are then used for associating username with users: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/state-manager/src/sagas/users/users.selectors.ts#L76

It appears that the CSR isn't validated in this example, so you could BER encode a CSR with someone else's public key and since we don't check the signature, we wouldn't notice.

Following the other path that CSRs take, they get signed by the owner via REGISTER_USER_CERTIFICATE. In this path, first they are decoded: https://github.com/TryQuiet/quiet/blob/277f96631805071654ad15f2bdf7f03567d3fadc/packages/identity/src/common.ts#L92-L95

They are then filtered so only CSRs with unregistered names are prepared for signing: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/registration/registration.functions.ts#L29

Then each filtered CSR is signed: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/registration/registration.functions.ts#L72

Inside issueCertificate, there is a validateCsr function, but that only appears to validate the BER encoding and basic structural properties: https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/registration/registration.functions.ts#L62-L67

https://github.com/TryQuiet/quiet/blob/e019b86fdf18d533ace217c4aaf63c945a12d3ee/packages/backend/src/nest/registration/registration.functions.ts#L11-L16

generateUserCert also doesn't appear to validate the CSR signature before signing: https://github.com/TryQuiet/quiet/blob/277f96631805071654ad15f2bdf7f03567d3fadc/packages/identity/src/generateUserCertificate.ts#L24

So it looks like anyone could send a CSR with another user's public key in the CSR and an invalid signature (because they don't control that public key) and thus change another user's username.

holmesworcester commented 11 months ago

How would overwriting others' usernames work? Do you mean submitting a request for the name alice right after someone else submits a request for the name alice? I think we can't stop that with an access controller. Or do you mean something else?

leblowl commented 11 months ago

@holmesworcester I added additional details to the issue. Basically, if we don't check CSR signatures then I think anyone can put someone else's public key and a random username in a CSR (the CSR signature won't be valid, but we don't check that) then get a certificate issued for it, thus changing another user's username. The signature of the CSR needs to match the CSR data and be signed by the public key included in the CSR data, otherwise you can just include any public key in the CSR.

holmesworcester commented 11 months ago

Note: we would do this together with #1892

leblowl commented 11 months ago

In order to validate a CSR, we need to check:

To implement this validation, we could use an access controller, but it looks like access controllers only check heads and we need to validate each entry. As an alternative, we can create a new validation function validateCsrEntry 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, getCsrs that iterates over the certificatesRequests log entries, and filters the logs with validateCsrEntry such that it only returns valid entries. We can then use getCsrs to send valid CSRs to other parts of the application for processing. Also logic from the state-manager that retrieves the latest CSR for each public key should be moved into getCsrs on the backend.

With this approach we will still be caching and replicating invalid entries in IPFS, however when we implement permanent deletion, we can revisit that.

This approach also involves re-validating invalid entries when iterating over the log successively. That's not ideal, but I think we can optimize it in the future.

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

holmesworcester commented 11 months ago

I think the validation should lowercase letters only, hyphens allowed, and numbers

kingalg commented 8 months ago

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