Mumble: Murmur crash with QMYSQL when emoji in username

[jgrancell]

Describe the bug
When using QMYSQL as the database option in murmur.ini, it is possible to consistently crash the murmur server by logging in with an emoji as part of the user name.

Steps to Reproduce
Steps to reproduce the behavior:

1. Connect to a server using an emoji (I used 🌲) in your username.
2. The server will accept you and then immediately close the connection.

Expected behavior
The server should not crash.

Desktop (please complete the following information):

• OS: CentOS 7
• Version: Murmur 1.3

Additional context
Logs for the crash:

<F>2020-05-28 00:07:45.781 SQL Error [INSERT INTO `murmur_slog` (`server_id`, `msg`) VALUES(?,?)]: Incorrect string value: '\xF0\x9F\x8C\xB2 A...' for column `dogft_murmur`.`murmur_slog`.`msg` at row 1 QMYSQL3: Unable to execute statement

[Krzmbrzl]

Did you close the issue again on purpose? :thinking:

[jgrancell]

I've done some further digging into this. The actual crash can be averted by ensuring the MySQL table is set to utf8mb4, as that clears up the error.

It doesn't really fix the underlying issue that a MySQL error is being unhandled and causing the entire murmur service to crash.

[TredwellGit]

@Krzmbrzl
This should be reopened and is possibly a security vulnerability. It appears that Mumble incorrectly uses a proprietary encoding that MySQL erroneously calls utf8. Actual UTF-8 is called utf8mb4 in MySQL. Please read: https://mathiasbynens.be/notes/mysql-utf8mb4

[Krzmbrzl]

@TredwellGit do you know whether this only affects MySQL or also SQLite and/or PostgreSQL?

[TredwellGit]

The utf8mb4 issue is specific to MySQL. SQLite is safe since it uses actual UTF-8 by default. I am currently looking at what Mumble does with PostgreSQL.

[TredwellGit]

The PostgreSQL situation seems more complicated.

The default character set is selected while initializing your PostgreSQL database cluster using initdb. It can be overridden when you create a database, so you can have multiple databases each with a different character set.

The default is derived from the locale, or SQL_ASCII if that does not work.

Maybe it would be better to explicitly set PostgreSQL to UTF8 which is actual UTF-8.

However, I see that Mumble uses QLatin1String instead of QString which is concerning. Why does Mumble do this?

[Krzmbrzl]

However, I see that Mumble uses QLatin1String instead of QString which is concerning. Why does Mumble do this?

I have been told that it is more performant and that it is the preferred solution if you don't require all the functionality a QString provides :shrug:

[TredwellGit]

Apparently QString uses UTF-16 instead of UTF-8 because Windows uses potentially ill-formed UTF-16…

[Krzmbrzl]

Okay after some fiddling around, I was able to reproduce this crash (using 🌲 in my name). For that to happen I first had to go and explicitly allow arbitrary user names on my server (using username=.*) and the switch my database driver to use MySQL (and also configuring what table it should use which took longer than one should think).

I'll look into this and see if I can fix it :point_up:

[felix91gr]

a proprietary encoding that MySQL erroneously calls utf8

Holy shit, naming something that's not utf8 "utf8" should be illegal, wtf is wrong with the MySQL people??

[jgrancell]

Okay after some fiddling around, I was able to reproduce this crash (using 🌲 in my name). For that to happen I first had to go and explicitly allow arbitrary user names on my server (using username=.*) and the switch my database driver to use MySQL (and also configuring what table it should use which took longer than one should think).

I'll look into this and see if I can fix it ☝️

@Krzmbrzl

To be clear, this bug does not (or did not now that it's fixed) require setting username=.* for it to cause a crash. I was reliably able to reproduce this issue with a much more strict [a-zA-Z] regex, because it was still attempting to insert the 🌲 emoji into the slog table to record it as a rejected username.

[Krzmbrzl]

Hm interesting :thinking:

I was just about to say that I wasn't able to reproduce this without lifting the restriction, but the first tests I made were with SQLite which didn't crash in the first place... Anyways I think for the fix it doesn't matter :shrug:

Thanks for clarifying though :)