SQL injection vulnerability in plugins/change-password-custom-sql/

[rolfen]

RainLoop version, browser, OS:

Latest (1.12.1)
Browser / OS version not relevant.

Expected behavior and actual behavior:

This affects Rainloop change password custom mysql plugin

Single quotation marks should have no effect on the query. The user should be able to use quotation marks in his password.

Also single quotation marks should be allowed in email addresses according to RFC5322

Steps to reproduce the problem:

Configure the plugin in admin, in the "SQL statement" input, delimit the new password input using single quotation marks such as ':newpass'

When the user tries to change the password to one containing single quotation marks, he will get a "Could not change password" error. rainloop logs show an SQL syntax error due to the ":newpass" placeholder being directly replaced by the password input. For example if the user tries to set his new password to It'sNotOK then rainloop will try to execute a query that could contain password='It'sNotOK' causing an SQL syntax error due to the unexpected single quote.

Details:

Extract from ChangePasswordCustomSqlDriver.php

$old = array(':email', ':oldpass', ':newpass', ':domain', ':username', ':table' );
$new = array($sEmail, $sPrevPassword, $sNewPassword, $sEmailDomain, $sEmailUser, $this->mTable);

$this->mSql = str_replace($old, $new, $this->mSql);

$update = $conn->prepare($this->mSql);
$mSqlReturn = $update->execute(array());

As you can see above, the PDO SQL injection protection facilities are not used. Instead, a simple str_replace is used to replace placeholders with values, some of which come directly from user input.

This is likely a privilege escalation vulnerability, allowing the user to craft operations which he would normally not be allowed (delete tables, etc). However I did not do further testing.

Solutions:

User input should be properly escaped before being passed to the database.

Potentially useful details: https://stackoverflow.com/a/12202218/370786

Possible quick and dirty fix: https://evertpot.com/escaping-mysql-strings-with-no-connection-available/

[rolfen]

Note:

"Proper" SQL injection protection requires using PDO bind.

$update = $conn->prepare($this->mSql);
$update->bindValue(':email', $sEmail);
$update->bindValue(':oldpass', $sPrevPassword);
etc.

PDO adds quotes around the bound values.

If the SQL statement in the plugin config already contains quotes then we will have repeated quotes ('') and syntax errors.

Therefore a "proper" fix would break backwards compatibility of the plugin with current setups.

Possible workaround is a "compatibility fix": automatically remove any quotation marks around placeholders before passing the the SQL statement to PDO. For example, ':username' would become :username.

Placeholders are easy to detect so this should be doable, maybe using regular expression replace.

I tried coming up with something to do a pull request but PHP threw lots of junk at me and I ran out of time.