Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Community owner will register the same username twice when it should not #2155

Closed
kingalg opened this issue Dec 8, 2023 · 4 comments
Closed
Labels
bug Something isn't working security

Comments

@kingalg
Copy link
Collaborator

kingalg commented Dec 8, 2023

Version: 2.0.3-alpha.13
System: MacOS, Windows, Linux
So far issue not spotted on mobile.

Issue: more then one user registered with the same username.
It was working before alpha.13. The last time we had a bug related to impersonation attack was around 5 weeks ago and it was fixed.

Steps to recreate (this is how I did it, but Wiktor and Kacper replicated it in different ways as well):

  1. Create community and register owner on desktop.
  2. Make sure that the owner is turn on.
  3. Join with another desktop and the same username as the owner.

What happens - both users are registered with the same username and channel users get "possible impersonation attack" popup.

@kingalg kingalg added bug Something isn't working security labels Dec 8, 2023
@kingalg kingalg added this to Quiet Dec 8, 2023
@kingalg kingalg moved this to In progress in Quiet Dec 8, 2023
@siepra
Copy link
Contributor

siepra commented Dec 8, 2023

HYPOTHESIS:

certificatesRequestsStore.ts:71 emits LOADED_USER_CSRS on database load, which then triggers REPLICATED_CSR in storage.service.ts:334. It accepts an array of certificates as a payload field, which is being loaded from the certificates store.

Method extractPendingCsrs should filter out CSRs containing usernames already used in registered certificates, but if it received an empty list of certificates, then it won't filter out anything.

My hypothesis is there's a race condition when the certificates store is being loaded AFTER the CSRs store and as a result it isn't providing certificates to verify username uniqueness against.

HOW TO VERIFY:

Modify the code so it always passes an empty array of certificates and see if the problem becomes deterministic.

POSSIBLE SOLUTION:

Make sure certificates are always being loaded before CSRs.

@holmesworcester
Copy link
Contributor

holmesworcester commented Dec 8, 2023

Lucas added this PR: #2157

@leblowl feel free to add this back to "in progress" if I jumped the gun in moving it to "waiting for review".

@holmesworcester holmesworcester moved this from In progress to Waiting for review in Quiet Dec 8, 2023
@holmesworcester holmesworcester moved this from Waiting for review to Sprint in Quiet Jan 2, 2024
@holmesworcester holmesworcester moved this from Sprint to Next Sprint in Quiet Jan 2, 2024
@holmesworcester holmesworcester changed the title Possible impersonation attack Community owner will register the same username twice when it should not Jan 2, 2024
@holmesworcester
Copy link
Contributor

This is a heisenbug but the warning works correctly (so there isn't a security issue, we do not think) and it happens only in unlikely cases. Moved to next sprint.

leblowl pushed a commit that referenced this issue Jan 31, 2024
refactor: Refactor the registration service so that we can more easily verify that it does not register duplicate certs. Related to #2155
leblowl pushed a commit that referenced this issue Jan 31, 2024
Refactor the registration service so that we can more easily verify that it
does not register duplicate certs. Related to #2155
leblowl pushed a commit that referenced this issue Jan 31, 2024
Refactor the registration service so that we can more easily verify that it
does not register duplicate certs. Related to #2155
leblowl pushed a commit that referenced this issue Jan 31, 2024
Refactor the registration service so that we can more easily verify that it
does not register duplicate certs. Related to #2155
leblowl pushed a commit that referenced this issue Feb 6, 2024
Refactor the registration service so that we can more easily verify that it
does not register duplicate certs. Related to #2155
@holmesworcester
Copy link
Contributor

Noting that we think this is fixed. We should wait to see if it comes up again and reopen.

@kingalg please reopen if you're still seeing this in the latest release.

@holmesworcester holmesworcester moved this from Sprint to Done in Quiet Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
Status: Done
Development

No branches or pull requests

3 participants