New security option for you to consider

[datakick]

If you update to bleeding edge / 1.5.0, you will find new option Allow multi statements queries in Advanced Parameters > Performance.

For backwards compatibility reasons, this option is enabled. That means that multiple sql queries can be executed at once. 

This is very dangerous, because it can be exploited by attackers if they find SQL injection vulnerability.

For example, if some module contains code with SQL injection like this one:

$conn->executeS('SELECT * from '._DB_PREFIX_.'orders WHERE id_order = ' . Tools::getValue('id_order'));

Then attacker can post request to your server with parameter id_order=0%3B%20UPDATE%20tb_employee%20SET%20passwd%20%3D%20%27pwd%27 which is url-encoded equivalent for

0; UPDATE tb_employee SET passwd = 'pwd'

PHP code would then execute following SQL query:

SELECT * FROM tb_orders WHERE id_order = 0; UPDATE tb_employee SET passwd = 'pwd'

Attacker just changed password for all administrators, and can log in to your back office. 

When you disable multiple statements, that SQL query would throw an exception.

So it's very good idea to block this.

Of course, there can be some modules (or even core code) that depends on this multi-statement functionality. I recommend installing collectlogs module before your disable this, and watch for any new errors.

Note that this is not some silver bullet that solves SQL injection attacks. Even with this option enabled, attacker can use the SQL injection to extract information from your store by constructing smart queries. But it significantly reduces the options they have

[pauld]

How safe is it to disable this option in terms of TB core and native modules?

[datakick]

Hard to tell. There were few places even in core that used multiple sql queries. Thise were fixed. But if that's all, nobody knows. Static code analysis is mostly useless here.

[wakabayashi]

Does this block subqueries or is this something different? I am talking about a SELECT in a SELECT...

[datakick]

8 minutes ago, wakabayashi said:

Does this block subqueries or is this something different? I am talking about a SELECT in a SELECT...

No, subqueries are integral part of parent query.

I'm talking about multiple independent sql statements executed in one call. With this option enabled, you could do

$conn->execute('delete from a; delete from b; delete from c');

and it would be the same as 

$conn->execute('delete from a');
$conn->execute('delete from b');
$conn->execute('delete from c');

Theoretically, the first would be a little bit faster, because you save two calls to db server.

It's nice, but very dangerous.

[the.rampage.rado]

So far no issues for me with this update. All errors should be visible in the logs, right?

 

What about Stringify fetches? What is affected by this change in layman terms?

[datakick]

12 hours ago, the.rampage.rado said:

So far no issues for me with this update. All errors should be visible in the logs, right?

I also haven't encountered any issues on my production server so far. Granted, I don't use that many modules, but its encouraging anyway.

12 hours ago, the.rampage.rado said:

What about Stringify fetches? What is affected by this change in layman terms?

This is a tricky one.

Do not turn off Stringify fetches on production server. 

Back in the days, MySql returned numbers (and other native types) as string. PHP would receive text value '100.45' instead of float value 100.45, even though in database it was stored as decimal number.

PrestaShop / thirtybees didn't convert the data to expected data types, and work with the strings. PHP is funny enough to support operations with mixed data types  without complains, for example 

$result = '100.45' * 10.65;

Of course, this is little bit slower than doing operations with floats directly. And there are some gotchas and corner cases that make developers go mad.

Newer db servers can return data in correct types, so we would receive floats or ints from mysql server directly. For developers this would be a big relief, because we wouldn't have to remember that the data we work with might be float of string or null. A lot of code could be simplified.

Unfortunately, this can cause a backwards compatibility issues. And what is worse, it can just change the behaviour of system without any exceptions being thrown.

Imagine there is a code

if ($address->id_country === Configuration::get('PS_COUNTRY_DEFAULT')) {
   // do something
}

method Configuration::get() returns string, and there is a strict comparison used === instead of normal comparison ==, so types of operands will be checked as well.

With stringify fetches, this condition returns true, because $id_country property contains string, so the condition is actually

if ('1' === '1') {
}

Without stringify fetches, the condition would be

if (1 === '1') {
}

and that evaluates to false.

The result is difference in evaluation of logic. 

We are trying to weed out these kind of things from core, but it's hard and slow process.

We will probably never get to the point where this option could be disabled safely. It's probably a good idea to hide this switch for normal users, or move it to some sort of 'Experimental' tab. I will do that. 

[wakabayashi]

16 hours ago, datakick said:

Theoretically, the first would be a little bit faster, because you save two calls to db server.

It's nice, but very dangerous.

I see. I believe this idea never came to my mind 😅 Sometimes it's good, to be not too clever 😏

[nickz]

On 6/22/2023 at 4:40 AM, datakick said:

Attacker just changed password for all administrators, and can log in to your back office. 

How does the hacker get the name of the admin folder?