2.0 DB: Sanitize table names etc.?

Franz · 2011-09-15 03:54:35

In some of the utility functions in the database layer, things like table names and column names are used directly in a query, without any validation or sanitation.

Now, of course there should not be user-generated content anywhere near these. But still, especially with having this API available for use in extensions, I smell the risk of SQL injections increasing.

So, how should we go about this?

FSX · 2011-09-15 15:55:32

It depends on where database tables come from. I would not sanitize them all the time.

Mpok · 2011-09-15 19:41:32

I think ur nose is pretty good, Franz…

FSX wrote:

It depends on where database tables come from. I would not sanitize them all the time.

But HOW can u determine that ?

More over on the use by the application itself, the database layer can be used by third parties applications (just as it is for version 1.2 and/or 1.4).
The more 'secure" it is, the more 'useful' it is for developers (even if FluxBB itself doesn't need this 'over-checking' on this code).

So, this part should, IMO, be as secured/sanitized as possible. I can imagine a little down in performance, but it would be minor (some 'trim' or 'pun_htmlspecialchars', maybe zéro, one or two 'preg_match'), and the advantages in security worth well the point.
For the application itself, for the future extensions, for the applications based on FluxBB.

Franz · 2011-09-16 02:08:27

FSX wrote:

I would not sanitize them all the time.

I honestly think this would make things even worse.

Inconsistency would make the danger even worse when it comes to third-party usage. We could, of course, properly document everything, but that hasn't exactly been our strongest point in the past, so I wouldn't count on it.

@Mpok:
When it comes to performance, I think things shouldn't get too bad, so you're right. As we still haven't yet thrown away the idea of caching generated queries, this would actually go in line with that, as the checks would only have to be done when caching queries.

Smartys · 2011-09-16 03:13:33

*PLEASE* sanitize your inputs. Thank you

Edit: Because this is what happens when you don't.

Last edited by Smartys (2011-09-16 03:14:47)

Franz · 2011-09-16 03:28:18

Ok.

So we do all agree on that. Great.

How should we go about this, then? For example, should we check that a table name only contains letters, numbers and underscores (or whatever would be appropriate)? Should we then give an error if the condition is not met (obviously just stripping out bad characters would not make sense e.g. when checking whether a table exists etc.).

Smartys · 2011-09-16 03:39:35

You can do exactly what ActiveRecord does: per-database quoting.

For MySQL, where `s can surround table/column names, the code looks something like this

$name = '`' . str_replace('`', '``', $name) . '`';

PostgreSQL: https://github.com/rails/rails/blob/mas … er.rb#L466
MySQL: https://github.com/rails/rails/blob/mas … er.rb#L183
SQLite: https://github.com/rails/rails/blob/mas … er.rb#L184

Last edited by Smartys (2011-09-16 03:42:02)

Franz · 2011-10-19 23:13:24

Ok, added a ticket and marked it as "bug".

Franz · 2011-11-02 23:31:51

Sad. This thing causes problems.

I just implemented quoting tables and columns that we put directly into queries.

That is more than needed, but I was going to do it to be absolutely on the safe side. Now, however, we are encountering problems with queries like this one:

$query = $db->select(array('subject' => 't.subject', 'closed' => 't.closed', 'num_replies' => 't.num_replies', 'sticky' => 't.sticky', 'first_post_id' => 't.first_post_id', 'forum_id' => 'f.id AS forum_id', 'forum_name' => 'f.forum_name', 'moderators' => 'f.moderators', 'post_replies' => 'fp.post_replies', 'is_subscribed' => '0 AS is_subscribed'), 'topics AS t');
$query->innerJoin('f', 'forums AS f', 'f.id = t.forum_id');
$query->leftJoin('fp', 'forum_perms AS fp', 'fp.forum_id = f.id AND fp.group_id = :group_id');
$query->where = '(fp.read_forum IS NULL OR fp.read_forum = 1) AND t.id = :tid AND t.moved_to IS NULL';
$params = array(':group_id' => 1, ':tid' => 1);
$result = $query->run($params);

This would now result in something like this:

SELECT ... FROM `forum_topics AS t` ...

Do you see where I am going?

To prevent this, I would either have to build some type of complicated workaround (e.g. have an extra field for the table alias or allow for table to be an array consisting of table name and alias). Or, simply drop all the escaping and let the developers of extensions (the core would obviously be safe) take on the small risk that's left.

What to do?

daris · 2011-11-03 07:06:46

Hmm, a little bit more complicated but working example:

function quoteTable($str)
{
	if (($pos = strpos(strtoupper($str), ' AS ')) !== false)
		return substr_replace($str, quoteTable(substr($str, 0, $pos)), 0, $pos);

	return '`'.str_replace(array('`', '.'), array('``', '`.`'), $str).'`';
}

echo quoteTable('fluxbb-20.forum_topics AS t').'<br />';
echo quoteTable('fluxbb-20.forum_topics as t').'<br />';
echo quoteTable('fluxbb-20.forum_topics');

`fluxbb-20`.`forum_topics` AS t
`fluxbb-20`.`forum_topics` as t
`fluxbb-20`.`forum_topics`

Last edited by daris (2011-11-03 07:14:42)

Franz · 2011-11-03 08:48:57

Works well, but not for alias definitions without "AS".

I've written unit tests, once I commit them, you can have a play.

Franz · 2011-11-03 09:26:06

Never mind, this is close to impossible.

The way you suggested adds lot of overhead and isn't very secure (as long as we're talking about SQL injection), as I could add everything I wanted behind the "AS" and it would simply be executed.

Also, we would have to take care of things like "SUM(t.num_topics) AS total_topics", which would have to look like "SUM(`t`.`num_topics`) AS total_topics.

So, as sad as that is, we will not escape table and column names that are added directly into queries.

It would have been nice as an additional layer of security, but very likely this is not necessary.

--

This is what the unit tests looked like in the end (for MySQL) - and they were not complete yet:

	public function testQuoteTable()
	{
		$this->assertEquals('`field``a`', $this->db->quoteTable('field`a'));
		$this->assertEquals('`fluxbb-20`.`forum_topics` AS t', $this->db->quoteTable('fluxbb-20.forum_topics AS t'));
		$this->assertEquals('`fluxbb-20`.`forum_topics` as t', $this->db->quoteTable('fluxbb-20.forum_topics as t'));
		$this->assertEquals('`fluxbb-20`.`forum_topics` t', $this->db->quoteTable('fluxbb-20.forum_topics t'));
		$this->assertEquals('`fluxbb-20`.`forum_topics` t', $this->db->quoteTable('fluxbb-20.forum_topics t'));
		$this->assertEquals('`fluxbb-20`.`forum_topics`', $this->db->quoteTable('fluxbb-20.forum_topics'));
	}

	public function testQuoteColumn()
	{
		$this->assertEquals('`a`.`np` AS num_posts', $this->db->quoteColumn('a.n_p AS num_posts'));
		$this->assertEquals('SUM(`t`.`num_posts`) AS total_posts', $this->db->quoteColumn('SUM(t.num_posts) AS total_posts'));
		$this->assertEquals('SUM(`t`.`num_posts`) as total_posts', $this->db->quoteColumn('SUM(t.num_posts) as total_posts'));
	}

Last edited by Franz (2011-11-03 09:27:14)

Franz · 2011-11-03 09:55:30

One more thing: we could, of course, use a very simple (not safe) implementation to at least quote table names that match (My)SQL keywords, although (especially with a prefix) this would probably rarely happen. Or is this a responsibility of the developer?

MattF · 2011-11-03 11:47:11

If anything, I'd suggest you let the DB crap out on the use of keywords/reserved words rather than hacking around their possible usage. There's no reason on a coders part, barring idleness or ignorance of the words, not to use unique names.

Forums

Announcement

#1 2011-09-15 03:54:35

2.0 DB: Sanitize table names etc.?

#2 2011-09-15 15:55:32

Re: 2.0 DB: Sanitize table names etc.?

#3 2011-09-15 19:41:32

Re: 2.0 DB: Sanitize table names etc.?

#4 2011-09-16 02:08:27

Re: 2.0 DB: Sanitize table names etc.?

#5 2011-09-16 03:13:33

Re: 2.0 DB: Sanitize table names etc.?

#6 2011-09-16 03:28:18

Re: 2.0 DB: Sanitize table names etc.?

#7 2011-09-16 03:39:35

Re: 2.0 DB: Sanitize table names etc.?

#8 2011-10-19 23:13:24

Re: 2.0 DB: Sanitize table names etc.?

#9 2011-11-02 23:31:51

Re: 2.0 DB: Sanitize table names etc.?

#10 2011-11-03 07:06:46

Re: 2.0 DB: Sanitize table names etc.?

#11 2011-11-03 08:48:57

Re: 2.0 DB: Sanitize table names etc.?

#12 2011-11-03 09:26:06

Re: 2.0 DB: Sanitize table names etc.?

#13 2011-11-03 09:55:30

Re: 2.0 DB: Sanitize table names etc.?

#14 2011-11-03 11:47:11

Re: 2.0 DB: Sanitize table names etc.?

Board footer