Use SQL parameters to prevent SQL injections in SearchRepository.php #75

GauthierPLM commented 2023年06月11日 15:36:47 +02:00

Fix #67

When using user' input in SQL queries, it's recommended to use parameters in prepared queries to avoid SQL Injections. The issue reported in ticket #67 is a typical SQL inject, in which it would have been super easy for an attacker to query kbin' DB.

SearchRepository.php is the only file I checked, so I don't know if this kind of issues also exists elsewhere, but it would be good to perform a review as kbin is growing and security will be a concern for many.

Fix #67 When using user' input in SQL queries, it's recommended [to use parameters in prepared queries](https://www.doctrine-project.org/projects/doctrine-dbal/en/current/reference/data-retrieval-and-manipulation.html#dynamic-parameters-and-prepared-statements) to avoid SQL Injections. The issue reported in ticket #67 is a typical SQL inject, in which it would have been super easy for an attacker to query kbin' DB. SearchRepository.php is the only file I checked, so I don't know if this kind of issues also exists elsewhere, but it would be good to perform a review as kbin is growing and security will be a concern for many.

GauthierPLM commented 2023年06月11日 18:51:42 +02:00

Since #47 is caused by the same requests that I changed earlier, I added a commit to fix it by filtering votes by choice = 1.

Since #47 is caused by the same requests that I changed earlier, I added a commit to fix it by filtering votes by choice = 1.

CoderKat commented 2023年06月13日 07:26:07 +02:00

Oof, noticed this coincidentally while looking for #47. This has potential to be very bad. I did a very cursory skim. There's quite a few places that aren't using proper SQL parameterization. Most are fortunately unlikely to be at risk because they're IDs that are presumably ints, but should still be parameterized to be safe (and particularly so that you never have to consider if any particular variable is safe being concatenated to a query).

Anyway, I don't personally have the time to figure out how to test anything (side note: oof, is there no unit tests anywhere?), but searching for some potentially risky keywords, I found a few other places that are likely highest risk, such as (line numbers @ 0ac805386d66e):

• src\Repository\EntryRepository.php:156 - $qb->andWhere("JSONB_CONTAINS(e.tags, '\"".$criteria->tag."\"') = true");
• src\Repository\EntryRepository.php:333 (basically the same thing)
• src\Repository\MagazineRepository.php:328 (basically the same thing)
• src\Repository\PostRepository.php:135 (basically the same thing)
• src\Repository\PostRepository.php:291 (basically the same thing)
• src\Repository\TagRepository.php (basically the same thing)
• src\Repository\EntryRepository.php:356 ['name' => "%{$name}%", 'title' => "%{$name}%", 'visibility' => VisibilityInterface::VISIBILITY_VISIBLE]

There's also frankly a ton of places that have user or magazine ID concatenated like what GauthierPLM had fixed in this one file. Though assuming those are ints as we'd expect, the risk there is lower. String fields are what I focused on as they are the highest risk for SQL injection. This was only a quick skim and not an exhaustive review. Also, given the type system of PHP (I'm verrrry rusty as I haven't touched PHP in about a decade), making assumptions about integer types may be a dangerous idea.

Oof, noticed this coincidentally while looking for #47. This has potential to be very bad. I did a very cursory skim. There's quite a few places that aren't using proper SQL parameterization. Most are fortunately unlikely to be at risk because they're IDs that are presumably ints, but should still be parameterized to be safe (and particularly so that you never have to consider if any particular variable is safe being concatenated to a query). Anyway, I don't personally have the time to figure out how to test anything (side note: oof, is there no unit tests anywhere?), but searching for some potentially risky keywords, I found a few other places that are likely highest risk, such as (line numbers @ `0ac805386d66e`): - `src\Repository\EntryRepository.php:156` - `$qb->andWhere("JSONB_CONTAINS(e.tags, '\"".$criteria->tag."\"') = true");` - `src\Repository\EntryRepository.php:333` (basically the same thing) - `src\Repository\MagazineRepository.php:328` (basically the same thing) - `src\Repository\PostRepository.php:135` (basically the same thing) - `src\Repository\PostRepository.php:291` (basically the same thing) - `src\Repository\TagRepository.php` (basically the same thing) - `src\Repository\EntryRepository.php:356` `['name' => "%{$name}%", 'title' => "%{$name}%", 'visibility' => VisibilityInterface::VISIBILITY_VISIBLE]` There's also frankly a *ton* of places that have user or magazine ID concatenated like what GauthierPLM had fixed in this one file. Though assuming those are ints as we'd expect, the risk there is lower. String fields are what I focused on as they are the highest risk for SQL injection. This was only a quick skim and not an exhaustive review. Also, given the type system of PHP (I'm verrrry rusty as I haven't touched PHP in about a decade), making assumptions about integer types may be a dangerous idea.

GauthierPLM commented 2023年06月13日 22:00:37 +02:00

I'm not sure how to fix the tags insertion, this seem more complicated than the usually query injections and I don't really know that part of kbin yet.

However, for src\Repository\EntryRepository.php:356, the strings are properly « injected » since they are passed using the setParameters() function, which ensure they are properly escaped and are safe from SQL injection.

Regarding PHP types, kbin uses declare(strict_types=1);, meaning any value not in the right type will cause an error (see https://www.phptutorial.net/php-tutorial/php-strict_types/). That and the fact that usually IDs are taken from Users or Magazines (which are automatically retrieved using ParamConverter) ensure they are valid.
Using prepared requests would be better for sure, but it's less critical than the SQL injections in the SearchRepoistory.

I'm not sure how to fix the tags insertion, this seem more complicated than the usually query injections and I don't really know that part of kbin yet. However, for `src\Repository\EntryRepository.php:356`, the strings are properly « injected » since they are passed using the `setParameters()` function, which ensure they are properly escaped and are safe from SQL injection. Regarding PHP types, kbin uses `declare(strict_types=1);`, meaning any value not in the right type will cause an error (see https://www.phptutorial.net/php-tutorial/php-strict_types/). That and the fact that usually IDs are taken from Users or Magazines (which are automatically retrieved using ParamConverter) ensure they are valid. Using prepared requests would be better for sure, but it's less critical than the SQL injections in the SearchRepoistory.