Repair Settings can't handle " in the DB password

[Arantor]

So I've been working on a little site that I wanted to launch, anyway, the DB password has a " in it.

repair_settings.php chops the password off, making it impossible to actually use, and even if you manually override it each time with the new password, the saved password in Settings.php escapes the " - but since it's inside ' the escape character is included as a literal, so that doesn't work either.

[emanuele]

An htmlspecialchars should fix that one I think hope.

Code (find) Select Expand


<input type="text" name="', $info[0], 'settings[', $setting, ']" id="', $setting, '" value="', isset($settings[$setting]) ? $settings[$setting] : '', '" size="', $settings_section == 'path_url_settings' || $settings_section == 'theme_path_url_settings' ? '60" style="width: 80%;' : '30', '" class="input_text" />';

Code (replace with) Select Expand


<input type="text" name="', $info[0], 'settings[', $setting, ']" id="', $setting, '" value="', isset($settings[$setting]) ? htmlspecialchars($settings[$setting]) : '', '" size="', $settings_section == 'path_url_settings' || $settings_section == 'theme_path_url_settings' ? '60" style="width: 80%;' : '30', '" class="input_text" />';

[emanuele]

And while we are looking at passwords and repair settings, there is also a problem with semi colons that break the mysql password.
I was thinking to change the regexp:

Code Select Expand

^[$]([a-zA-Z_]+)\s*=\s*(["\'])?(.*?)(?:\\2)?;
to

Code Select Expand

^[$]([a-zA-Z_]+)\s*=\s*(["\'])?(.*?)(?:\\2)?([^'].*;)
but since regexp are scary...

[Arantor]

Well, now that I have my setup all working I'm a bit reluctant to test it with anything

But easy to reproduce, just set up a MySQL account with a " in the middle of the password and see what happens.

Not sure about that regex, is complicated.

[MrPhil]

but since regexp are scary...

^[$]([a-zA-Z_]+)

Starting at the beginning of the line (do we just assume that everything starts at column 1?), look for a literal dollar sign (could also be \$) followed by a variable name (one or more letters and underscores (aren't digits allowed after the first alpha?). Stick the name (less the $) into variable $1.

Note, since in the future we might want variable names with digits, change this to

Code Select Expand

^\$([a-zA-Z_][a-zA-Z0-9_]*)
As we're using Perl style regexps, this can be simplified to

Code Select Expand

^\$([a-zA-Z_]\w*)
To allow whitespace before a variable name,

Code Select Expand

^\s*\$([a-zA-Z_]\w*)
or you could trim() the element first. Since we're using Perl style, we could also just say a-z and stick an "i" modifier (case insensitive) at the end of the pattern, provided we don't have any places where it might be a case sensitive match.

\s*=\s*

any whitespace, equals sign, any whitespace

(["\'])?

optional " or ' to start the value string. Put into $2 variable (if no match, $2 should be an empty string). Numeric values don't need quotes, so they will be missing here.

(.*?)

0 or more characters, non-greedy match (don't match the closing delimiter here, which comes next, or the semicolon). If the field contains a character matching the string delimiter (if used), it will stop here. Note that this allows $varname=; which is invalid PHP, but presumably that will be caught elsewhere.

(?:\\2)?

match of $2 (the opening delimiter, if it was used). It is "capture less" (not put into $3). The second ? makes it optional for the closing, which I'm not sure is correct. I don't think the second ? should be there, as it permits $var = 'value;. Someone ought to look at that carefully.

;

a literal semicolon is next to match

Note that nothing beyond the ; is matched. As there could be whitespace and even comments following, it would not be good to put a $ here (force match to end of string). I will have to look over repair_settings.php in more detail to see if we need to save leading whitespace and trailing comments for the rewrite of the file.

Code Select Expand

^[$]([a-zA-Z_]+)\s*=\s*(["\'])?(.*?)(?:\\2)?([^'].*;)
This does not handle any leading whitespace before the variable name, nor does it allow variable names containing digits. The closing quote on a string is optional, which is not good. Then you match any one non-' character followed by any number of characters followed by a ;. I suspect that's not what you're aiming for.

We have two problems we're trying to solve:

• A delimiter ' or " inside a string but not escaped. How can it be in a PHP string in the first place? If you happen to have a password with a " in it, either use ' to delimit the string, or escape a " in the middle: \". Is the problem that the escaped \" is not being treated as an ordinary character in this regexp, but as two characters \ and "? Please give an example.
• Semicolons inside the string are doing something bad? Please elaborate. They should not be matched until after the closing delimiter is matched. A semicolon could only be in a string, which means there must be ' or " delimiters around it.

I will have to do some experimenting later to see if a Settings.php line with "blah\"blah" is read into the array element as "blah"blah" (escape disappears and the password is chopped off as "blah"). If that's what's happening, the cure could be difficult. I'll have to think about it. I'll report back later unless someone finds a good solution in the next few hours. We may end up with additional code to split up the line and process the pieces using regexp, but not be able to properly handle the whole thing in one regexp.

The following is not tested, but should match

• lines with leading whitespace (the leading whitespace is not saved)
• PHP variables with digits in them (in $1)
• properly constructed strings (' ' or " ") (in $3). A delimiter within the string may still be a problem, even if it's escaped in Settings.php.
• any trailing goop after the semicolon, if you wish to retain comments, etc. (in $4)

Code Select Expand

^\s*\$([a-zA-Z_]\w*)\s*=\s*(["\']?(.*?)(?:\\2);(.*)$

Note that repair_settings.php suffers from the same race condition that SMF's attempt to rewrite the Settings.php on database error does! It empties out the file first, which is a stupid thing to do (needed for some ancient server software). Please consider getting rid of this step in both places!!!!!

[Arantor]

Given that this is dealing with Settings.php, we don't need to be nearly as generous as some of the conditions you've outlined... there are no variables with names with digits in, there should be no leading spaces etc. - no need to worry about them.

[MrPhil]

@Arantor -- can you give an example where the db_password fails with a " in it? What are the string delimiters... ' or "? Is the " escaped \" (mandatory if the string delimiters are ")? Is it PHP that's throwing an error, or MySQL (that it's received a bogus password)? stripslashes() appears to be applied to everything. Is it only the db password that has problems, or does any string containing a ' or " (escaped or not) have a problem? Don't forget that when repair_settings.php rewrites the Settings.php file, it always uses ' as the string delimiter. I added a call to addslashes() for the string data -- maybe that's needed?

I looked through the code and the regexp, and I see that an escape \ will be ignored, which is bad news if the escaped character is the same as the delimiters. I had to write some code to go through the string and skip over escaped characters, so it would know where the string really ended. Since a value string might have both ' and " in it, we just can't scold users to remember to always use the "other" character as the delimiter (and the rewrite is '). We have the delimiter available in $match, so the rewrite of lines could use the original delimiter.

@emanuele, can you describe an example of ; breaking a password? What happens to ; in other strings?

In repair_settings.php, find

Code Select Expand

if (substr($settingsArray[$i], 0, 1) == '$')
{
preg_match('~^[$]([a-zA-Z_]+)\s*=\s*(["\'])?(.*?)(?:\\2)?;~', $settingsArray[$i], $match);
if (isset($match[3]))
{
if ($match[3] == 'dirname(__FILE__)')
$settings[$match[1]] = dirname(__FILE__);
elseif ($match[3] == 'dirname(__FILE__) . \'/Sources\'')
$settings[$match[1]] = dirname(__FILE__) . '/Sources';
elseif ($match[3] == '$boarddir . \'/Sources\'')
$settings[$match[1]] = $settings['boarddir'] . '/Sources';
else
$settings[$match[1]] = stripslashes($match[3]);
}
}

and change it to

Code Select Expand

if (preg_match('~^\s*\$~', $settingsArray[$i]))  // allow ws before $ in var name
{
preg_match('~^(\s*)\$([a-zA-Z_]\w*)\s*=\s*(["\'])?(.*)~', $settingsArray[$i], $match);
                        // match[1] is any leading whitespace, if you need it
                        // process match[4] to find its end, and split off remainder to match[5]
                        $string = $match[4];
                        for ($j=0; $j<strlen($string); $j++) {
                               $char = substr($string, $j, 1);
                               if ($char == '\\') {
                                       $j++;  // skip escaped character, too
                                       continue;
                               }
                               if ($char == $match[3]) { // is non-empty delimiter (and not escaped)?
                                       $match[4] = substr($string, 0, $j); // truncate string here
                                       //$match[5] = substr($string, $j+1);  // remainder including ; to element 5
                                       break;
                               }
                               if ($match[3] == '' && $char == ';') {  // no string delimiter, ran into semi
                                       $match[4] = substr($string, 0, $j); // truncate non-string here at ;
                                       //$match[5] = substr($string, $j);    // remainder including ; to element 5
                               }
                       }
                       $match[4] = trim($match[4]);
                       //$match[5] = trim($match[5]);
if (isset($match[4]))  // if not set, it's a failure mode... just keeps from blowing up PHP
{
if ($match[4] == 'dirname(__FILE__)')
$settings[$match[2]] = dirname(__FILE__);
elseif ($match[4] == 'dirname(__FILE__) . \'/Sources\'')
$settings[$match[2]] = dirname(__FILE__) . '/Sources';
elseif ($match[4] == '$boarddir . \'/Sources\'')
$settings[$match[2]] = $settings['boarddir'] . '/Sources';
else
$settings[$match[2]] = stripslashes($match[4]);
}
}

Find

Code Select Expand

if (substr($settingsArray[$i], 0, 1) == '$' && preg_match('~^[$]([a-zA-Z_]+)\s*=\s*(["\'])?(.*?)(?:\\2)?;~', $settingsArray[$i], $match) == 1)
$settings[$match[1]] = stripslashes($match[3]);

foreach ($file_updates as $var => $val)
if (strncasecmp($settingsArray[$i], '$' . $var, 1 + strlen($var)) == 0)
{
$comment = strstr($settingsArray[$i], '#');
$settingsArray[$i] = '$' . $var . ' = \'' . $val . '\';' . ($comment != '' ? "\t\t" . $comment : '');
}
(Why is strncasecmp() used? That makes it case-insensitive, which doesn't seem to make sense.)

and change to

Code Select Expand

if (preg_match('~^(\s*)\$([a-zA-Z_]\w*)\s*=\s*(["\'])?(.*)~', $settingsArray[$i], $match)) {
                        // match[1] is any leading whitespace, if you need it
                        // process match[4] to find its end, and split off remainder to match[5]
                        $string = $match[4];
                        for ($j=0; $j<strlen($string); $j++) {
                               $char = substr($string, $j, 1);
                               if ($char == '\\') {
                                       $j++;  // skip escaped character, too
                                       continue;
                               }
                               if ($char == $match[3]) { // is non-empty delimiter (and not escaped)?
                                       $match[4] = substr($string, 0, $j); // truncate string here
                                       //$match[5] = substr($string, $j+1);  // remainder including ; to element 5
                                       break;
                               }
                               if ($match[3] == '' && $char == ';') {  // no string delimiter, ran into semi
                                       $match[4] = substr($string, 0, $j); // truncate non-string here at ;
                                       //$match[5] = substr($string, $j);    // remainder including ; to element 5
                               }
                       }
                       $match[4] = trim($match[4]);
                       //$match[5] = trim($match[5]);
$settings[$match[2]] = stripslashes($match[4]);
                }

foreach ($file_updates as $var => $val)
                        // compare just up through variable name. was strncasecmp()... why case insensitive?
                        if (preg_match('~^\s*\$'. $var .'~', $settingsArray[$i]))
{
                                // rewrite line in a standard format, and add slashes to string values
                                $val = str_replace('\\', '\\\\', $val);
                                $val = str_replace("'", "\\'", $val);
$comment = strstr($settingsArray[$i], '#');
$settingsArray[$i] = '$' . $var . ' = \'' . $val . '\';' . ($comment != '' ? "\t\t" . $comment : '');
}

This has not been more than very lightly tested (just segments in a test frame, not the whole thing end to end), so play with it for a while before turning it loose on a real live system! As with the original code, it assumes that the contents of Settings.php is legitimate PHP code, and that any syntax errors have already be caught and corrected (i.e., not extremely robust!). I've added a little flexibility in the ability to handle leading whitespaces and variable names with digits.

I didn't do anything (yet) about emptying out the file before rewriting it. That's a horrendous bug in the updating of Settings.php for database errors, and might cause problems here too.

Mod: replaced addslashes() in rewriting with str_replace() to only escape ' and \. Output strings should always be ' delimited.
Mod 2: do \ ->\\ before ' -> \'

[Arantor]

@Arantor -- can you give an example where the db_password fails with a " in it?

I thought I'd already explained. I started out setting the password manually myself by editing Settings.php, and needed repair_settings.php only to fix the paths in the database, namely the ones in the settings and themes tables.

The password is inserted into the first page of repair_settings.php as is, meaning it gets truncated (and on saving, everything fails because now the DB password is wrong.

If the correct password is then supplied, it is saved into the file with a \ in it, which in that case is wrong, it should not be so, because it is in single quotes, meaning that the password supplied to the DB is no longer blah"blah but now blah\"blah - no escape character should be inserted into single-quote strings unless the character being escaped is a ' or a \ itself - it is always treated as a literal in there.

I didn't look at any other string, as it was only the DB password field that had a " in it.

[emanuele]

The root cause of the " breaking the password is simply because the html would look like this:

Code Select Expand

<input value="passw"ord" .... />

The root cause of the ; breaking the password is that the regexp tries to read the line and extracts the password stopping at the first ; that it finds (since ; is the end of line delimiter in php). So the solution I tried to implement in the regexp is: do the same thing, but it doesn't work...I think I'll simply change it to:

Code Select Expand

^[$]([a-zA-Z_]+)\s*=\s*(["\'])?(.*?)(?:\\2)?;$

[Arantor]

The root cause of the " breaking the password is simply because the html would look like this:

That's certainly why it truncates it, but that's not why it also proceeds to try escaping said " later on when actually saving.

[MrPhil]

Even better, if you have confirmed that the value attribute string is terminated early due to a " in it,  find

Code Select Expand

<input type="text" name="', $info[0], 'settings[', $setting, ']" id="', $setting, '" value="', isset($settings[$setting]) ? $settings[$setting] : '', '" size="', $settings_section == 'path_url_settings' ? '60" style="width: 80%;' : '30', '" />';

and change to

Code Select Expand

<input type="text" name="', $info[0], 'settings[', $setting, ']" id="', $setting, '" value="', isset($settings[$setting]) ? addslashes($settings[$setting]) : '', '" size="', $settings_section == 'path_url_settings' ? '60" style="width: 80%;' : '30', '" />';

There's another place in the code where addslashes() is already called, but I'm not sure if it has any bearing on this problem:

Code Select Expand

// Add slashes, as long as they aren't already being added.
if (get_magic_quotes_gpc() == 0)
{
foreach ($_POST as $k => $v)
{
if (is_array($v))
foreach ($v as $k2 => $v2)
$_POST[$k][$k2] = addslashes($v2);
else
$_POST[$k] = addslashes($v);
}
}

[emanuele]

Oh...yep you are right...well in fact I can't even install with a " in the password...
And the problem apparently is:

Code Select Expand

// Add slashes, as long as they aren't already being added.
if (!function_exists('get_magic_quotes_gpc') || @get_magic_quotes_gpc() == 0)
foreach ($_POST as $k => $v)
$_POST[$k] = addslashes($v);


Honestly I'm dubious if consider the password a special case, or just escape only single-quotes for everything...

* emanuele needs more testing.

@MrPhill if you use addslashes in the html the result will simply be:

Code Select Expand

<input value="pass\"wd" ... />
that is anyway broken html, see my first answer for a better fix.

[Arantor]

or just escape only single-quotes for everything...

That's the correct behaviour. The password is assumed to be that seeing how it doesn't remove the slashes before sending it on to the DB connection.

[emanuele]

Okay, it makes sense.

Changing the above code to:

Code Select Expand

if (function_exists('get_magic_quotes_gpc') && @get_magic_quotes_gpc() != 0)
foreach ($_POST as $k => $v)
$_POST[$k] = stripslashes($v);

foreach ($_POST as $k => $v)
$_POST[$k] = addcslashes($v, '\'');

in both repair_settings and install.php

[MrPhil]

You need to always be careful not to add escapes (backslashes) too early in code, or at least be aware whether or not a string has already been escaped at any given point of use. My preference is to leave strings unescaped (run stripslashes) until the point they are used, and then do an addslashes() or whatever is appropriate for their intended usage, at the last moment. You get into all sorts of problems when you escape a string right away and then later forget to partially or completely remove escapes (different uses need different kinds of escaping). Just my 2 cents... you may prefer doing it other ways.

[emanuele]

Okay, found a potentially valid alternative for the regex:

Code Select Expand

^[$]([a-zA-Z_]+)\s*=\s*(?:(["\'])(.*?["\'])(?:\\2)?|(.*?)(?:\\2)?);
What does it mean?
Instead of try to do everything in one go, let's split the two cases and deal with them one at a time:

Code Select Expand

(["\'])(.*?["\'])(?:\\2)?
for strings in the form: $setting = 'texthere'; # anything else here
and

Code Select Expand

(.*?)(?:\\2)?
for settings like maintenance mode (i.e. $maintenance = 0; )
I pushed that to my "tools" repo.