Security: SQL injection is possible though table names, column names, etc.

SQL injection is possible through table names, table aliases, column names, indexes, and sequences. As their names are not quoted properly.

The quoteName function in

Aura.SqlQuery/src/Common/Quoter.php

Line 88 in 4697be9

$seps = array(' AS ', ' ', '.');

doesn't care about new lines or tab characters inside a name of a column/table.

require_once "../vendor/autoload.php";

use Aura\SqlQuery\QueryFactory;

$queryFactory = new QueryFactory('mysql');
$select = $queryFactory->newSelect();
$select
    ->cols(["id from members limit 1;drop table pentest; SELECT *"]) // tabs or new lines instead of spaces
    ->from('members');

echo ($select->__toString());

Will generate this query

SELECT
  id	from	members	limit	1;drop	table	pentest;	SELECT	*
  FROM
  `members`

This is a problem when the table or column names, etc can be directly passed in by the user.

While it's possible the escaping code here could be improved, I don't think it's really a security issue in itself.

You should always filter any table or column names coming from user input because even if properly escaped to avoid this specific issue, they could still be valid names of tables or columns containing information the user might not normally have access to.

It's also possible to mitigate this issue when using MySQL by disabling PDO::MYSQL_ATTR_MULTI_STATEMENTS

Additionally, there are cases where you want to use complex column definitions (containing functions, aliases or other constructs such as CASE) that I think make implementing more complex escaping here not so straight forward.

Personally I don't think I've ever considered this library to be for security purposes in any real way. It's for aiding in the construction of (dynamic) queries. You should still pay careful consideration to security.

@peterjakubek thanks for the report. I try to be attentive to security issues.

I have to say I am with @AllenJB on this -- if one accepts user input as a table name, column name, etc., then you are asking for trouble. (The same applies if you accept user input as full WHERE or HAVING conditions directly, for example.)

Even so, if you can provide (by link or PR) some identifier-escaping logic that you would find satisfactory, I would be very happy to review it for possible inclusion here.

Thank you @AllenJB and @pmjones for responding so quickly.

I noticed the issue while working on a huge enterprise production system where the developer using the library didn't know it wasn't fully escaped and they passed in the user-supplied data directly.

One thing I would suggest is to use the cols method with strict escaping and implementing something like colsRaw for custom queries. So that the person using it can immediately expect it's unescaped.

I'm aware that any change should be backwards compatible.

Perhaps checking in the replaceName method

Aura.SqlQuery/src/Common/Quoter.php

Line 228 in 4697be9

protected function replaceName($name)

and making sure that the $name variable doesn't contain $this->quote_name_prefix or $this->quote_name_suffix and throwing an exception if it does could be sufficient.

I'll have a closer look in the morning.

I'll have a closer look in the morning.

👍

Hello,

thank you for your comments on the issue. I hardened it on the client-side by replacing new lines and tab characters with spaces and making sure that there are no multiple queries.

On the library side, it's a question of how flexible you want it to be. Optionally you can restrict inputs with quoting characters or the presence of the semicolon character.

I guess it's always a matter of flexibility vs security.