RPC executeQuery is open to SQL Injection

[carstenklein@…]

Please adjust the implementation of the executeQuery Method so that it prevents SQL Injection.

Both parameters "query" and "sort" are not tested against common types of SQL Injection attacks.

E.g. providing for sort a value of e.g. "start_time asc; DELETE FROM ticket WHERE 1=1; DELETE FROM wiki WHERE 1=1;" would remove all of your important ticket and wiki data. Actual table names and field names may be different from the ones used in the above example, but you should get the picture.

[Yuji OKAZAKI]

(In [7305]) refs #6381

[carstenklein@…]

Deactivating the RPC functionality is an option, but how about trying to test against for example the possible injection of sql statements?

e.g.

if ( sort != ):

assertNotAnSqlInjection( query )

else:

sort = DEFAULT

if ( query != ):

assertNotAnSqlInjection( sort )

else:

query = DEFAULT

[...]

and (PSEUDO Code here)

def assertNotAnSqlInjection( sql ):

if SELECT, DELETE or UPDATE or CREATE or DROP in ucase(sql) or ucase(sql):

raise SqlInjectionDetectedException(...)

Of course, the assertion should be somewhat more expressive in that other possible injections can also be detected.

[Yuji OKAZAKI]

ありがとうございます、私はそれを試みるつもりです。

[Yuji OKAZAKI]

Thank you, I will try it.

[osimons]

The pattern that should be used is not string substitution, but parameter substition at db level. Use cursor.execute(statement, args) and not cursor.execute(statement % args). The db backends will then handle all the necessary escaping.

Your args array need to contain a matching number of arguments according to the number of %s you put into your string, and there is no need to consider type or special marking of your substitutions in your string other than plain %s (ie. don't use the string substitution ideom of ...version='%s' -just use ...version=%s.

Example:

cursor.execute("select * from wiki where name=%s and version=%s", ["WikiStart", 1])