Yii2: Empy string in _csrf causes a problem

[lebedevsky]

I'm not sure if this is a bug, but anyway, if in _csrf an empty string is written, if causes a 500: First parameter ($length) must be greater than 0 in yiisoft/yii2/base/Security.php:462, without regenerate token.

        if ($this->_csrfToken === null || $regenerate) {
            if ($regenerate || ($token = $this->loadCsrfToken()) === null) { // here token not generated and empty string write in $token
                $token = $this->generateCsrfToken();
            }
            $this->_csrfToken = Yii::$app->security->maskToken($token); // here in falls with 500 on generateRandomKey(0)
        }

What steps will reproduce the problem?

Write in _csrf string ""

What is the expected result?

Regenerate token

What do you get instead?

Invalid Parameter – yii\base\InvalidParamException

Additional info

| Q | A
| ---------------- | ---
| Yii version | 2.0.13.1
| PHP version | 7.1.10

| Operating system | Linux 9ab5b593e8ae 3.10.0-327.13.1.el7.x86_64 #1 SMP Thu Mar 31 16:04:38 UTC 2016 x86_64

[samdark]

Why writing empty string it it?

[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._

[lebedevsky]

we don't, but somehow we get users with this problem, rarely.
it's hard to find out how it happened

[samdark]

OK. How to reproduce it locally?

[lebedevsky]

only when _csrf in cookie and set cookie value to ""

[samdark]

I've tried that. If that's a regular page, I'm not getting the error. Token in the cookie is regenerated silently. If that's a form submit, I'm getting CSRF validation error as expected.

[SamMousa]

@samdark I can confirm this, am working on a PR that includes a failing test:

 public function testIssue15317()
    {
        $this->mockWebApplication();
        $_COOKIE[(new Request())->csrfParam] = '';
        $request = new Request();
        $request->enableCsrfCookie = true;
        $request->enableCookieValidation = false;
        $this->assertEmpty($request->getCsrfToken());
    }

The question is how to resolve it; I'm leaning towards making maskToken return an empty string in case its input is an empty string. Since an empty string is already uncompressible there should be no harm; but i'm not a security expert.

This is the relevant code: https://github.com/yiisoft/yii2/blob/master/framework/base/Security.php#L725
@tom-- you are the security guru, right? :)

What is your opinion on altering maskToken and unmaskToken to support empty strings?
As far as I can tell, the underlying attack vector, which is that someone could incrementally discover plain text via a partial chosen ciphertext in combination with compression, doesn't apply to empty strings.

[tom--]

CSRF is not really my area. Who was it that gave us the masking algorithm not long ago?

Is it an option to test for this condition and throw an exception? Is there a valid reason to use empty string as CSRF token? It seems to me that an empty string does not qualify as any kind of a token.

[samdark]

I don't think there's no valid reason for it.

[SamMousa]

I agree with that; so the request should refuse CSRF validation when the token is empty.
But that is a reason to throw an exception on the masking function imo.

[samdark]

Not in the security class since it's general masking not specific to CSRF tokens. It does not make sense to mask empty token but it may make sense for other token type.

[tom--]

that makes sense to me

[SamMousa]

While working on a fix, I noted this:
https://github.com/yiisoft/yii2/blame/master/tests/framework/base/SecurityTest.php#L1287

@cebe Are you of the opinion that masking an empty string should always result in an exception?

An alternative solution without changing the Security component, would be to regenerate the CSRF token:
````
public function getCsrfToken($regenerate = false)
{
if ($this->_csrfToken === null || $regenerate) {
if ($regenerate || ($token = $this->loadCsrfToken()) === null) {
$token = $this->generateCsrfToken();
}
$this->_csrfToken = Yii::$app->security->maskToken($token);
}

    return $this->_csrfToken;
}

``` Instead of only checking for=== nullwe could check forempty()` and regenerate?

Interestingly, unmasking the empty string does work..
Which means that the range of maskToken() is not equal to the domain of unmaskToken(), which is not intuitive for a function that is the inverse of another function...

[SamMousa]

@samdark I've fixed the issue without changing the Security component, the discussion about masking / unmasking can then be tabled or continued separately, whichever you prefer!

[samdark]

Regenerating token sounds fine to me.

[tom--]

@SamMousa As I said before, I'm not especially comfortable with CSRF mechanisms, but it seems reasonable to test for empty in your getCsrfToken() in both places where it compares with null.

On a matter of style, I dislike the assignment operator in a condition expression.

[samdark]

That thing I've already fixed in master. Wasn't supported by 5.4.