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

FIX(Server): Unauthenticated connections no longer add to user count #4817

Merged
merged 1 commit into from Mar 3, 2021

Conversation

LucasToole
Copy link
Contributor

This commit fixes both issues discussed in #4277 which were related to
the fact that any socket connection was allocated an ID and marked as
a user. This bug allowed for a malicious user to lock out all users of
a server regardless of server password usage. This change simply moves ID
allocation to after a connection is marked as Authenticated.

FIXES: #4277

This is a modification and reopening of #4812 because I accidentally killed it.

Checks

This commit fixes both issues discussed in mumble-voip#4277 which were related to
the fact that any socket connection was allocated an ID and marked as
a user. This bug allowed for a malicious user to lock out all users of
a server regardless of server password usage. This change simply moves ID
allocation to after a connection is marked as Authenticated.

FIXES: mumble-voip#4277
@LucasToole
Copy link
Contributor Author

Copied from the other PR on why it was changed:

I initially decided to keep both changes as one commit because they were both needed to fix the issue.

The second change is one solution to the two issues that cause ID exhaustion.

  1. The max amount of IDs a server has is (iMaxUsers * 2) - 1.
  2. Murmur assumes that all connections are potential clients and always hands out IDs.

Therefore without addressing at least one of these issues, the connections needed to lock users out of a server goes from iMaxUsers -> (iMaxUsers * 2) - 1.

I had originally gone with solution 1, and changed the IDs from a very limited pool of in-order integers (until someone disconnects out of order) to randomly generated unique IDs.

However after sleeping on it I revisited option 2 which I had initially thought as harder and found that Murmur doesn't seem to require that connections need IDs until they are authenticated. So I'm adding a simpler patch which just move ID assignment to after a connection is authenticated and it looks like it solve both problems.

I still believe that assigning IDs in a stateless manner is preferable to the current ID Queue Approach. Maybe it will make its way into a future patch.

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not have been necessary to create a new PR. Just changing the commits in the PR would have been sufficient.

But in the end it also doesn't hurt 🤷

@Krzmbrzl Krzmbrzl added bug A bug (error) in the software server labels Mar 2, 2021
@Krzmbrzl Krzmbrzl merged commit 6657a6f into mumble-voip:master Mar 3, 2021
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Mar 3, 2021

Thank you for fixing this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

murmur: each connection to the server is counted as a "user"
2 participants