Home » Authentication » Auth » Bug #8407

[2006-08-10 03:43 UTC] pear at adamharvey dot name

Description:
------------
The four database containers that exist in Auth (DB, DBLite, MDB and MDB2) currently use the table and field names as is without any quoting. Furthermore, the documentation at present doesn't actually inform the developer that the table and field names should be quoted.

The linked patch quotes identifiers before using them in SQL queries, thereby avoiding any possibility of SQL injection. (Any injection attack would still require some seriously sloppy code on the part of developer using Auth, but this closes the door fairly decisively.)

This patch *will* break BC for the case where Auth users are already quoting identifiers before passing them to the container. The behaviour of the db_fields option has also changed somewhat, as the case where a string ", field_a, field_b" was given for this option will now break. Note that it wouldn't have worked without the preceding comma anyway; the preferred way of doing this has always been array('field_a', 'field_b') to my mind.

In short, this is probably a candidate for Auth 2 rather than 1.x.

Test script:
---------------
Patch is at: http://www.adamharvey.name/patches/Auth-container-quoting.patch

[2006-08-11 01:15 UTC] aashley at php dot net

Patch merged to CVS, rolling RC to get feedback from users.

[2006-08-16 23:23 UTC] ohm at morishima dot net

The simplest example below, just like in the Introduction - Auth tutorial in the documentation:

<?php
require_once "Auth.php";

$dsn = "pgsql://user@localhost/database";
$a = new Auth("DB", $dsn);
$a->start();
if ($a->checkAuth()) {
echo "login completed.\n";
} else {
echo "login incorrect.\n";
}
?>

does not work. PostgreSQL complains that

ERROR: zero-length delimited identifier at or near """" at character 32

and the SQL in question was:

SELECT "username", "password", "" FROM "auth" WHERE "username" = 'testuser'

I didn't test on MySQL but it's likely similar error will be produced.

This is because the default value of options['db_fields'] is '' (that is, not 'unset') and the patched _quoteDBFields() just checks the value is set or not, and quoteIdentifier($str) (DB/common.php) does not check $str is empty string or not.

Quick fix, add 2 lines to _quoteDBFields():

function _quoteDBFields()
{
if (isset($this->options['db_fields'])) {
if (is_array($this->options['db_fields'])) {
$fields = array();
foreach ($this->options['db_fields'] as $field) {
if (!$field) continue; // <- added
$fields[] = $this->db->quoteIdentifier($field);
}
return implode(', ', $fields);
} else {
if (!$this->options['db_fields']) return ''; // <- added
return $this->db->quoteIdentifier($this->options['db_fields']);
}
}

return '';
}

[2006-08-17 01:04 UTC] aashley at php dot net

Thanx for catching that Akitoshi, fix in CVS.

[2006-08-21 02:20 UTC] aashley at php dot net

Thank you for your bug report. This issue has been fixed
in the latest released version of the package, which you can download at
http://pear.php.net/get/Auth