Yii 2 <= 2.0.47 SQL Injection Vulnerability

[cc7b3r]

Description

Yii 2 Framework is a project used for PHP application development. Yii versions <= 2.0.47 are susceptible to a SQL injection vulnerability in its "yiibaseController::runAction($route,$params)" function. This vulnerability occurs when parameters received by the affected function are not properly sanitized.

### What steps will reproduce the problem?

A simple proof-of-concept test on the affected run could generate an injection into the database system:

yii\baseModule::runAction('PARAM0', ['VAR0' => 'INJECTION', 'VAR2' => 'PARAM2'])

### What is the expected result?

A parameter result not found or a badly formed query

### What do you get instead?

Syntax error or query executed directly on database system

Additional info

Q	A
Yii version	<= 2.0.47
PHP version	7.1.33
Operating system	Linux / Debian - Apache 2.4.38

[DeryabinSergey]

Show please your code with SQL in Controller / Action

[cc7b3r]

Here is a snippet of the requested code which generates the injection:

in /.../yiisoft/yii2/base/Module.php at line 534– [yii\base\Controller::runAction]('cms', ['page' => 'PAG' or 1=1--', 'ad' => '0'])

  $parts = $this->createController($route);
    if (is_array($parts)) {
        /* @var $controller Controller */
        list($controller, $actionID) = $parts;
        $oldController = Yii::$app->controller;
        Yii::$app->controller = $controller;
        $result = $controller->**runAction($actionID, $params);**
        if ($oldController !== null) {
            Yii::$app->controller = $oldController;
        }

        return $result;
    }

[DeryabinSergey]

There is no queries to DB here. Show code where you use this params in query and catch injection

[DeryabinSergey]

I think you not correct use ActiveQuery or QueryBuilder. And I’m not sure that is framework’s problem.

[cc7b3r]

Injection occurs when any element of the array in the $params variable contains any SQL statement, for example you can choose to use the following payload:

' OR NOT 5406=5406#

It should look inside the function like this:

[yii\baseController::runAction]('cms', ['page' => 'PAG'' OR NOT 5406=5406#, 'ad' => '0'])

The following Payload generated a disclosure of internal information from the Database, here is an image of evidence:

what you see in red is the injection.

In conclusion, I would suggest implementing a native function to sanitize any kind of variable that is sent, don't you think?

[DeryabinSergey]

Show here the code controller action, where you build the query.

[cc7b3r]

Sorry my friend, this is very confidential information about the business logic, is there any other alternative?

Why in this project do you move the security of the requests to each user of the framework? Don't you think that could generate a security problem which can be managed more efficiently by sanitizing the incoming data received by that function?.

[InsaneSkull]

@cc7b3r part of code where you build query will be helpful, you can replace confidential information with fake data.

[bizley]

@cc7b3r it looks like you are using some custom query building method without proper PDO params binding. Your example is suggesting that this is injected into the pagination but Yii's pagination is using LIMIT and OFFSET and on the screen I can see that the injection is a part of WHERE clause. So unless you can provide some minimal proof of the vulnerability we don't see any problem in the Yii's core.

[yii-bot]

Thanks for posting in our issue tracker.
In order to properly assist you, we need additional information:

• When does the issue occur?
• What do you see?
• What was the expected result?
• Can you supply us with a stacktrace? (optional)
• Do you have exact code to reproduce it? Maybe a PHPUnit tests that fails? (optional)

Thanks!

This is an automated comment, triggered by adding the label status:need more info.

[cc7b3r]

Here is part of the code:

in TemplateController.php at line 108– yii\db\Command::queryAll()

$sql .= " and (dab.company = '' or dab.company is null) ";
}

    $sql = \Yii::$app->db->createCommand($sql)->bindValue('pagename',$pageName);
    $sql = \Yii::$app->db->createCommand($sql);
    $space = $sql->queryAll();

    $sql = "SELECT
                dab.id,
                dab.ubication,
                dab.carrousel,
                dab.position,

The SQL being executed was:

SELECT dab.id,dab.ubication,dab.carrousel,dab.position,dab.html,dab.adapt,dab.text FROM pages AS p
LEFT JOIN templates AS dab ON p.id=dab.id_pag AND dab.status = 1 AND dab.position <> 0
WHERE dab.code='PAG'' AND dab.carrousel = 0 AND dab.adapt = 0 and (dab.company = '' or dab.company is null)

[bizley]

Could you paste here the rest of the $sql string?

[cc7b3r]

SELECT dab.id,dab.ubication,dab.carrousel,dab.position,dab.html,dab.adapt,dab.text FROM pages AS p
LEFT JOIN templates AS dab ON p.id=dab.id_pag AND dab.status = 1 AND dab.position <> 0
WHERE dab.code='PAG'' AND dab.carrousel = 0 AND dab.adapt = 0 and (dab.company = '' or dab.company is null)

[cc7b3r]

what I was saying is that SQL code can be injected because it is passed without sanitizing or being escaped into the "dab.code" field.

[bizley]

Not the generated string, the code please.

[cc7b3r]

I don't have the rest of the code because I am reporting the bug as a black box test and I don't have access to that code.

[cc7b3r]

Perhaps part of the stack trace can give you more information:

[bizley]

The problematic part is most probably in the code for this variable. If this is as you described "a black box" for you, you must report it to the person that owns that code. I'm closing this report now, if you can provide as with required information we'll gladly reopen it.

[marcovtwout]

@cc7b3r Did you create the following CVE when you created this issue? https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-26750

If so, please withdraw/reject it. No framework exploit was found so the CVE submission is invalid.

[cc7b3r]

I see that a FIX was performed on the queryScalar method and which has to do with the construction of the ActiveQuery.

the documentation is at #19790

[cc7b3r]

I always wanted to ask myself why the user is not given the freedom to make his queries directly and the Frameworkn is not able by itself to be resilient with injections. Don't you think it can make a design problem?

[cc7b3r]

@cc7b3r Did you create the following CVE when you created this issue? https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-26750

If so, please withdraw/reject it. No framework exploit was found so the CVE submission is invalid.

I reported the vulnerability because they closed the thread and I was never able to respond in time.

[bizley]

What do you mean? Who are "they"? Do you still claim that there is a point to keep CVE?

[DeryabinSergey]

I propose that this CVE only affects one project and this project is not Yii2

[cc7b3r]

Please,

@cc7b3r Did you create the following CVE when you created this issue? https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-26750
If so, please withdraw/reject it. No framework exploit was found so the CVE submission is invalid.

I reported the vulnerability because they closed the thread and I was never able to respond in time.

What do you mean? Who are "they"? Do you still claim that there is a point to keep CVE?

The CVE is the least of it, I would really like to understand why the user of the Framework is given the freedom to handle the statements in that way?

Shouldn't the framework limit that kind of bad practices or sanitize the default data once it receives any parameter that creates a new query instance?

[bizley]

I'm not sure what you mean. We are not able to stop the developer creating vulnerable code if the code is not following best practices and the safety guides.

[cc7b3r]

queryAll()

I'm not sure what you mean. We are not able to stop the developer creating vulnerable code if the code is not following best practices and the safety guides.

That is true, but one of the fundamental principles is to think in the worst-case scenario. Some constructors that receive the query do not sanitize the data before being sent to the faulty code created by the framework user, they are exposed above.

[hyncica]

Shouldn't the framework limit that kind of bad practices or sanitize the default data once it receives any parameter that creates a new query instance?

No it shouldn't. It's impossible to predict all use cases that developers will need to use the framework for. Not providing a method to execute SQL query developer want to execute may result in case where developer will be unable to implement some use case. Because of that responsibility for sanitizing inputs and for using best practices/safe methods must be on application developers not on framework.

[diego-mathis]

Just follow best practices: https://www.yiiframework.com/doc/guide/2.0/en/security-best-practices#avoiding-sql-injections

[schmunk42]

@cc7b3r

I reported the vulnerability because they closed the thread and I was never able to respond in time.

The thread was closed, because you were not able to give any useful information or code to reproduce the issue.
I've seen similar stuff in several Security Audits and Pen-Tests and it was in 100% of the cases the fault of the project developer, not the framework.

You say you were not able to respond in time, but you had the time to create a CVE?
This is just ridiculous, sorry to say that.

[cc7b3r]

@cc7b3r

I reported the vulnerability because they closed the thread and I was never able to respond in time.

The thread was closed, because you were not able to give any useful information or code to reproduce the issue. I've seen similar stuff in several Security Audits and Pen-Tests and it was in 100% of the cases the fault of the project developer, not the framework.

You say you were not able to respond in time, but you had the time to create a CVE? This is just ridiculous, sorry to say that.

Yes, that's why I reported the CVE. I still believe there is a problem, the risk of SQL injection cannot be transferred to the user of the framework because not all users follow best practices, so why not substantially minimize that problem from the framework design?

[DeryabinSergey]

Why you not starting CVE to PHP PDO::query and others?

[ailmanki]

There seems to be a missunderstanding @cc7b3r
If you use the framework, that is the ActiveRecord and QueryBuilder properly, then there will be no SQL injections possible.
But if you ignore how it should be used, then yes there are many ways to create SQL injections, there are many other ways to to cerate security problems as well.
At least one would need to understand how to use it, which is describe in detail in the documentation.

Yii also allows to be extended, at which point you get a lot of ways to create security issues.

Since Yii has to communicate with a 3d party software - the database, and SQL Implementations being most often a turing complete language, there is only so much Yii can sanitize automatically - which if you use its code correctly - also works.
You could already start to write SQL code instead of a tablename, where a tablename is expected and shoot yourself in the foot.
This is basically the reason why SQL Injections are so prevalent.

[schmunk42]

@cc7b3r

I still believe there is a problem

It's not about believing, give us some code to reproduce the issue, or even better: write a test-case.
Everything else is just a waste of time.

My 2 cents.

[marcovtwout]

The CVE record at MITRE is now marked as DISPUTED.

@cc7b3r There really is no basis for a CVE here. You reported the issue based on a black box test for a specific client project without knowing what code actually causes the vulnerability. Even a hypothetical design problem is not relevant without you being able to demonstrate this is a framework issue, and as others have stated, it most likely is not. If you still wish to discuss this further, perhaps try one of the chat options here https://www.yiiframework.com/chat. But in any case please withdraw the CVE.

[iBotPeaches]

Sorry to dredge up an old thread, but alerting systems are flagging back to this via the Advisory. I read the thread and its quite embarrassing how somehow a user gets into a position that request/obtain an CVE without a single shred of reproducible evidence against the framework. Its like some vendetta to take their point seriously by submitting non-sense.

It looks like this was then synced/created into GitHub Advisories at GHSA-gq63-p39p-jrjf in quite possibly the most embarrassing advisory I've seen. The title is grossly typo`d with:

SQL injectionin Yii 2

@ccchapman Put up a PR to help fix some of the request up to make it a bit more readable, but it makes 0 sense to me. The Advisory itself in the JSON file states it was not reviewed by GitHub

    "github_reviewed": false,
    "github_reviewed_at": null,

But GitHub said it was reviewed?

So we have a DISPUTED cve in MITRE, but it was pulled into GitHub Advisories, then Dependabot is telling people to just upgrade to 2.0.47 for some phantom patch of a non-existent problem?

Sure I can squash the warning from Dependabot by just upgrading to 2.0.47, but we know in this thread that nothing was changed to fix anything because nothing was found.

Should we request the Advisory be pulled? I saw in this example its how its handled when a similar situation occurred for Laravel.

[bizley]

We tried to make it rejected several times already and I don't know what else we can do. It's truly worrying to see how easy it is to make unproven claims like that and how far can the damage spread...

[iBotPeaches]

Okay thanks. I took a stab at getting this withdrawn - github/advisory-database#2607