MySQL Password Sanity Checking

[cedwardsmedia]

MODX Revolution, as of 2.3.5, does not seem to have any password sanity check in place when asking for the database password during setup. This leads to unintended behavior if someone uses a database password containing a single quote mark as the config file uses single quote marks to enclose the string. Using a password with one single quote results in the remainder of line 8 in config.inc.php being read as code as opposed to the string - leading to a PHP fatal error. Using a password with two single quotes creates two strings on line 8 that aren't meant to be there, resulting in a fatal error.

Example 1: Bob uses mypassword'youcantguessit for his password during setup. Modx writes the following to config.inc.php:

$database_user = 'modx_database';
$database_password = 'mypassword'youcantguessit';
$database_connection_charset = 'utf8';

You can see that $database_password is encapsulated with single quotes. However, due to the lack of sanity checking, the string itself contains a single quote. When PHP reads this file, it will assign mypassword to the variable and read youcantguessit; as code which will throw an error.

Example 2: Bob uses mypassword'youcan'tguessit for his password during setup. Modx writes the following to config.inc.php:

$database_user = 'modx_database';
$database_password = 'mypassword'youcan'tguessit';
$database_connection_charset = 'utf8';

You can see that $database_password is encapsulated with single quotes. However, due to the lack of sanity checking, the string itself contains two single quotes that are separated by text. When PHP reads this file, it will assign mypassword to the variable and read youcan as code, then read tguessit; as a string. This, again, will throw an error.

I propose that a sanity check be set in place to either restrict the characters allowed in the password, or more user-friendly, to iterate through the submitted password and escape any single quotes so as to maximize the password character set as well as eliminate the possibility of this error occurring.

Note: This issue, as of right now, only applies to single quotes as they are used to encapsulate the string in the file. Double quotes (") are exempt from this behavior as they would be considered part of the string by PHP.

[alroniks]

Related to https://github.com/modxcms/revolution/issues/5765

[wuuti]

Uh. Isn't this a big security issue??? One could easily pipe in php code there?

[cedwardsmedia]

Exactly. Granted, the risk in this specific case is minimal considering its only found in the setup code. Once setup is complete and the directory removed, the code responsible for this issue is no longer there.

Sent from my iPhone

On Jul 8, 2015, at 01:01, Jens Külzer notifications@github.com wrote:

Uh. Isn't this a big security issue??? One could easily pipe in php code there?

— Reply to this email directly or view it on GitHub.

[wuuti]

Yeah. But considering the fact, that shorthly after an update comes out it is likely that a setup directory would pop up at a randomly picked modx installation, and then is not secured for a certain period of time (at least as long as the admin needs to go through it) this is quite important. Not considering the fact, that we ideally assume that all setup folders are really deleted after setup. Which shouldn't be assumed.

[cedwardsmedia]

Correct. Then again, when the site owner has had it drilled into their head to remove the directory and they don't... It's no substitute for proper design and development. However, there also comes a point when you can't babysit the user and prepare for every possible stupid decision they make. That's why it's important to educate users on proper installation and post installation steps to ensure the most secure setup possible with the current codebase.

Regardless, this is an issue that needs patched quickly.

[OptimusCrime]

While this might be an issue I don't see how to fix this. I agree that we should do something about the quote that crashes the system, but the rest is really up to the developer and/or sysadmin. There is no good way to avoid people piping PHP code into the config file.

If you upload the setup-directory to a server, be sure to delete it. Modx even reminds you about this afterwards. If you ignore this, well, that is really your problem.

[cedwardsmedia]

I agree completely. I have an idea of how to fix this but I'll have to look at the code responsible before I can be sure my idea will work. I'll poke at it this weekend and see if I can offer a fix.

[OptimusCrime]

Should be pretty doable by either disallowing a single quote character, or by looking for a quote character in the string and do addslashes / stripslashes before returning and adding the password to the system.

[cedwardsmedia]

That's what I'm thinking. A simple function to sanitize the input before moving forward.

[cedwardsmedia]

At the moment, I'm looking through the code for the setup process and I can't seem to find the portion of code that actually passes the database password onward in the workflow. Anyone have any ideas?

[OptimusCrime]

I think setup/includes/runner/modinstallrunner.class.php#L153 might be the correct file and method.

[cedwardsmedia]

To be honest, after poking around at it for awhile, I almost think the best and most efficient option would simply be to disallow single or double quotes in the password. I say that mostly because if we addslashes and stripslashes, while this solves this particular issue, we then create another issue if the user enters a password which intentionally has a \ before a single or double quote. This, unfortunately, would introduce a new bug in which the database password in MySQL may legitimately be something such as change\'me. However, by doing a stripslashes() on the password when reading from the config, Modx would then try using change'me as the password, which MySQL would refuse.

This is a bit annoying because I spent quite some time tweaking to write the original patch we suggested. Unfortunately, after doing so, I discovered this new issue. That being said, it's pointless to submit a patch that corrects one flaw but creates another. In this case, I suggest MODX simply refuse to accept a password containing any single or double quotes - any other character would be just fine as nothing else would recreate this issue.

[OptimusCrime]

Yeah, I guess you're right

[Mark-H]

Hmm maybe I'm just being dense here, but addslashes also escapes back slashes. So if the password is deliberately change\'me, that would be turned into change\\\'me which PHP interprets as change\'me, making it work as expected?

I don't see why we'd need stripslashes, we just want to make sure the data is "safe" to write to the config file.

[cedwardsmedia]

Correct but we also have to read the data from the config file so we have to remove the escapes or they will be read as part of the password.

Does addslashes escape existing backslash chars in the supplied string? I haven't used it much so I don't know for sure. Either way, we still need to stripslashes when we read the config to ensure we don't supply the MySQL calls with a bad password.

If addslashes does, in fact, escape existing backslash chars, then perhaps stripslashes would strip the proper ones as well. If so, we could likely still use the original idea to patch this flaw.

[cedwardsmedia]

Just to clarify, we would still stripslashes because otherwise, PHP would read any escaping slashes to be part of the string as opposed to an escape. So without it, "change\'me" would be read as-is, \ and all, even though it's an actual escape. So if we're going to escape things with addslashes, we'll have to use stripslashes as well or we'll be left with broken passwords being read from the config.

[Mark-H]

Either way, we still need to stripslashes when we read the config to ensure we don't supply the MySQL calls with a bad password.

I really don't think we need stripslashes at all. The data is inserted into the tpl like this:

$database_password = '{database_password}';

which causes the problem that when {database_password} is change'me, the resulting string is

$database_password = 'change'me';

which is obviously incorrect, and what spurred this issue.

By calling addslashes, change'me is turned into change\'me, which results in the following, valid, string:

$database_password = 'change\'me';

That's valid php, and when doing a var_dump or echo of that variable (or like, pass it to mysql), it will actually be represented as change'me, without the slash. We just need to make sure it's valid PHP in the config file.

Likewise, if change\'me with addslashes results in the following valid php:

$database_password = 'change\\\'me';

but when looking at the value of that variable when read, it will show as change\'me.

[Mark-H]

Keep in mind the difference between $database_password = 'single quotes' and $database_password = "double quotes", if it were stored with double quotes you would be correct, but as this is written with single quotes, I don't think you are.

[Mark-H]

In 'change\\\'me' the first slash escapes the second, leaving a literal string \. The third slash escapes the single quote so that is treated as a string instead of "end value". So without changing the actual string stored in the variable, it is escaped properly.

[cedwardsmedia]

Ah I see where you're coming from now. In that case, I have a patch done that corrects the data written to the config file. It's about 2-3 lines and probably the worst possible way to achieve the goal, but it works.

I'll push my change to my fork later this weekend. I still need someone else to help me test it and double check me to make sure it doesn't introduce any new problems. Once that's done, I'll submit a pull request.

[opengeek]

Thanks for working this out!

[cedwardsmedia]

No worries @opengeek. I'm glad I can help contribute to something. This will actually be my first technical contribution to a project outside my personal circle of influence, so I'm hoping it works right for everyone.

As of right now, I have a hotfix for this issue in commit 75c62400d4. I need a few volunteers to help me test and ensure this fix works in various environments. Once that's confirmed, I'll submit a pull request.

[Mark-H]

Fixed in 2.3.6