From 1e13bfd13d671fb070fe39dc99b9060bb73044ae Mon Sep 17 00:00:00 2001 From: SilverFire - Dmitry Naumenko Date: Thu, 22 Nov 2018 12:47:13 +0200 Subject: [PATCH 1/3] Fixed CSRF token check bypassing in Request::getMethod() --- framework/CHANGELOG.md | 2 ++ framework/web/Request.php | 7 ++++++- tests/framework/web/RequestTest.php | 17 +++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index ef69e2026d..3f10970eb8 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -86,6 +86,8 @@ Yii Framework 2 Change Log - Bug #16828: `yii\console\controllers\MessageController::translator` recognized object' methods and functions calls as identical sets of tokens (erickskrauch) - Bug #16858: Allow `\yii\console\widgets\Table` to render empty table when headers provided but no columns (damiandziaduch) - Bug #16897: Fixed `yii\db\sqlite\Schema` missing primary key constraint detection in case of `INTEGER PRIMARY KEY` (bizley) +- Bug: (CVE-2018-14578): Fixed CSRF token check bypassing in `\yii\web\Request::getMethod()` (silverfire) + 2.0.15.1 March 21, 2018 ----------------------- diff --git a/framework/web/Request.php b/framework/web/Request.php index c05937579a..3a799adeab 100644 --- a/framework/web/Request.php +++ b/framework/web/Request.php @@ -371,7 +371,12 @@ class Request extends \yii\base\Request */ public function getMethod() { - if (isset($_POST[$this->methodParam])) { + if ( + isset($_POST[$this->methodParam]) + // Never allow to downgrade request from WRITE methods (POST, PATCH, DELETE, etc) + // to read methods (GET, HEAD, OPTIONS) for security reasons. + && !in_array(strtoupper($_POST[$this->methodParam]), ['GET', 'HEAD', 'OPTIONS'], true) + ) { return strtoupper($_POST[$this->methodParam]); } diff --git a/tests/framework/web/RequestTest.php b/tests/framework/web/RequestTest.php index 6c50c4ae31..33877f55e6 100644 --- a/tests/framework/web/RequestTest.php +++ b/tests/framework/web/RequestTest.php @@ -724,4 +724,21 @@ class RequestTest extends TestCase $this->assertSame(null, $request->getBodyParam('unexisting')); $this->assertSame('default', $request->getBodyParam('unexisting', 'default')); } + + /** + * @testWith ["POST", "GET", "POST"] + * ["POST", "OPTIONS", "POST"] + * ["POST", "HEAD", "POST"] + * ["POST", "DELETE", "DELETE"] + * ["POST", "CUSTOM", "CUSTOM"] + */ + public function testRequestMethodCanNotBeDowngraded($requestMethod, $requestOverrideMethod, $expectedMethod) + { + $request = new Request(); + + $_SERVER['REQUEST_METHOD'] = $requestMethod; + $_POST[$request->methodParam] = $requestOverrideMethod; + + $this->assertSame($expectedMethod, $request->getMethod()); + } } From 826a0fecdd16eac80b658ff92391f8b75a28b7d3 Mon Sep 17 00:00:00 2001 From: SilverFire - Dmitry Naumenko Date: Thu, 22 Nov 2018 13:40:54 +0200 Subject: [PATCH 2/3] Fixed excess logging of sensitive information in `\yii\log\Target` --- docs/guide/runtime-logging.md | 11 ++++++++++ framework/CHANGELOG.md | 1 + framework/log/Target.php | 34 +++++++++++++++++++++++++++++- tests/framework/log/TargetTest.php | 8 ++++++- 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/docs/guide/runtime-logging.md b/docs/guide/runtime-logging.md index bf226c8531..815af1d455 100644 --- a/docs/guide/runtime-logging.md +++ b/docs/guide/runtime-logging.md @@ -216,6 +216,17 @@ You may configure `logVars` to be an empty array to totally disable the inclusio Or if you want to implement your own way of providing context information, you may override the [[yii\log\Target::getContextMessage()]] method. +In case some of your request fields contain sensitive information you would not like to log (e.g. passwords, access tokens), +you may additionally configure `maskVars` property. By default, the following request parameters will be masked with `***`: +`$_SERVER[HTTP_AUTHORIZATION]`, `$_SERVER[PHP_AUTH_USER]`, `$_SERVER[PHP_AUTH_PW]`, but you can set your own: + +```php +[ + 'class' => 'yii\log\FileTarget', + 'logVars' => ['_SERVER'], + 'maskVars' => ['_SERVER.HTTP_X_PASSWORD'] +] +``` ### Message Trace Level diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 3f10970eb8..38f85f8a55 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -87,6 +87,7 @@ Yii Framework 2 Change Log - Bug #16858: Allow `\yii\console\widgets\Table` to render empty table when headers provided but no columns (damiandziaduch) - Bug #16897: Fixed `yii\db\sqlite\Schema` missing primary key constraint detection in case of `INTEGER PRIMARY KEY` (bizley) - Bug: (CVE-2018-14578): Fixed CSRF token check bypassing in `\yii\web\Request::getMethod()` (silverfire) +- Bug: (CVE-2018-19454): Fixed excess logging of sensitive information in `\yii\log\Target` (silverfire) 2.0.15.1 March 21, 2018 diff --git a/framework/log/Target.php b/framework/log/Target.php index f49d76ab0e..55e7402341 100644 --- a/framework/log/Target.php +++ b/framework/log/Target.php @@ -73,7 +73,34 @@ abstract class Target extends Component * * @see \yii\helpers\ArrayHelper::filter() */ - public $logVars = ['_GET', '_POST', '_FILES', '_COOKIE', '_SESSION', '_SERVER']; + public $logVars = [ + '_GET', + '_POST', + '_FILES', + '_COOKIE', + '_SESSION', + '_SERVER', + ]; + + /** + * @var array list of the PHP predefined variables that should NOT be logged "as is" and should always be replaced + * with a mask `***` before logging, when exist. + * + * Defaults to `[ '_SERVER.HTTP_AUTHORIZATION', '_SERVER.PHP_AUTH_USER', '_SERVER.PHP_AUTH_PW']` + * + * Each element could be specified as one of the following: + * + * - `var` - `var` will be logged as `***` + * - `var.key` - only `var[key]` will be logged as `***` + * + * @since 2.0.16 + */ + public $maskVars = [ + '_SERVER.HTTP_AUTHORIZATION', + '_SERVER.PHP_AUTH_USER', + '_SERVER.PHP_AUTH_PW', + ]; + /** * @var callable a PHP callable that returns a string to be prefixed to every exported message. * @@ -145,6 +172,11 @@ abstract class Target extends Component protected function getContextMessage() { $context = ArrayHelper::filter($GLOBALS, $this->logVars); + foreach ($this->maskVars as $var) { + if (ArrayHelper::getValue($context, $var) !== null) { + ArrayHelper::setValue($context, $var, '***'); + } + } $result = []; foreach ($context as $key => $value) { $result[] = "\${$key} = " . VarDumper::dumpAsString($value); diff --git a/tests/framework/log/TargetTest.php b/tests/framework/log/TargetTest.php index 332c5ce23c..8819d5091f 100644 --- a/tests/framework/log/TargetTest.php +++ b/tests/framework/log/TargetTest.php @@ -92,6 +92,10 @@ class TargetTest extends TestCase 'C', 'C.C_a', 'D', ], + 'maskVars' => [ + 'C.C_b', + 'D.D_a' + ] ]); $GLOBALS['A'] = [ 'A_a' => 1, @@ -105,7 +109,7 @@ class TargetTest extends TestCase ]; $GLOBALS['C'] = [ 'C_a' => 1, - 'C_b' => 1, + 'C_b' => 'mySecret', 'C_c' => 1, ]; $GLOBALS['E'] = [ @@ -129,6 +133,8 @@ class TargetTest extends TestCase $this->assertNotContains('E_a', $context); $this->assertNotContains('E_b', $context); $this->assertNotContains('E_c', $context); + $this->assertNotContains('mySecret', $context); + $this->assertContains('***', $context); } /** From 8c72db9b48087992e109aaf297d91dd30bafbdc1 Mon Sep 17 00:00:00 2001 From: SilverFire - Dmitry Naumenko Date: Sun, 25 Nov 2018 11:55:49 +0200 Subject: [PATCH 3/3] Enhanced PHPDocs for IdentityInterface::getAuthKey() --- framework/web/IdentityInterface.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/framework/web/IdentityInterface.php b/framework/web/IdentityInterface.php index cf5c0e28ff..fb5765d476 100644 --- a/framework/web/IdentityInterface.php +++ b/framework/web/IdentityInterface.php @@ -82,7 +82,12 @@ interface IdentityInterface * * The space of such keys should be big enough to defeat potential identity attacks. * - * This is required if [[User::enableAutoLogin]] is enabled. + * This is required if [[User::enableAutoLogin]] is enabled. The returned key will be stored on the + * client side as a cookie and will be used to authenticate user even if PHP session has been expired. + * + * Make sure to invalidate earlier issued authKeys when you implement force user logout, password change and + * other scenarios, that require forceful access revocation for old sessions. + * * @return string a key that is used to check the validity of a given identity ID. * @see validateAuthKey() */