Skip to content

Commit

Permalink
Merge pull request from GHSA-2jx3-5j9v-prpp
Browse files Browse the repository at this point in the history 
Validate order by and order way
  • Loading branch information
@atomiix
atomiix committed Jun 24, 2022
2 parents 13e64b2 + be79516 commit b3ec4b8
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/Search/WishListProductSearchProvider.php
View file
Expand Up @@ -35,6 +35,7 @@
use Product;
use Shop;
use Symfony\Component\Translation\TranslatorInterface;
use Validate;
use WishList;

/**
Expand Down Expand Up @@ -167,7 +168,10 @@ private function getProductsOrCount(

if ('products' === $type) {
$sortOrder = $query->getSortOrder()->toLegacyOrderBy(true);
$querySearch->orderBy($sortOrder . ' ' . $query->getSortOrder()->toLegacyOrderWay());
$sortWay = $query->getSortOrder()->toLegacyOrderWay();
if (Validate::isOrderBy($sortOrder) && Validate::isOrderWay($sortWay)) {
$querySearch->orderBy($sortOrder . ' ' . $sortWay);
}
$querySearch->limit((int) $query->getResultsPerPage(), ((int) $query->getPage() - 1) * (int) $query->getResultsPerPage());
$products = $this->db->executeS($querySearch);

Expand Down

5 comments on commit b3ec4b8

@doekia
Copy link

@doekia doekia commented on b3ec4b8 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So apparently there is unfiltered value returned by toLegacyOrderWay() (I'm doubt full of it see below)
If this is the case CHANGE the function, not simply one module otherwise it is an open door for other modules.

Why I'm doubtfull ? Because toLegacyOrderWay() calls getDirection() which return $this->direction. $this->direction is set by setDirection() (during construct) and tests are done against values asc, desc, random otherwise exception. How can this lead to sql injection

@infiniweb
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doekia this issue is not with toLegacyOrderWay. The SQL injection can be reproduced because of the usage of toLegacyOrderBy (not toLegacyOrderWay) combined with a wrong usage of getLegacyPrefix. I can confirm the exploit. The usage of Validate::isOrderBy($sortOrder) is fixing the issue.

@doekia
Copy link

@doekia doekia commented on b3ec4b8 Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore I repeat, the function that need to be patched SHOULD BE toLegacyOrderBy() that should implement the Validate::isOrderBy() before returning values.
Such way will protect ANY modules that use the framework function toLegacyOrderBy()

@Pilypas
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me if the vulnerability in the module is also in Prestashop 1.6.1.24 version of blockwishlist 1.3.2 ?

@infiniweb
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pilypas . No it's not. Please refer to GHSA-2jx3-5j9v-prpp

Please sign in to comment.