sql-injection: consider case using psycopg2.extensions.AsIs

[moylop260]

Steps:

1. Create a odoo module using the following sql injection case:

from psycopg2.extensions import AsIs
self.env.cr.execute("SELECT id FROM %s", (AsIs('res_users; DELETE FROM res_users;'),))

2. Run pylint enabling sql-injection checker.

Saw:
sql-injection checker is not detected.

Expected:
sql-injection checker detected.

[pedrobaeza]

Well, you are ruining the trick for avoiding the check, as there's no other way of for example build the SQL views reports.

[moylop260]

We can use a sanitize method like
OCA/server-tools#1426 (comment)

Could you share me a code bypassing this check using AsIs?

In order to check if there is a way to skip a special case from the check

[pedrobaeza]

This is one example: https://github.com/OCA/crm/blob/c0a0abc5c8b1a9ef17d224bbab768c8f089eddaf/crm_phonecall/report/crm_phonecall_report.py#L134

[moylop260]

Using private attributes was fixed here:

• https://github.com/OCA/pylint-odoo/pull/122/files

I mean, sql-injection is not face out if the variables startswith _

I just have created the following PR to test the case:

• [REF] sql-injection: Test private attributes #226

[pedrobaeza]

Can you try with a variable inside the same scope (not an object variable)?

[moylop260]

Done
https://github.com/OCA/pylint-odoo/pull/226/files

[pedrobaeza]

Well, I mean if it doesn't count when the variable is in the same scope. Any way, I don't like this, as you are reducing also the possibilities. What about if I want to put this:

"CREATE VIEW %s AS (SELECT %s FROM res_partner)" % (_variable, self.select()))

[moylop260]

I just have created a table of True with all cases using public and private attributes and using with and without underscored variables.

Use	Result
self._private_attribute	skip sql-injection
self.public_attribute	sql-injection
_variable	sql-injection
variable	sql-injection

Really, if you use a variable doesn't matter the name.
For OOP the attributes change if it starts with a underscore.
Then, the variables are out of scope for this case.

[pedrobaeza]

Then that's why I don't want this new check, because that are valid cases.

[moylop260]

I don't get you.

[pedrobaeza]

Enabling this check, my cases will be considered error.

[moylop260]

Considering AsIs in the sql-injection check, the new result should be:

	Use	Current Result	New Result
1	AsIs(self._private_attribute)	skip sql-injection	skip sql-injection
2	AsIs(self.public_attribute)	skip sql-injection	sql-injection
3	AsIs(_variable)	skip sql-injection	sql-injection
4	AsIs(variable)	skip sql-injection	sql-injection
5	self._private_attribute	skip sql-injection	skip sql-injection
6	self.public_attribute	sql-injection	sql-injection
7	_variable	sql-injection	sql-injection
8	variable	sql-injection	sql-injection

In this case, you are in the first case, then, your cases won't considered sql-injection error.
In fact, if you remove AsIs like case # 5, neither will considered error, since that we are skipping private attributes from #226

[moylop260]

Closing since it is a valid workaround where the dev needs to be sure it is not a sql-injection