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

server.d: username and roomname character set length limits #10

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

slook
Copy link
Contributor

@slook slook commented Aug 18, 2024

Is is a bad idea for enormous strings to be primary keys in a WITHOUT ROWID table, it works best if keys are < 50 bytes or so. This PR aims to avoid a potential performance issue during lookups whilst also being compliant with the protocol.

  • Changed: Maximum new username length from 1,000,000,000 (a thousand million) characters to 30 (thirty)
  • Changed: Maximum roomname length to 24
  • Changed: Validate username and roomname input strings as printable ASCII instead of UTF
  • Added: Reject usernames and roomnames containing " " (double-spaces), with possiblity for excluding other strings
  • Added: Reject names that are only a punctuation symbol (but still allow other single-character alpha/numeric names)
  • Added: Minimum of at least 1 character length for user and room names (reject zero-length strings)
  • Fixed: Vulnerability where any unregistered alien could execute two non-sanitized database queries before Login

Some other method to properly deal with UTF strings (chat messages, tickers, likes, etc) may have to be devised at a later point, but that will be unrelated to the check_login() Login and JoinRoom protocol apparatus. Hence, the function is renamed.

@slook slook marked this pull request as ready for review August 18, 2024 21:37
src/client.d Outdated Show resolved Hide resolved
src/server.d Outdated Show resolved Hide resolved
src/server.d Outdated Show resolved Hide resolved
src/server.d Outdated Show resolved Hide resolved
@mathiascode
Copy link
Member

Added: Reject names that are only a punctuation symbol (but still allow other single-character alpha/numeric names

Is there a technical reason behind this? Does the official server do this?

@slook
Copy link
Contributor Author

slook commented Aug 19, 2024

Is there a technical reason behind this?

Mostly paranoia caused by trauma that I experienced when working with MS SQL ("Jet Engine") which had many vulnerabilities, one of which using an * (asterisk) used to select all columns and then somehow sequence an escape from within a query string component to select more columns (including perhaps the 'password' or 'privileges' column) than what was normally available.

There are probably other wildcard type operators, but I don't know much about it other than extreme caution is always needed when allowing users a remote opportunity to enter arbitrary TEXT values (INTEGER values aren't so exploitable) anywhere near where any SQL query statements are being prepared. Of course, SQLITE is much more mature than any proprietary engine ever will be, so there's probably nothing to worry about, really.

Even though most injection attacks have probably been well patched these days, I still wouldn't be confident about allowing such primary keys unless sensitive data was stored elsewhere, like in a protected table (or other database file) that would usually be disconnected from engine that runs the authentication database, in such way that unauthenticated peers only get to see boolean values which tells their client if they are registered/banned or not before allowing them to select the table required for logging in, or performing any privileged actions. If such hardening measures were in place, then maybe this sort of restriction could be relaxed.

Does the official server do this?

No, in this case the official server usually seems to respond with INVALIDPASSWORD, rather than INVALIDUSERNAME.

It would be impossible (without performing some kind of dictionary attack on the official server) to determine if that happens because this sort of thing is purposefully done, or if it's simply the case that all single-character names are more likely to be registered by someone's spam bot script.

@mathiascode mathiascode merged commit 2de794c into soulfind-dev:master Aug 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants