MessageSource::getMessageFilePath() - filename injection

[uaoleg]

Yii::$app->language can contain any string, and this string is directly passed into MessageSource::getMessageFilePath() and used in filename without any validation:

yii2/framework/i18n/PhpMessageSource.php

Line 135 in 435b6dc

$messageFile = Yii::getAlias($this->basePath) . "/$language/";

That could be a potetial security issue:

The message file for category 'yii' does not exist: /var/www/app/vendor/yiisoft/yii2/messages/"><script >alert(String.fromCharCode(88,83,83))</script>/yii.php Fallback file does not exist as well: /var/www/app/vendor/yiisoft/yii2/messages/">/yii.php

[bizley]

Is there any place where end user could provide the language directly and that message would pop up? I cannot find such case.

[WinterSilence]

Yii don't have "fools protection"

[uaoleg]

@bizley the case is any website that provides language selection on UI
@WinterSilence it's not obvious at all that I have to escape somehow the language name

[WinterSilence]

@uaoleg you must escape ANY user data - it's contained in ANY security guide

[uaoleg]

@WinterSilence no, I should not. For example if user has name O'Reilly I won't escape it to save in DB, I will save it as is. And framework's ORM will do the job

[WinterSilence]

@uaoleg read more about second-order SQL injection
I don't know if I translated it correctly, it's called in Russian: SQL-инъекция второго порядка

[bizley]

the case is any website that provides language selection on UI

Since the framework is not using that value (I mean "provided by user") directly we rely on the developer to validate it when passing to the component.

[uaoleg]

@bizley it's not obvious that this property is used as a part of filepath. I'll create a simple PR which will fix this issue

yii2/framework/base/Application.php

Line 116 in 435b6dc

public $language = 'en-US';

[bizley]

Hm, ok, please do. Maybe we need to add some more clarification for that.

[samdark]

We should escape it. Something like preg_replace('/[^\w_-]/', '_', $language) should be sufficient.

[uaoleg]

Created a PR #18913

[WinterSilence]

@samdark class must generate error instead escape: if (preg_match('/[^a-zA-Z_-]/', $language) === 1) { throw new ... }

[samdark]

Agree 👍

[samdark]

@uaoleg would you please adjust the pull request? It should throw \InvalidArgumentException.

[uaoleg]

@samdark done