move check_query() into the database interface files [#13180]

The check_query documentation says 'Prepare user input for use in a database query, preventing SQL injection attacks.' But the code itself is a simple addslashes. This is simply not correct -- for mysql, yes, but the world is not just mysql. There are _escape_string functions for that: mysql_escape_string, pg_escape_string, sqlite_escape_string... Read the manual for pg_escape_string: "Use of this function is recommended instead of addslashes()." Or read sqlite_escape_string: "addslashes() should NOT be used to quote your strings for SQLite queries; it will lead to strange results when retrieving your data." Ooooops.

So I propose moving check_query() declaration from bootstrap.inc to database.*.inc. I've already written a letter to the devel list about this, but drumm said "code is better than talk" so here is a patch.

At this moment, this does absolutely nothing, only opens the possibility to use the aforementioned escape_string functions.

But database.sqlite.inc simply will not work without this change.

Comment	File	Size	Author
#12	mysql_escape_patch_0.txt	573 bytes	chx
#11	mysql_escape_patch.txt	572 bytes	chx
#9	db_escape_string_0.txt	20.88 KB	chx
#8	db_escape_string.txt	20.87 KB	chx
#6	check_output_patch_0.txt	1.6 KB	chx
	check_output_patch.txt	1.54 KB	chx

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

drumm

he/him

NY, US

CreditAttribution: drumm commented 19 November 2004 at 19:42

Looks straightfoward enough.

Is there any reason to not put in pg_escape_string() now?

Comment #2

chx CreditAttribution: chx commented 19 November 2004 at 19:47

Actually, yes. Read on the mentioned manual page: "Note: This function requires PostgreSQL 7.2 or later." I have no idea how the check it. There is a pg_version() but I will leave this postgres business to someone who has a working postgres install to code and check this thing.

Comment #3

Chris Johnson CreditAttribution: Chris Johnson commented 19 November 2004 at 20:58

PostgreSQL 7.2 was released February 4, 2002. Major production releases 73. and 7.4, along with many point releases, have been issued since then. Current production is 7.4.6 and 8.0 is in the 4th revision of beta.

It seems pretty safe to assume that any Drupal installation using PostgreSQL would have version 7.2 or later. Document if a user encounters errors with PostgreSQL that they need to check the version is 7.2 or later.

Comment #4

Dries CreditAttribution: Dries commented 20 November 2004 at 13:42

I suggest creating a db_escape_string() function, and removing check_query() all together.

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 20 November 2004 at 13:55

+1 on Dries

Comment #6

chx CreditAttribution: chx commented 20 November 2004 at 17:32

File	Size
check_output_patch_0.txt	1.6 KB

db_escape_string sounds find to me, it's really a better name, thanks Dries. But removing check_query? I'd suggest deprecating it and removing it later. I am afraid It would break a lot of custom modules.

Here is a slightly modified patch with pg_escape_string included and documented.

Comment #7

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 20 November 2004 at 17:43

We were never afraid of breakign things for the good of the project. Please don't change this policy.

Comment #8

chx CreditAttribution: chx commented 20 November 2004 at 18:26

File	Size
db_escape_string.txt	20.87 KB

Let's do it.

Comment #9

chx CreditAttribution: chx commented 20 November 2004 at 18:29

File	Size
db_escape_string_0.txt	20.88 KB

Something funny happened to the PostgreSQL db_escape_string comment and I have missed it, sorry. Reposting the patch.

Comment #10

Dries CreditAttribution: Dries commented 21 November 2004 at 08:29

I committed the patch. Thanks chx. Keep it coming.

I've also updated the upgrade notes at http://drupal.org/node/12347.

I'll update some contributed modules unless someone objects.

Comment #11

chx CreditAttribution: chx commented 21 November 2004 at 11:27

File	Size
mysql_escape_patch.txt	572 bytes

Keep it coming? Well, let's see the MySQL case... It's far from being trivial, and I expect some controversy over it.

Comment #12

chx CreditAttribution: chx commented 9 March 2005 at 23:36

File	Size
mysql_escape_patch_0.txt	573 bytes

OK, here is mysql version a bit corrected, this should get into 4.6 -- addslashes is not a correct way any more.

Comment #13

Dries CreditAttribution: Dries commented 10 March 2005 at 18:29

Can we read up on that somewhere?

Comment #14

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 10 March 2005 at 18:37

Dries, sure: http://php.net/mysql_escape_string and http://php.net/mysql_real_escape_string. The latter also uses the connections charset settings to properly escape, so it is the best (but is not available in old PHP versions). The addslashes page user notes also have more information.

Comment #15

Dries CreditAttribution: Dries commented 12 March 2005 at 08:50

addslashes() has been working for ages (and is simpler code-wise). I guess the mysql_ functions are slightly more secure or not even that? That aside, I'm not really a big fan of version_compare()s. Also note that the patch violates Drupal's coding standards (again) but that's easy enough to fix.

Comment #16

chx CreditAttribution: chx commented 13 March 2005 at 19:49

I can only repeat what Goba said: . mysql_real_escape_string uses the connections charset settings to properly escape, so it is the best (but is not available in old PHP versions).

Comment #17

Steven CreditAttribution: Steven commented 1 June 2005 at 03:40

Actually, Drupal does nothing to ensure the database character set is set to UTF-8 (can it?). Thus, escaping with mysql_real_escape_string() is not advised.